| Message ID | 20210520073301.1676294-1-ying.huang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [-V2] mm, swap: Remove unnecessary smp_rmb() in swap_type_to_swap_info() | expand |
On Thu, May 20, 2021 at 03:33:01PM +0800, Huang Ying wrote: > Before commit c10d38cc8d3e ("mm, swap: bounds check swap_info array > accesses to avoid NULL derefs"), the typical code to reference the > swap_info[] is as follows, > > type = swp_type(swp_entry); > if (type >= nr_swapfiles) > /* handle invalid swp_entry */; > p = swap_info[type]; > /* access fields of *p. OOPS! p may be NULL! */ > > Because the ordering isn't guaranteed, it's possible that > swap_info[type] is read before "nr_swapfiles". And that may result > in NULL pointer dereference. > > So after commit c10d38cc8d3e, the code becomes, > > struct swap_info_struct *swap_type_to_swap_info(int type) > { > if (type >= READ_ONCE(nr_swapfiles)) > return NULL; > smp_rmb(); > return READ_ONCE(swap_info[type]); > } > > /* users */ > type = swp_type(swp_entry); > p = swap_type_to_swap_info(type); > if (!p) > /* handle invalid swp_entry */; > /* dereference p */ > > Where the value of swap_info[type] (that is, "p") is checked to be > non-zero before being dereferenced. So, the NULL deferencing > becomes impossible even if "nr_swapfiles" is read after > swap_info[type]. Therefore, the "smp_rmb()" becomes unnecessary. > > And, we don't even need to read "nr_swapfiles" here. Because the > non-zero checking for "p" is sufficient. We just need to make sure we > will not access out of the boundary of the array. With the change, > nr_swapfiles will only be accessed with swap_lock held, except in > swapcache_free_entries(). Where the absolute correctness of the value > isn't needed, as described in the comments. > > We still need to guarantee swap_info[type] is read before being > dereferenced. That can be satisfied via the data dependency ordering > enforced by READ_ONCE(swap_info[type]). This needs to be paired with > proper write barriers. So smp_store_release() is used in > alloc_swap_info() to guarantee the fields of *swap_info[type] is > initialized before swap_info[type] itself being written. Note that > the fields of *swap_info[type] is initialized to be 0 via kvzalloc() > firstly. The assignment and deferencing of swap_info[type] is like > rcu_assign_pointer() and rcu_dereference(). > > Signed-off-by: "Huang, Ying" <ying.huang@intel.com> > Cc: Daniel Jordan <daniel.m.jordan@oracle.com> > Cc: Dan Carpenter <dan.carpenter@oracle.com> > Cc: Andrea Parri <andrea.parri@amarulasolutions.com> > Cc: Peter Zijlstra (Intel) <peterz@infradead.org> > Cc: Andi Kleen <ak@linux.intel.com> > Cc: Dave Hansen <dave.hansen@linux.intel.com> > Cc: Omar Sandoval <osandov@fb.com> > Cc: Paul McKenney <paulmck@kernel.org> > Cc: Tejun Heo <tj@kernel.org> > Cc: Will Deacon <will.deacon@arm.com> > Cc: Miaohe Lin <linmiaohe@huawei.com> > > v2: > > - Revise the patch description and comments per Peter's comments. > > --- > mm/swapfile.c | 15 ++++++--------- > 1 file changed, 6 insertions(+), 9 deletions(-) > > diff --git a/mm/swapfile.c b/mm/swapfile.c > index 2aad85751991..65dd979a0f94 100644 > --- a/mm/swapfile.c > +++ b/mm/swapfile.c > @@ -100,11 +100,10 @@ atomic_t nr_rotate_swap = ATOMIC_INIT(0); > > static struct swap_info_struct *swap_type_to_swap_info(int type) > { > - if (type >= READ_ONCE(nr_swapfiles)) > + if (type >= MAX_SWAPFILES) > return NULL; > > - smp_rmb(); /* Pairs with smp_wmb in alloc_swap_info. */ > - return READ_ONCE(swap_info[type]); > + return READ_ONCE(swap_info[type]); /* rcu_dereference() */ > } > > static inline unsigned char swap_count(unsigned char ent) > @@ -2884,14 +2883,12 @@ static struct swap_info_struct *alloc_swap_info(void) > } > if (type >= nr_swapfiles) { > p->type = type; > - WRITE_ONCE(swap_info[type], p); > /* > - * Write swap_info[type] before nr_swapfiles, in case a > - * racing procfs swap_start() or swap_next() is reading them. > - * (We never shrink nr_swapfiles, we never free this entry.) > + * Publish the swap_info_struct after initializing it. > + * Note that kvzalloc() above zeroes all its fields. > */ > - smp_wmb(); > - WRITE_ONCE(nr_swapfiles, nr_swapfiles + 1); > + smp_store_release(&swap_info[type], p); /* rcu_assign_pointer() */ > + nr_swapfiles++; Although I like this change, I comment you are removing refers to some dodgy-looking code. For example, swap_start() has this loop: for (type = 0; (si = swap_type_to_swap_info(type)); type++) { if (!(si->flags & SWP_USED) || !si->swap_map) continue; so won't this just end up dereferencing NULL if nr_swapfiles < MAX_SWAPFILES? I think you need to check all callers of swap_type_to_swap_info() are either validating the 'type' they pass in or check the returned pointer. Will
Hi, Will, Will Deacon <will@kernel.org> writes: > On Thu, May 20, 2021 at 03:33:01PM +0800, Huang Ying wrote: >> Before commit c10d38cc8d3e ("mm, swap: bounds check swap_info array >> accesses to avoid NULL derefs"), the typical code to reference the >> swap_info[] is as follows, >> >> type = swp_type(swp_entry); >> if (type >= nr_swapfiles) >> /* handle invalid swp_entry */; >> p = swap_info[type]; >> /* access fields of *p. OOPS! p may be NULL! */ >> >> Because the ordering isn't guaranteed, it's possible that >> swap_info[type] is read before "nr_swapfiles". And that may result >> in NULL pointer dereference. >> >> So after commit c10d38cc8d3e, the code becomes, >> >> struct swap_info_struct *swap_type_to_swap_info(int type) >> { >> if (type >= READ_ONCE(nr_swapfiles)) >> return NULL; >> smp_rmb(); >> return READ_ONCE(swap_info[type]); >> } >> >> /* users */ >> type = swp_type(swp_entry); >> p = swap_type_to_swap_info(type); >> if (!p) >> /* handle invalid swp_entry */; >> /* dereference p */ >> >> Where the value of swap_info[type] (that is, "p") is checked to be >> non-zero before being dereferenced. So, the NULL deferencing >> becomes impossible even if "nr_swapfiles" is read after >> swap_info[type]. Therefore, the "smp_rmb()" becomes unnecessary. >> >> And, we don't even need to read "nr_swapfiles" here. Because the >> non-zero checking for "p" is sufficient. We just need to make sure we >> will not access out of the boundary of the array. With the change, >> nr_swapfiles will only be accessed with swap_lock held, except in >> swapcache_free_entries(). Where the absolute correctness of the value >> isn't needed, as described in the comments. >> >> We still need to guarantee swap_info[type] is read before being >> dereferenced. That can be satisfied via the data dependency ordering >> enforced by READ_ONCE(swap_info[type]). This needs to be paired with >> proper write barriers. So smp_store_release() is used in >> alloc_swap_info() to guarantee the fields of *swap_info[type] is >> initialized before swap_info[type] itself being written. Note that >> the fields of *swap_info[type] is initialized to be 0 via kvzalloc() >> firstly. The assignment and deferencing of swap_info[type] is like >> rcu_assign_pointer() and rcu_dereference(). >> >> Signed-off-by: "Huang, Ying" <ying.huang@intel.com> >> Cc: Daniel Jordan <daniel.m.jordan@oracle.com> >> Cc: Dan Carpenter <dan.carpenter@oracle.com> >> Cc: Andrea Parri <andrea.parri@amarulasolutions.com> >> Cc: Peter Zijlstra (Intel) <peterz@infradead.org> >> Cc: Andi Kleen <ak@linux.intel.com> >> Cc: Dave Hansen <dave.hansen@linux.intel.com> >> Cc: Omar Sandoval <osandov@fb.com> >> Cc: Paul McKenney <paulmck@kernel.org> >> Cc: Tejun Heo <tj@kernel.org> >> Cc: Will Deacon <will.deacon@arm.com> >> Cc: Miaohe Lin <linmiaohe@huawei.com> >> >> v2: >> >> - Revise the patch description and comments per Peter's comments. >> >> --- >> mm/swapfile.c | 15 ++++++--------- >> 1 file changed, 6 insertions(+), 9 deletions(-) >> >> diff --git a/mm/swapfile.c b/mm/swapfile.c >> index 2aad85751991..65dd979a0f94 100644 >> --- a/mm/swapfile.c >> +++ b/mm/swapfile.c >> @@ -100,11 +100,10 @@ atomic_t nr_rotate_swap = ATOMIC_INIT(0); >> >> static struct swap_info_struct *swap_type_to_swap_info(int type) >> { >> - if (type >= READ_ONCE(nr_swapfiles)) >> + if (type >= MAX_SWAPFILES) >> return NULL; >> >> - smp_rmb(); /* Pairs with smp_wmb in alloc_swap_info. */ >> - return READ_ONCE(swap_info[type]); >> + return READ_ONCE(swap_info[type]); /* rcu_dereference() */ >> } >> >> static inline unsigned char swap_count(unsigned char ent) >> @@ -2884,14 +2883,12 @@ static struct swap_info_struct *alloc_swap_info(void) >> } >> if (type >= nr_swapfiles) { >> p->type = type; >> - WRITE_ONCE(swap_info[type], p); >> /* >> - * Write swap_info[type] before nr_swapfiles, in case a >> - * racing procfs swap_start() or swap_next() is reading them. >> - * (We never shrink nr_swapfiles, we never free this entry.) >> + * Publish the swap_info_struct after initializing it. >> + * Note that kvzalloc() above zeroes all its fields. >> */ >> - smp_wmb(); >> - WRITE_ONCE(nr_swapfiles, nr_swapfiles + 1); >> + smp_store_release(&swap_info[type], p); /* rcu_assign_pointer() */ >> + nr_swapfiles++; > > Although I like this change, I comment you are removing refers to some > dodgy-looking code. For example, swap_start() has this loop: > > for (type = 0; (si = swap_type_to_swap_info(type)); type++) { > if (!(si->flags & SWP_USED) || !si->swap_map) > continue; > > so won't this just end up dereferencing NULL if nr_swapfiles < MAX_SWAPFILES? for (type = 0; (si = swap_type_to_swap_info(type)); type++) { ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Because this is the second sub-statement inside "for ()", I think that "si" will be checked to be non-NULL before executing the statements inside "{}" follows "for ()"? > I think you need to check all callers of swap_type_to_swap_info() are > either validating the 'type' they pass in or check the returned pointer. Yes. I have checked all callers of swap_type_to_swap_info(). One suspecting caller is swap_cluster_readahead(). But after the following patch in mmotm tree, mm/swapfile: use percpu_ref to serialize against concurrent swapoff get/put_swap_device() will enclose the swap_cluster_readahead() to check the swap entry beforehand. Best Regards, Huang, Ying
On Mon, Jun 07, 2021 at 09:49:35AM +0800, Huang, Ying wrote: > Hi, Will, > > Will Deacon <will@kernel.org> writes: > > > On Thu, May 20, 2021 at 03:33:01PM +0800, Huang Ying wrote: > >> Before commit c10d38cc8d3e ("mm, swap: bounds check swap_info array > >> accesses to avoid NULL derefs"), the typical code to reference the > >> swap_info[] is as follows, > >> > >> type = swp_type(swp_entry); > >> if (type >= nr_swapfiles) > >> /* handle invalid swp_entry */; > >> p = swap_info[type]; > >> /* access fields of *p. OOPS! p may be NULL! */ > >> > >> Because the ordering isn't guaranteed, it's possible that > >> swap_info[type] is read before "nr_swapfiles". And that may result > >> in NULL pointer dereference. > >> > >> So after commit c10d38cc8d3e, the code becomes, > >> > >> struct swap_info_struct *swap_type_to_swap_info(int type) > >> { > >> if (type >= READ_ONCE(nr_swapfiles)) > >> return NULL; > >> smp_rmb(); > >> return READ_ONCE(swap_info[type]); > >> } > >> > >> /* users */ > >> type = swp_type(swp_entry); > >> p = swap_type_to_swap_info(type); > >> if (!p) > >> /* handle invalid swp_entry */; > >> /* dereference p */ > >> > >> Where the value of swap_info[type] (that is, "p") is checked to be > >> non-zero before being dereferenced. So, the NULL deferencing > >> becomes impossible even if "nr_swapfiles" is read after > >> swap_info[type]. Therefore, the "smp_rmb()" becomes unnecessary. > >> > >> And, we don't even need to read "nr_swapfiles" here. Because the > >> non-zero checking for "p" is sufficient. We just need to make sure we > >> will not access out of the boundary of the array. With the change, > >> nr_swapfiles will only be accessed with swap_lock held, except in > >> swapcache_free_entries(). Where the absolute correctness of the value > >> isn't needed, as described in the comments. > >> > >> We still need to guarantee swap_info[type] is read before being > >> dereferenced. That can be satisfied via the data dependency ordering > >> enforced by READ_ONCE(swap_info[type]). This needs to be paired with > >> proper write barriers. So smp_store_release() is used in > >> alloc_swap_info() to guarantee the fields of *swap_info[type] is > >> initialized before swap_info[type] itself being written. Note that > >> the fields of *swap_info[type] is initialized to be 0 via kvzalloc() > >> firstly. The assignment and deferencing of swap_info[type] is like > >> rcu_assign_pointer() and rcu_dereference(). > >> > >> Signed-off-by: "Huang, Ying" <ying.huang@intel.com> > >> Cc: Daniel Jordan <daniel.m.jordan@oracle.com> > >> Cc: Dan Carpenter <dan.carpenter@oracle.com> > >> Cc: Andrea Parri <andrea.parri@amarulasolutions.com> > >> Cc: Peter Zijlstra (Intel) <peterz@infradead.org> > >> Cc: Andi Kleen <ak@linux.intel.com> > >> Cc: Dave Hansen <dave.hansen@linux.intel.com> > >> Cc: Omar Sandoval <osandov@fb.com> > >> Cc: Paul McKenney <paulmck@kernel.org> > >> Cc: Tejun Heo <tj@kernel.org> > >> Cc: Will Deacon <will.deacon@arm.com> > >> Cc: Miaohe Lin <linmiaohe@huawei.com> > >> > >> v2: > >> > >> - Revise the patch description and comments per Peter's comments. > >> > >> --- > >> mm/swapfile.c | 15 ++++++--------- > >> 1 file changed, 6 insertions(+), 9 deletions(-) > >> > >> diff --git a/mm/swapfile.c b/mm/swapfile.c > >> index 2aad85751991..65dd979a0f94 100644 > >> --- a/mm/swapfile.c > >> +++ b/mm/swapfile.c > >> @@ -100,11 +100,10 @@ atomic_t nr_rotate_swap = ATOMIC_INIT(0); > >> > >> static struct swap_info_struct *swap_type_to_swap_info(int type) > >> { > >> - if (type >= READ_ONCE(nr_swapfiles)) > >> + if (type >= MAX_SWAPFILES) > >> return NULL; > >> > >> - smp_rmb(); /* Pairs with smp_wmb in alloc_swap_info. */ > >> - return READ_ONCE(swap_info[type]); > >> + return READ_ONCE(swap_info[type]); /* rcu_dereference() */ > >> } > >> > >> static inline unsigned char swap_count(unsigned char ent) > >> @@ -2884,14 +2883,12 @@ static struct swap_info_struct *alloc_swap_info(void) > >> } > >> if (type >= nr_swapfiles) { > >> p->type = type; > >> - WRITE_ONCE(swap_info[type], p); > >> /* > >> - * Write swap_info[type] before nr_swapfiles, in case a > >> - * racing procfs swap_start() or swap_next() is reading them. > >> - * (We never shrink nr_swapfiles, we never free this entry.) > >> + * Publish the swap_info_struct after initializing it. > >> + * Note that kvzalloc() above zeroes all its fields. > >> */ > >> - smp_wmb(); > >> - WRITE_ONCE(nr_swapfiles, nr_swapfiles + 1); > >> + smp_store_release(&swap_info[type], p); /* rcu_assign_pointer() */ > >> + nr_swapfiles++; > > > > Although I like this change, I comment you are removing refers to some > > dodgy-looking code. For example, swap_start() has this loop: > > > > for (type = 0; (si = swap_type_to_swap_info(type)); type++) { > > if (!(si->flags & SWP_USED) || !si->swap_map) > > continue; > > > > so won't this just end up dereferencing NULL if nr_swapfiles < MAX_SWAPFILES? > > for (type = 0; (si = swap_type_to_swap_info(type)); type++) { > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Because this is the second sub-statement inside "for ()", I think that "si" > will be checked to be non-NULL before executing the statements inside > "{}" follows "for ()"? Sorry, yes, you're right. I misread the loop condition. Will
diff --git a/mm/swapfile.c b/mm/swapfile.c index 2aad85751991..65dd979a0f94 100644 --- a/mm/swapfile.c +++ b/mm/swapfile.c @@ -100,11 +100,10 @@ atomic_t nr_rotate_swap = ATOMIC_INIT(0); static struct swap_info_struct *swap_type_to_swap_info(int type) { - if (type >= READ_ONCE(nr_swapfiles)) + if (type >= MAX_SWAPFILES) return NULL; - smp_rmb(); /* Pairs with smp_wmb in alloc_swap_info. */ - return READ_ONCE(swap_info[type]); + return READ_ONCE(swap_info[type]); /* rcu_dereference() */ } static inline unsigned char swap_count(unsigned char ent) @@ -2884,14 +2883,12 @@ static struct swap_info_struct *alloc_swap_info(void) } if (type >= nr_swapfiles) { p->type = type; - WRITE_ONCE(swap_info[type], p); /* - * Write swap_info[type] before nr_swapfiles, in case a - * racing procfs swap_start() or swap_next() is reading them. - * (We never shrink nr_swapfiles, we never free this entry.) + * Publish the swap_info_struct after initializing it. + * Note that kvzalloc() above zeroes all its fields. */ - smp_wmb(); - WRITE_ONCE(nr_swapfiles, nr_swapfiles + 1); + smp_store_release(&swap_info[type], p); /* rcu_assign_pointer() */ + nr_swapfiles++; } else { defer = p; p = swap_info[type];
Before commit c10d38cc8d3e ("mm, swap: bounds check swap_info array accesses to avoid NULL derefs"), the typical code to reference the swap_info[] is as follows, type = swp_type(swp_entry); if (type >= nr_swapfiles) /* handle invalid swp_entry */; p = swap_info[type]; /* access fields of *p. OOPS! p may be NULL! */ Because the ordering isn't guaranteed, it's possible that swap_info[type] is read before "nr_swapfiles". And that may result in NULL pointer dereference. So after commit c10d38cc8d3e, the code becomes, struct swap_info_struct *swap_type_to_swap_info(int type) { if (type >= READ_ONCE(nr_swapfiles)) return NULL; smp_rmb(); return READ_ONCE(swap_info[type]); } /* users */ type = swp_type(swp_entry); p = swap_type_to_swap_info(type); if (!p) /* handle invalid swp_entry */; /* dereference p */ Where the value of swap_info[type] (that is, "p") is checked to be non-zero before being dereferenced. So, the NULL deferencing becomes impossible even if "nr_swapfiles" is read after swap_info[type]. Therefore, the "smp_rmb()" becomes unnecessary. And, we don't even need to read "nr_swapfiles" here. Because the non-zero checking for "p" is sufficient. We just need to make sure we will not access out of the boundary of the array. With the change, nr_swapfiles will only be accessed with swap_lock held, except in swapcache_free_entries(). Where the absolute correctness of the value isn't needed, as described in the comments. We still need to guarantee swap_info[type] is read before being dereferenced. That can be satisfied via the data dependency ordering enforced by READ_ONCE(swap_info[type]). This needs to be paired with proper write barriers. So smp_store_release() is used in alloc_swap_info() to guarantee the fields of *swap_info[type] is initialized before swap_info[type] itself being written. Note that the fields of *swap_info[type] is initialized to be 0 via kvzalloc() firstly. The assignment and deferencing of swap_info[type] is like rcu_assign_pointer() and rcu_dereference(). Signed-off-by: "Huang, Ying" <ying.huang@intel.com> Cc: Daniel Jordan <daniel.m.jordan@oracle.com> Cc: Dan Carpenter <dan.carpenter@oracle.com> Cc: Andrea Parri <andrea.parri@amarulasolutions.com> Cc: Peter Zijlstra (Intel) <peterz@infradead.org> Cc: Andi Kleen <ak@linux.intel.com> Cc: Dave Hansen <dave.hansen@linux.intel.com> Cc: Omar Sandoval <osandov@fb.com> Cc: Paul McKenney <paulmck@kernel.org> Cc: Tejun Heo <tj@kernel.org> Cc: Will Deacon <will.deacon@arm.com> Cc: Miaohe Lin <linmiaohe@huawei.com> v2: - Revise the patch description and comments per Peter's comments. --- mm/swapfile.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)