Hi, Daniel,
Daniel Jordan <daniel.m.jordan@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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
>>
