mm, swap: Potential NULL dereference in get_swap_page_of_type()
diff mbox series

Message ID 20190111095919.GA1757@kadam
State New
Headers show
Series
  • mm, swap: Potential NULL dereference in get_swap_page_of_type()
Related show

Commit Message

Dan Carpenter Jan. 11, 2019, 9:59 a.m. UTC
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(-)

Comments

Daniel Jordan Jan. 11, 2019, 5:41 p.m. UTC | #1
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
>
Andrea Parri Jan. 11, 2019, 11:20 p.m. UTC | #2
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
> >
Huang\, Ying Jan. 14, 2019, 2:12 a.m. UTC | #3
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
>>
Dan Carpenter Jan. 14, 2019, 8:43 a.m. UTC | #4
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
Daniel Jordan Jan. 14, 2019, 10:25 p.m. UTC | #5
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];
Daniel Jordan Jan. 14, 2019, 11:40 p.m. UTC | #6
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.
Andrea Parri Jan. 15, 2019, 12:28 a.m. UTC | #7
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];

Patch
diff mbox series

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};
 }