Re: [PATCH] mm, swap: Potential NULL dereference in get_swap_page_of_type()

[Daniel Jordan <daniel.m.jordan@oracle.com>]

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
>