| Message ID | 1594059372-15563-3-git-send-email-jrdr.linux@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Few bug fixes and Convert to pin_user_pages*() | expand |
On 06.07.20 20:16, Souptick Joarder wrote: > pages need to be marked as dirty before unpinned it in > unlock_pages() which was oversight. This is fixed now. > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > Suggested-by: John Hubbard <jhubbard@nvidia.com> > Cc: John Hubbard <jhubbard@nvidia.com> > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> > Cc: Paul Durrant <xadimgnik@gmail.com> > --- > drivers/xen/privcmd.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c > index 33677ea..f6c1543 100644 > --- a/drivers/xen/privcmd.c > +++ b/drivers/xen/privcmd.c > @@ -612,8 +612,11 @@ static void unlock_pages(struct page *pages[], unsigned int nr_pages) > { > unsigned int i; > > - for (i = 0; i < nr_pages; i++) > + for (i = 0; i < nr_pages; i++) { > + if (!PageDirty(pages[i])) > + set_page_dirty_lock(pages[i]); With put_page() directly following I think you should be able to use set_page_dirty() instead, as there is obviously a reference to the page existing. > put_page(pages[i]); > + } > } > > static long privcmd_ioctl_dm_op(struct file *file, void __user *udata) > Juergen
On Tue, Jul 7, 2020 at 3:08 PM Jürgen Groß <jgross@suse.com> wrote: > > On 06.07.20 20:16, Souptick Joarder wrote: > > pages need to be marked as dirty before unpinned it in > > unlock_pages() which was oversight. This is fixed now. > > > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > > Suggested-by: John Hubbard <jhubbard@nvidia.com> > > Cc: John Hubbard <jhubbard@nvidia.com> > > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > Cc: Paul Durrant <xadimgnik@gmail.com> > > --- > > drivers/xen/privcmd.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c > > index 33677ea..f6c1543 100644 > > --- a/drivers/xen/privcmd.c > > +++ b/drivers/xen/privcmd.c > > @@ -612,8 +612,11 @@ static void unlock_pages(struct page *pages[], unsigned int nr_pages) > > { > > unsigned int i; > > > > - for (i = 0; i < nr_pages; i++) > > + for (i = 0; i < nr_pages; i++) { > > + if (!PageDirty(pages[i])) > > + set_page_dirty_lock(pages[i]); > > With put_page() directly following I think you should be able to use > set_page_dirty() instead, as there is obviously a reference to the page > existing. Patch [3/3] will convert above codes to use unpin_user_pages_dirty_lock() which internally do the same check. So I thought to keep linux-stable and linux-next code in sync. John had a similar concern [1] and later agreed to keep this check. Shall I keep this check ? No ? [1] https://lore.kernel.org/xen-devel/a750e5e5-fd5d-663b-c5fd-261d7c939ba7@nvidia.com/ > > > put_page(pages[i]); > > + } > > } > > > > static long privcmd_ioctl_dm_op(struct file *file, void __user *udata) > > > > Juergen
On 07.07.20 13:30, Souptick Joarder wrote: > On Tue, Jul 7, 2020 at 3:08 PM Jürgen Groß <jgross@suse.com> wrote: >> >> On 06.07.20 20:16, Souptick Joarder wrote: >>> pages need to be marked as dirty before unpinned it in >>> unlock_pages() which was oversight. This is fixed now. >>> >>> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> >>> Suggested-by: John Hubbard <jhubbard@nvidia.com> >>> Cc: John Hubbard <jhubbard@nvidia.com> >>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> >>> Cc: Paul Durrant <xadimgnik@gmail.com> >>> --- >>> drivers/xen/privcmd.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c >>> index 33677ea..f6c1543 100644 >>> --- a/drivers/xen/privcmd.c >>> +++ b/drivers/xen/privcmd.c >>> @@ -612,8 +612,11 @@ static void unlock_pages(struct page *pages[], unsigned int nr_pages) >>> { >>> unsigned int i; >>> >>> - for (i = 0; i < nr_pages; i++) >>> + for (i = 0; i < nr_pages; i++) { >>> + if (!PageDirty(pages[i])) >>> + set_page_dirty_lock(pages[i]); >> >> With put_page() directly following I think you should be able to use >> set_page_dirty() instead, as there is obviously a reference to the page >> existing. > > Patch [3/3] will convert above codes to use unpin_user_pages_dirty_lock() > which internally do the same check. So I thought to keep linux-stable and > linux-next code in sync. John had a similar concern [1] and later agreed to keep > this check. > > Shall I keep this check ? No ? > > [1] https://lore.kernel.org/xen-devel/a750e5e5-fd5d-663b-c5fd-261d7c939ba7@nvidia.com/ I wasn't referring to checking PageDirty(), but to the use of set_page_dirty_lock(). Looking at the comment just before the implementation of set_page_dirty_lock() suggests that it is fine to use set_page_dirty() instead (so not calling lock_page()). Only the transition from get_user_pages_fast() to pin_user_pages_fast() requires to use the locked version IMO. Juergen
On 2020-07-07 04:43, Jürgen Groß wrote: > On 07.07.20 13:30, Souptick Joarder wrote: >> On Tue, Jul 7, 2020 at 3:08 PM Jürgen Groß <jgross@suse.com> wrote: ... >>>> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c >>>> index 33677ea..f6c1543 100644 >>>> --- a/drivers/xen/privcmd.c >>>> +++ b/drivers/xen/privcmd.c >>>> @@ -612,8 +612,11 @@ static void unlock_pages(struct page *pages[], unsigned int nr_pages) >>>> { >>>> unsigned int i; >>>> >>>> - for (i = 0; i < nr_pages; i++) >>>> + for (i = 0; i < nr_pages; i++) { >>>> + if (!PageDirty(pages[i])) >>>> + set_page_dirty_lock(pages[i]); >>> >>> With put_page() directly following I think you should be able to use >>> set_page_dirty() instead, as there is obviously a reference to the page >>> existing. >> >> Patch [3/3] will convert above codes to use unpin_user_pages_dirty_lock() >> which internally do the same check. So I thought to keep linux-stable and >> linux-next code in sync. John had a similar concern [1] and later agreed to keep >> this check. >> >> Shall I keep this check ? No ? It doesn't matter *too* much, because patch 3/3 fixes up everything by changing it all to unpin_user_pages_dirty_lock(). However, there is something to be said for having correct interim patches, too. :) Details: >> >> [1] https://lore.kernel.org/xen-devel/a750e5e5-fd5d-663b-c5fd-261d7c939ba7@nvidia.com/ > > I wasn't referring to checking PageDirty(), but to the use of > set_page_dirty_lock(). > > Looking at the comment just before the implementation of > set_page_dirty_lock() suggests that it is fine to use set_page_dirty() > instead (so not calling lock_page()). no no, that's a misreading of the comment. Unless this xen/privcmd code has somehow taken a reference on page->mapping->host (which I do *not* think is the case), then it is still racy to call set_page_dirty() here. Instead, set_page_dirty_lock() should be used. > > Only the transition from get_user_pages_fast() to pin_user_pages_fast() > requires to use the locked version IMO. > That's a different misunderstanding. :) pin_user_pages*() APIs are meant to be functionally drop-in replacements for get_user_pages*(). Internally, pin_user_pages*() functions do some additional tracking, but from a caller's perspective, it should look the same. In other words, there is nothing about pin_user_pages_fast() that requires set_page_dirty_lock() upon release. The reason set_page_dirty_lock() was chosen is that there are very few (none at all?) call sites that need to release and dirty a page, that also meet the requirements to safely call set_page_dirty(). That's why there is a unpin_user_pages_dirty_lock(), but there is not a corresponding unpin_user_pages_dirty() call: the latter has not been required so far, even though the call site conversions are nearly done. thanks,
On 07.07.20 21:30, John Hubbard wrote: > On 2020-07-07 04:43, Jürgen Groß wrote: >> On 07.07.20 13:30, Souptick Joarder wrote: >>> On Tue, Jul 7, 2020 at 3:08 PM Jürgen Groß <jgross@suse.com> wrote: > ... >>>>> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c >>>>> index 33677ea..f6c1543 100644 >>>>> --- a/drivers/xen/privcmd.c >>>>> +++ b/drivers/xen/privcmd.c >>>>> @@ -612,8 +612,11 @@ static void unlock_pages(struct page *pages[], >>>>> unsigned int nr_pages) >>>>> { >>>>> unsigned int i; >>>>> >>>>> - for (i = 0; i < nr_pages; i++) >>>>> + for (i = 0; i < nr_pages; i++) { >>>>> + if (!PageDirty(pages[i])) >>>>> + set_page_dirty_lock(pages[i]); >>>> >>>> With put_page() directly following I think you should be able to use >>>> set_page_dirty() instead, as there is obviously a reference to the page >>>> existing. >>> >>> Patch [3/3] will convert above codes to use >>> unpin_user_pages_dirty_lock() >>> which internally do the same check. So I thought to keep linux-stable >>> and >>> linux-next code in sync. John had a similar concern [1] and later >>> agreed to keep >>> this check. >>> >>> Shall I keep this check ? No ? > > It doesn't matter *too* much, because patch 3/3 fixes up everything by > changing it all to unpin_user_pages_dirty_lock(). However, there is > something > to be said for having correct interim patches, too. :) Details: > >>> >>> [1] >>> https://lore.kernel.org/xen-devel/a750e5e5-fd5d-663b-c5fd-261d7c939ba7@nvidia.com/ >>> >> >> I wasn't referring to checking PageDirty(), but to the use of >> set_page_dirty_lock(). >> >> Looking at the comment just before the implementation of >> set_page_dirty_lock() suggests that it is fine to use set_page_dirty() >> instead (so not calling lock_page()). > > > no no, that's a misreading of the comment. Unless this xen/privcmd code has > somehow taken a reference on page->mapping->host (which I do *not* think is > the case), then it is still racy to call set_page_dirty() here. Instead, > set_page_dirty_lock() should be used. Ah, okay. Thanks for the clarification. So you can add my Reviewed-by: Juergen Gross <jgross@suse.com> Juergen
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c index 33677ea..f6c1543 100644 --- a/drivers/xen/privcmd.c +++ b/drivers/xen/privcmd.c @@ -612,8 +612,11 @@ static void unlock_pages(struct page *pages[], unsigned int nr_pages) { unsigned int i; - for (i = 0; i < nr_pages; i++) + for (i = 0; i < nr_pages; i++) { + if (!PageDirty(pages[i])) + set_page_dirty_lock(pages[i]); put_page(pages[i]); + } } static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
pages need to be marked as dirty before unpinned it in unlock_pages() which was oversight. This is fixed now. Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> Suggested-by: John Hubbard <jhubbard@nvidia.com> Cc: John Hubbard <jhubbard@nvidia.com> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> Cc: Paul Durrant <xadimgnik@gmail.com> --- drivers/xen/privcmd.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)