| Message ID | 20231119194740.94101-2-ryncsn@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Swapin path refactor for optimization and bugfix | expand |
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.
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
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.
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 --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;