diff mbox series

[01/24] mm/swap: fix a potential undefined behavior issue

Message ID 20231119194740.94101-2-ryncsn@gmail.com
State New
Headers
Series Swapin path refactor for optimization and bugfix |

Commit Message

Kairui Song Nov. 19, 2023, 7:47 p.m. UTC 
  From: Kairui Song <kasong@tencent.com>

When folio is NULL, taking the address of its struct member is an
undefined behavior, the UB is caused by applying -> operator
to a pointer not pointing to any object. Although in practice this
won't lead to a real issue, still better to fix it, also makes the
code less error-prone, when folio is NULL, page is also NULL,
instead of a meanless offset value.

Signed-off-by: Kairui Song <kasong@tencent.com>
---
 mm/memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Matthew Wilcox Nov. 19, 2023, 8:55 p.m. UTC | #1 
On Mon, Nov 20, 2023 at 03:47:17AM +0800, Kairui Song wrote:
> From: Kairui Song <kasong@tencent.com>
> 
> When folio is NULL, taking the address of its struct member is an
> undefined behavior, the UB is caused by applying -> operator
> to a pointer not pointing to any object. Although in practice this
> won't lead to a real issue, still better to fix it, also makes the
> code less error-prone, when folio is NULL, page is also NULL,
> instead of a meanless offset value.

Um, &folio->page is NULL if folio is NULL.  The offset of 'page' within
'folio' is 0.  By definition; and this will never change.
Chris Li Nov. 20, 2023, 3:35 a.m. UTC | #2 
Hi Kairui,

On Sun, Nov 19, 2023 at 12:55 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Mon, Nov 20, 2023 at 03:47:17AM +0800, Kairui Song wrote:
> > From: Kairui Song <kasong@tencent.com>
> >
> > When folio is NULL, taking the address of its struct member is an
> > undefined behavior, the UB is caused by applying -> operator

I think dereferencing the NULL pointer is undefined behavior. There is
no dereferencing here. It is just pointer arithmetic of NULL pointers,
which is adding offset of page to the NULL pointer, you got NULL.

> > won't lead to a real issue, still better to fix it, also makes the
> > code less error-prone, when folio is NULL, page is also NULL,
> > instead of a meanless offset value.

I consider your reasoning is invalid. NULL pointer arithmetic should
be legal. This patch is not needed.

Chris
Kairui Song Nov. 20, 2023, 11:14 a.m. UTC | #3 
Chris Li <chrisl@kernel.org> 于2023年11月20日周一 11:36写道：
>
> Hi Kairui,
>
> On Sun, Nov 19, 2023 at 12:55 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Mon, Nov 20, 2023 at 03:47:17AM +0800, Kairui Song wrote:
> > > From: Kairui Song <kasong@tencent.com>
> > >
> > > When folio is NULL, taking the address of its struct member is an
> > > undefined behavior, the UB is caused by applying -> operator
>
> I think dereferencing the NULL pointer is undefined behavior. There is
> no dereferencing here. It is just pointer arithmetic of NULL pointers,
> which is adding offset of page to the NULL pointer, you got NULL.
>
> > > won't lead to a real issue, still better to fix it, also makes the
> > > code less error-prone, when folio is NULL, page is also NULL,
> > > instead of a meanless offset value.
>
> I consider your reasoning is invalid. NULL pointer arithmetic should
> be legal. This patch is not needed.
>
> Chris

Hi, Chris and Matthew.

Thanks for the comments.

Right, it's just a language syntax level thing, since "->" have a
higher priority, so in the syntax level it is doing a member access
first, then take the address. By C definition  member access should
not happen if the object is invalid (NULL). Only a hypothesis problem
on paper...

This is indeed not needed since in reality it's just pointer
arithmetic. I'm OK dropping this.
Chris Li Nov. 20, 2023, 5:34 p.m. UTC | #4 
Hi Kairui,

On Mon, Nov 20, 2023 at 3:15 AM Kairui Song <ryncsn@gmail.com> wrote:
> > Chris
>
> Hi, Chris and Matthew.
>
> Thanks for the comments.
>
> Right, it's just a language syntax level thing, since "->" have a
> higher priority, so in the syntax level it is doing a member access
> first, then take the address. By C definition  member access should
> not happen if the object is invalid (NULL). Only a hypothesis problem
> on paper...

The dereference only shows up in the abstract syntax tree level.
According to the C standard there are expansion and evaluation phases
after that. At the evaluation phase the dereference will turn into
pointer arithmetic. Per my understanding, the dereference never
actually happens, due to the evaluation rules, not even in theory.

> This is indeed not needed since in reality it's just pointer
> arithmetic. I'm OK dropping this.

Thanks

Chris
diff mbox series

Patch

diff --git a/mm/memory.c b/mm/memory.c
index e27e2e5beb3f..70ffa867b1be 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3861,7 +3861,6 @@  vm_fault_t do_swap_page(struct vm_fault *vmf)
 			/* skip swapcache */
 			folio = vma_alloc_folio(GFP_HIGHUSER_MOVABLE, 0,
 						vma, vmf->address, false);
-			page = &folio->page;
 			if (folio) {
 				__folio_set_locked(folio);
 				__folio_set_swapbacked(folio);
@@ -3879,6 +3878,7 @@  vm_fault_t do_swap_page(struct vm_fault *vmf)
 					workingset_refault(folio, shadow);
 
 				folio_add_lru(folio);
+				page = &folio->page;
 
 				/* To provide entry to swap_readpage() */
 				folio->swap = entry;