| Message ID | 20190111095919.GA1757@kadam (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm, swap: Potential NULL dereference in get_swap_page_of_type() | expand |
On Fri, Jan 11, 2019 at 12:59:19PM +0300, Dan Carpenter wrote: > Smatch complains that the NULL checks on "si" aren't consistent. This > seems like a real bug because we have not ensured that the type is > valid and so "si" can be NULL. > > Fixes: ec8acf20afb8 ("swap: add per-partition lock for swapfile") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > mm/swapfile.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/mm/swapfile.c b/mm/swapfile.c > index f0edf7244256..21e92c757205 100644 > --- a/mm/swapfile.c > +++ b/mm/swapfile.c > @@ -1048,9 +1048,12 @@ swp_entry_t get_swap_page_of_type(int type) > struct swap_info_struct *si; > pgoff_t offset; > > + if (type >= nr_swapfiles) > + goto fail; > + As long as we're worrying about NULL, I think there should be an smp_rmb here to ensure swap_info[type] isn't NULL in case of an (admittedly unlikely) racing swapon that increments nr_swapfiles. See smp_wmb in alloc_swap_info and the matching smp_rmb's in the file. And READ_ONCE's on either side of the barrier per LKMM. I'm adding Andrea (randomly selected from the many LKMM folks to avoid spamming all) who can correct me if I'm wrong about any of this. > si = swap_info[type]; > spin_lock(&si->lock); > - if (si && (si->flags & SWP_WRITEOK)) { > + if (si->flags & SWP_WRITEOK) { > atomic_long_dec(&nr_swap_pages); > /* This is called for allocating swap entry, not cache */ > offset = scan_swap_map(si, 1); > @@ -1061,6 +1064,7 @@ swp_entry_t get_swap_page_of_type(int type) > atomic_long_inc(&nr_swap_pages); > } > spin_unlock(&si->lock); > +fail: > return (swp_entry_t) {0}; > } > > -- > 2.17.1 >
Hi Daniel, On Fri, Jan 11, 2019 at 09:41:28AM -0800, Daniel Jordan wrote: > On Fri, Jan 11, 2019 at 12:59:19PM +0300, Dan Carpenter wrote: > > Smatch complains that the NULL checks on "si" aren't consistent. This > > seems like a real bug because we have not ensured that the type is > > valid and so "si" can be NULL. > > > > Fixes: ec8acf20afb8 ("swap: add per-partition lock for swapfile") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > mm/swapfile.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/mm/swapfile.c b/mm/swapfile.c > > index f0edf7244256..21e92c757205 100644 > > --- a/mm/swapfile.c > > +++ b/mm/swapfile.c > > @@ -1048,9 +1048,12 @@ swp_entry_t get_swap_page_of_type(int type) > > struct swap_info_struct *si; > > pgoff_t offset; > > > > + if (type >= nr_swapfiles) > > + goto fail; > > + > > As long as we're worrying about NULL, I think there should be an smp_rmb here > to ensure swap_info[type] isn't NULL in case of an (admittedly unlikely) racing > swapon that increments nr_swapfiles. See smp_wmb in alloc_swap_info and the > matching smp_rmb's in the file. And READ_ONCE's on either side of the barrier > per LKMM. > > I'm adding Andrea (randomly selected from the many LKMM folks to avoid spamming > all) who can correct me if I'm wrong about any of this. This is to confirm that your analysis seems correct to me: the barriers should guarantee that get_swap_page_of_type() will observe the store to swap_info[type] performed by alloc_swap_info() (or a "co"-later store), provided get_swap_page_of_type() observes the increment of nr_swapfiles performed by the (same instance of) alloc_swap_info(). One clarification about the READ_ONCE() matter: the LKMM cannot handle plain or unmarked (shared memory) accesses in their generality at the moment (patches providing support for these accesses are in the making, but they will take some time); IAC, I'm confident to anticipate that, for the particular pattern in question (aka, MP), marking the accesses to nr_swapfiles will be "LKMM-sane" (one way to achieve this would be to convert nr_swapfiles to an atomic_t type...). I take the liberty of adding other LKMM folks (so that they can blame me for "the spam"! ;-) ): I've learnt from experience that four or more eyes are better than two when it comes to discuss these matters... ;-) Andrea > > > si = swap_info[type]; > > spin_lock(&si->lock); > > - if (si && (si->flags & SWP_WRITEOK)) { > > + if (si->flags & SWP_WRITEOK) { > > atomic_long_dec(&nr_swap_pages); > > /* This is called for allocating swap entry, not cache */ > > offset = scan_swap_map(si, 1); > > @@ -1061,6 +1064,7 @@ swp_entry_t get_swap_page_of_type(int type) > > atomic_long_inc(&nr_swap_pages); > > } > > spin_unlock(&si->lock); > > +fail: > > return (swp_entry_t) {0}; > > } > > > > -- > > 2.17.1 > >
Hi, Daniel, Daniel Jordan <daniel.m.jordan@oracle.com> writes: > On Fri, Jan 11, 2019 at 12:59:19PM +0300, Dan Carpenter wrote: >> Smatch complains that the NULL checks on "si" aren't consistent. This >> seems like a real bug because we have not ensured that the type is >> valid and so "si" can be NULL. >> >> Fixes: ec8acf20afb8 ("swap: add per-partition lock for swapfile") >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >> --- >> mm/swapfile.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/mm/swapfile.c b/mm/swapfile.c >> index f0edf7244256..21e92c757205 100644 >> --- a/mm/swapfile.c >> +++ b/mm/swapfile.c >> @@ -1048,9 +1048,12 @@ swp_entry_t get_swap_page_of_type(int type) >> struct swap_info_struct *si; >> pgoff_t offset; >> >> + if (type >= nr_swapfiles) >> + goto fail; >> + > > As long as we're worrying about NULL, I think there should be an smp_rmb here > to ensure swap_info[type] isn't NULL in case of an (admittedly unlikely) racing > swapon that increments nr_swapfiles. See smp_wmb in alloc_swap_info and the > matching smp_rmb's in the file. And READ_ONCE's on either side of the barrier > per LKMM. I think you are right here. And smp_rmb() for nr_swapfiles are missing in many other places in swapfile.c too (e.g. __swap_info_get(), swapdev_block(), etc.). In theory, I think we need to fix this. Best Regards, Huang, Ying > I'm adding Andrea (randomly selected from the many LKMM folks to avoid spamming > all) who can correct me if I'm wrong about any of this. > >> si = swap_info[type]; >> spin_lock(&si->lock); >> - if (si && (si->flags & SWP_WRITEOK)) { >> + if (si->flags & SWP_WRITEOK) { >> atomic_long_dec(&nr_swap_pages); >> /* This is called for allocating swap entry, not cache */ >> offset = scan_swap_map(si, 1); >> @@ -1061,6 +1064,7 @@ swp_entry_t get_swap_page_of_type(int type) >> atomic_long_inc(&nr_swap_pages); >> } >> spin_unlock(&si->lock); >> +fail: >> return (swp_entry_t) {0}; >> } >> >> -- >> 2.17.1 >>
I'm really terribly ignorant when it comes to things like this... To me it looked like the barrier in alloc_swap_info() was enough but when so many smarter people disagree then I must be wrong. I'd like to help, but I sort of feel unqualified. Could someone else take care of it? regards, dan carpenter
On Sat, Jan 12, 2019 at 12:20:07AM +0100, Andrea Parri wrote: > On Fri, Jan 11, 2019 at 09:41:28AM -0800, Daniel Jordan wrote: > > On Fri, Jan 11, 2019 at 12:59:19PM +0300, Dan Carpenter wrote: > > > diff --git a/mm/swapfile.c b/mm/swapfile.c > > > index f0edf7244256..21e92c757205 100644 > > > --- a/mm/swapfile.c > > > +++ b/mm/swapfile.c > > > @@ -1048,9 +1048,12 @@ swp_entry_t get_swap_page_of_type(int type) > > > struct swap_info_struct *si; > > > pgoff_t offset; > > > > > > + if (type >= nr_swapfiles) > > > + goto fail; > > > + > > > > As long as we're worrying about NULL, I think there should be an smp_rmb here > > to ensure swap_info[type] isn't NULL in case of an (admittedly unlikely) racing > > swapon that increments nr_swapfiles. See smp_wmb in alloc_swap_info and the > > matching smp_rmb's in the file. And READ_ONCE's on either side of the barrier > > per LKMM. > > > > I'm adding Andrea (randomly selected from the many LKMM folks to avoid spamming > > all) who can correct me if I'm wrong about any of this. > > This is to confirm that your analysis seems correct to me: the barriers > should guarantee that get_swap_page_of_type() will observe the store to > swap_info[type] performed by alloc_swap_info() (or a "co"-later store), > provided get_swap_page_of_type() observes the increment of nr_swapfiles > performed by the (same instance of) alloc_swap_info(). That's good to hear, thanks for looking into it. > One clarification about the READ_ONCE() matter: the LKMM cannot handle > plain or unmarked (shared memory) accesses in their generality at the > moment (patches providing support for these accesses are in the making, > but they will take some time); IAC, I'm confident to anticipate that, > for the particular pattern in question (aka, MP), marking the accesses > to nr_swapfiles will be "LKMM-sane" (one way to achieve this would be > to convert nr_swapfiles to an atomic_t type...). I guess you mean we could either use READ_ONCE or make nr_swapfiles atomic, they're different ways of achieving the same thing. > I take the liberty of adding other LKMM folks (so that they can blame > me for "the spam"! ;-) ): I've learnt from experience that four or more > eyes are better than two when it comes to discuss these matters... ;-) Ok, it's fine with me as long as they blame you :) > > > si = swap_info[type];
On Mon, Jan 14, 2019 at 11:43:10AM +0300, Dan Carpenter wrote: > I'm really terribly ignorant when it comes to things like this... To me > it looked like the barrier in alloc_swap_info() was enough but when so > many smarter people disagree then I must be wrong. I'd like to help, > but I sort of feel unqualified. > > Could someone else take care of it? I'm not the most qualified person either, but I gave it a try anyway. Patch to follow.
On Mon, Jan 14, 2019 at 02:25:29PM -0800, Daniel Jordan wrote: > On Sat, Jan 12, 2019 at 12:20:07AM +0100, Andrea Parri wrote: > > On Fri, Jan 11, 2019 at 09:41:28AM -0800, Daniel Jordan wrote: > > > On Fri, Jan 11, 2019 at 12:59:19PM +0300, Dan Carpenter wrote: > > > > diff --git a/mm/swapfile.c b/mm/swapfile.c > > > > index f0edf7244256..21e92c757205 100644 > > > > --- a/mm/swapfile.c > > > > +++ b/mm/swapfile.c > > > > @@ -1048,9 +1048,12 @@ swp_entry_t get_swap_page_of_type(int type) > > > > struct swap_info_struct *si; > > > > pgoff_t offset; > > > > > > > > + if (type >= nr_swapfiles) > > > > + goto fail; > > > > + > > > > > > As long as we're worrying about NULL, I think there should be an smp_rmb here > > > to ensure swap_info[type] isn't NULL in case of an (admittedly unlikely) racing > > > swapon that increments nr_swapfiles. See smp_wmb in alloc_swap_info and the > > > matching smp_rmb's in the file. And READ_ONCE's on either side of the barrier > > > per LKMM. > > > > > > I'm adding Andrea (randomly selected from the many LKMM folks to avoid spamming > > > all) who can correct me if I'm wrong about any of this. > > > > This is to confirm that your analysis seems correct to me: the barriers > > should guarantee that get_swap_page_of_type() will observe the store to > > swap_info[type] performed by alloc_swap_info() (or a "co"-later store), > > provided get_swap_page_of_type() observes the increment of nr_swapfiles > > performed by the (same instance of) alloc_swap_info(). > > That's good to hear, thanks for looking into it. > > > One clarification about the READ_ONCE() matter: the LKMM cannot handle > > plain or unmarked (shared memory) accesses in their generality at the > > moment (patches providing support for these accesses are in the making, > > but they will take some time); IAC, I'm confident to anticipate that, > > for the particular pattern in question (aka, MP), marking the accesses > > to nr_swapfiles will be "LKMM-sane" (one way to achieve this would be > > to convert nr_swapfiles to an atomic_t type...). > > I guess you mean we could either use READ_ONCE or make nr_swapfiles atomic, > they're different ways of achieving the same thing. Indeed: I was suggesting to mark the read _and the increment of nr_swapfiles, as I see you did in the patch you just submitted. Andrea > > swap_info[type] performed by alloc_swap_info() (or a "co"-later store), > > > I take the liberty of adding other LKMM folks (so that they can blame > > me for "the spam"! ;-) ): I've learnt from experience that four or more > > eyes are better than two when it comes to discuss these matters... ;-) > > Ok, it's fine with me as long as they blame you :) > > > > > si = swap_info[type];
diff --git a/mm/swapfile.c b/mm/swapfile.c index f0edf7244256..21e92c757205 100644 --- a/mm/swapfile.c +++ b/mm/swapfile.c @@ -1048,9 +1048,12 @@ swp_entry_t get_swap_page_of_type(int type) struct swap_info_struct *si; pgoff_t offset; + if (type >= nr_swapfiles) + goto fail; + si = swap_info[type]; spin_lock(&si->lock); - if (si && (si->flags & SWP_WRITEOK)) { + if (si->flags & SWP_WRITEOK) { atomic_long_dec(&nr_swap_pages); /* This is called for allocating swap entry, not cache */ offset = scan_swap_map(si, 1); @@ -1061,6 +1064,7 @@ swp_entry_t get_swap_page_of_type(int type) atomic_long_inc(&nr_swap_pages); } spin_unlock(&si->lock); +fail: return (swp_entry_t) {0}; }
Smatch complains that the NULL checks on "si" aren't consistent. This seems like a real bug because we have not ensured that the type is valid and so "si" can be NULL. Fixes: ec8acf20afb8 ("swap: add per-partition lock for swapfile") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- mm/swapfile.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)