Re: [RFC PATCH v2] xen/privcmd: Convert get_user_pages*() to pin_user_pages*()

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 6/23/20 9:36 PM, Souptick Joarder wrote:
> On Tue, Jun 23, 2020 at 11:11 PM Boris Ostrovsky
> <boris.ostrovsky@xxxxxxxxxx> wrote:
>> On 6/23/20 7:58 AM, Souptick Joarder wrote:
>>> In 2019, we introduced pin_user_pages*() and now we are converting
>>> get_user_pages*() to the new API as appropriate. [1] & [2] could
>>> be referred for more information. This is case 5 as per document [1].
>>>
>>> As discussed, pages need to be marked as dirty before unpinned it.
>>>
>>> Previously, if lock_pages() end up partially mapping pages, it used
>>> to return -ERRNO due to which unlock_pages() have to go through
>>> each pages[i] till *nr_pages* to validate them. This can be avoided
>>> by passing correct number partially mapped pages & -ERRNO separately
>>> while returning from lock_pages() due to error.
>>> With this fix unlock_pages() doesn't need to validate pages[i] till
>>> *nr_pages* for error scenario.
>>
>> This should be split into two patches please. The first one will fix the
>> return value bug (and will need to go to stable branches) and the second
>> will use new routine to pin pages.
> Initially I split the patches into 2 commits. But at last moment I
> figure out that,
> this bug fix ( better to call coding error, doesn't looks like lead to
> any runtime bug) is tightly coupled to 2nd commit for
> pin_user_pages*() conversion,
> which means we don't need the bug fix patch if we are not converting the API 
> to
> pin_user_pages*()/ unpin_user_pages_dirty_lock(). That's the reason to
> clubbed these two
> commits into a single one.


I am not sure I understand why the two issues are connected. Failure of
either get_user_pages_fast() or pin_user_pages() will result in the same
kind of overall ioctl failure, won't it?


One concern though is that we are changing how user will see this error.
Currently Xen devicemodel (which AFAIK is the only caller) ignores it,
which is I think is wrong. So another option would be to fix this in Xen
and continue returning positive number here. I guess it's back to Paul
again.


-boris