Re: [PATCH -V5 RESEND 03/21] swap: Support PMD swap mapping in swap_duplicate()

[Huang\, Ying]

Daniel Jordan <daniel.m.jordan@xxxxxxxxxx> writes:

> On Wed, Sep 26, 2018 at 08:55:59PM +0800, Huang, Ying wrote:
>> Daniel Jordan <daniel.m.jordan@xxxxxxxxxx> writes:
>> > On Tue, Sep 25, 2018 at 03:13:30PM +0800, Huang Ying wrote:
>> >>  /*
>> >>   * Increase reference count of swap entry by 1.
>> >> - * Returns 0 for success, or -ENOMEM if a swap_count_continuation is required
>> >> - * but could not be atomically allocated.  Returns 0, just as if it succeeded,
>> >> - * if __swap_duplicate() fails for another reason (-EINVAL or -ENOENT), which
>> >> - * might occur if a page table entry has got corrupted.
>> >> + *
>> >> + * Return error code in following case.
>> >> + * - success -> 0
>> >> + * - swap_count_continuation is required but could not be atomically allocated.
>> >> + *   *entry is used to return swap entry to call add_swap_count_continuation().
>> >> + *								      -> ENOMEM
>> >> + * - otherwise same as __swap_duplicate()
>> >>   */
>> >> -int swap_duplicate(swp_entry_t entry)
>> >> +int swap_duplicate(swp_entry_t *entry, int entry_size)
>> >>  {
>> >>  	int err = 0;
>> >>  
>> >> -	while (!err && __swap_duplicate(entry, 1) == -ENOMEM)
>> >> -		err = add_swap_count_continuation(entry, GFP_ATOMIC);
>> >> +	while (!err &&
>> >> +	       (err = __swap_duplicate(entry, entry_size, 1)) == -ENOMEM)
>> >> +		err = add_swap_count_continuation(*entry, GFP_ATOMIC);
>> >>  	return err;
>> >
>> > Now we're returning any error we get from __swap_duplicate, apparently to
>> > accommodate ENOTDIR later in the series, which is a change from the behavior
>> > introduced in 570a335b8e22 ("swap_info: swap count continuations").  This might
>> > belong in a separate patch given its potential for side effects.
>> 
>> I have checked all the calls of the function and found there will be no
>> bad effect.  Do you have any side effect?
>
> Before I was just being vaguely concerned about any unintended side effects,
> but looking again, yes I do.
>
> Now when swap_duplicate returns an error in copy_one_pte, copy_one_pte returns
> a (potentially nonzero) entry.val, which copy_pte_range interprets
> unconditionally as 'try adding a swap count continuation.'  Not what we want
> for returns other than -ENOMEM.

Thanks for pointing this out!  Before the change in the patchset, the
behavior is,

Something wrong is detected in swap_duplicate(), but the error is
ignored.  Then copy_one_pte() will think everything is OK, so that it
can proceed to call set_pte_at().  The system will be in inconsistent
state and some data may be polluted!

But this doesn't cause any problem in practical.  Per my understanding,
because if other part of the kernel works correctly, it's impossible for
swap_duplicate() return any error except -ENOMEM before the change in
this patchset.  But the error may be possible during development, and it
may serve as some kind of document too.  So I suggest to add

VM_BUG_ON(error != -ENOMEM);

in swap_duplicate().  What do you think about that?

> So it might make sense to have a separate patch that changes swap_duplicate's
> return and makes callers handle it.

Thanks for your help to take a deep look at this.  I want to try to fix
all potential problems firstly, because the number of the caller is
quite limited.  Do you agree?

Best Regards,
Huang, Ying