From: "Jürgen Groß" <jgross@suse.com> To: Souptick Joarder <jrdr.linux@gmail.com>, boris.ostrovsky@oracle.com, sstabellini@kernel.org Cc: xen-devel@lists.xenproject.org, linux-kernel@vger.kernel.org, John Hubbard <jhubbard@nvidia.com>, Paul Durrant <xadimgnik@gmail.com> Subject: Re: [PATCH 1/2] xen/privcmd: Corrected error handling path and mark pages dirty Date: Fri, 26 Jun 2020 07:52:18 +0200 [thread overview] Message-ID: <9ff52733-6ce0-6bda-8e49-a6908b4ff7dc@suse.com> (raw) In-Reply-To: <1593054160-12628-1-git-send-email-jrdr.linux@gmail.com> On 25.06.20 05:02, Souptick Joarder wrote: > 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 of 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 and few condition checks can be ignored. > > As discussed, pages need to be marked as dirty before unpinned it in > unlock_pages() which was oversight. > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > Cc: John Hubbard <jhubbard@nvidia.com> > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> > Cc: Paul Durrant <xadimgnik@gmail.com> > --- > Hi, > > I'm compile tested this, but unable to run-time test, so any testing > help is much appriciated. > > drivers/xen/privcmd.c | 34 +++++++++++++++++++--------------- > 1 file changed, 19 insertions(+), 15 deletions(-) > > diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c > index a250d11..0da417c 100644 > --- a/drivers/xen/privcmd.c > +++ b/drivers/xen/privcmd.c > @@ -580,43 +580,44 @@ static long privcmd_ioctl_mmap_batch( > > static int lock_pages( > struct privcmd_dm_op_buf kbufs[], unsigned int num, > - struct page *pages[], unsigned int nr_pages) > + struct page *pages[], unsigned int nr_pages, int *pinned) unsigned int *pinned, please. > { > unsigned int i; > + int errno = 0, page_count = 0; Please drop the errno variable. It is misnamed (you never assign an errno value to it) and not needed, as you can ... > > for (i = 0; i < num; i++) { > unsigned int requested; > - int pinned; > > + *pinned += page_count; > requested = DIV_ROUND_UP( > offset_in_page(kbufs[i].uptr) + kbufs[i].size, > PAGE_SIZE); > if (requested > nr_pages) > return -ENOSPC; > > - pinned = get_user_pages_fast( > + page_count = get_user_pages_fast( > (unsigned long) kbufs[i].uptr, > requested, FOLL_WRITE, pages); > - if (pinned < 0) > - return pinned; > + if (page_count < 0) { > + errno = page_count; > + return errno; ... just return page_count her, and ... > + } > > - nr_pages -= pinned; > - pages += pinned; > + nr_pages -= page_count; > + pages += page_count; > } > > - return 0; > + return errno; ... leave this as it was. > } > > static void unlock_pages(struct page *pages[], unsigned int nr_pages) > { > unsigned int i; > > - if (!pages) > - return; > - > for (i = 0; i < nr_pages; i++) { > - if (pages[i]) > - put_page(pages[i]); > + if (!PageDirty(page)) > + set_page_dirty_lock(page); page? Not pages[i]? > + put_page(pages[i]); > } > } > > @@ -630,6 +631,7 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata) > struct xen_dm_op_buf *xbufs = NULL; > unsigned int i; > long rc; > + int pinned = 0; > > if (copy_from_user(&kdata, udata, sizeof(kdata))) > return -EFAULT; > @@ -683,9 +685,11 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata) > goto out; > } > > - rc = lock_pages(kbufs, kdata.num, pages, nr_pages); > - if (rc) > + rc = lock_pages(kbufs, kdata.num, pages, nr_pages, &pinned); > + if (rc < 0) { > + nr_pages = pinned; > goto out; > + } > > for (i = 0; i < kdata.num; i++) { > set_xen_guest_handle(xbufs[i].h, kbufs[i].uptr); > Juergen
WARNING: multiple messages have this Message-ID (diff)
From: "Jürgen Groß" <jgross@suse.com> To: Souptick Joarder <jrdr.linux@gmail.com>, boris.ostrovsky@oracle.com, sstabellini@kernel.org Cc: xen-devel@lists.xenproject.org, linux-kernel@vger.kernel.org, Paul Durrant <xadimgnik@gmail.com>, John Hubbard <jhubbard@nvidia.com> Subject: Re: [PATCH 1/2] xen/privcmd: Corrected error handling path and mark pages dirty Date: Fri, 26 Jun 2020 07:52:18 +0200 [thread overview] Message-ID: <9ff52733-6ce0-6bda-8e49-a6908b4ff7dc@suse.com> (raw) In-Reply-To: <1593054160-12628-1-git-send-email-jrdr.linux@gmail.com> On 25.06.20 05:02, Souptick Joarder wrote: > 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 of 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 and few condition checks can be ignored. > > As discussed, pages need to be marked as dirty before unpinned it in > unlock_pages() which was oversight. > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > Cc: John Hubbard <jhubbard@nvidia.com> > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> > Cc: Paul Durrant <xadimgnik@gmail.com> > --- > Hi, > > I'm compile tested this, but unable to run-time test, so any testing > help is much appriciated. > > drivers/xen/privcmd.c | 34 +++++++++++++++++++--------------- > 1 file changed, 19 insertions(+), 15 deletions(-) > > diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c > index a250d11..0da417c 100644 > --- a/drivers/xen/privcmd.c > +++ b/drivers/xen/privcmd.c > @@ -580,43 +580,44 @@ static long privcmd_ioctl_mmap_batch( > > static int lock_pages( > struct privcmd_dm_op_buf kbufs[], unsigned int num, > - struct page *pages[], unsigned int nr_pages) > + struct page *pages[], unsigned int nr_pages, int *pinned) unsigned int *pinned, please. > { > unsigned int i; > + int errno = 0, page_count = 0; Please drop the errno variable. It is misnamed (you never assign an errno value to it) and not needed, as you can ... > > for (i = 0; i < num; i++) { > unsigned int requested; > - int pinned; > > + *pinned += page_count; > requested = DIV_ROUND_UP( > offset_in_page(kbufs[i].uptr) + kbufs[i].size, > PAGE_SIZE); > if (requested > nr_pages) > return -ENOSPC; > > - pinned = get_user_pages_fast( > + page_count = get_user_pages_fast( > (unsigned long) kbufs[i].uptr, > requested, FOLL_WRITE, pages); > - if (pinned < 0) > - return pinned; > + if (page_count < 0) { > + errno = page_count; > + return errno; ... just return page_count her, and ... > + } > > - nr_pages -= pinned; > - pages += pinned; > + nr_pages -= page_count; > + pages += page_count; > } > > - return 0; > + return errno; ... leave this as it was. > } > > static void unlock_pages(struct page *pages[], unsigned int nr_pages) > { > unsigned int i; > > - if (!pages) > - return; > - > for (i = 0; i < nr_pages; i++) { > - if (pages[i]) > - put_page(pages[i]); > + if (!PageDirty(page)) > + set_page_dirty_lock(page); page? Not pages[i]? > + put_page(pages[i]); > } > } > > @@ -630,6 +631,7 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata) > struct xen_dm_op_buf *xbufs = NULL; > unsigned int i; > long rc; > + int pinned = 0; > > if (copy_from_user(&kdata, udata, sizeof(kdata))) > return -EFAULT; > @@ -683,9 +685,11 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata) > goto out; > } > > - rc = lock_pages(kbufs, kdata.num, pages, nr_pages); > - if (rc) > + rc = lock_pages(kbufs, kdata.num, pages, nr_pages, &pinned); > + if (rc < 0) { > + nr_pages = pinned; > goto out; > + } > > for (i = 0; i < kdata.num; i++) { > set_xen_guest_handle(xbufs[i].h, kbufs[i].uptr); > Juergen
next prev parent reply other threads:[~2020-06-26 5:52 UTC|newest] Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-06-25 3:02 [PATCH 1/2] xen/privcmd: Corrected error handling path and mark pages dirty Souptick Joarder 2020-06-25 3:02 ` Souptick Joarder 2020-06-25 3:02 ` [PATCH 2/2] xen/privcmd: Convert get_user_pages*() to pin_user_pages*() Souptick Joarder 2020-06-25 3:02 ` Souptick Joarder 2020-06-25 5:49 ` John Hubbard 2020-06-25 5:49 ` John Hubbard 2020-06-26 5:26 ` Souptick Joarder 2020-06-26 5:26 ` Souptick Joarder 2020-06-26 6:54 ` John Hubbard 2020-06-26 6:54 ` John Hubbard 2020-06-25 23:31 ` [PATCH 1/2] xen/privcmd: Corrected error handling path and mark pages dirty Boris Ostrovsky 2020-06-25 23:31 ` Boris Ostrovsky 2020-06-26 5:36 ` Souptick Joarder 2020-06-26 5:36 ` Souptick Joarder 2020-06-26 5:52 ` Jürgen Groß [this message] 2020-06-26 5:52 ` Jürgen Groß 2020-06-26 6:48 ` Souptick Joarder 2020-06-26 6:48 ` Souptick Joarder
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=9ff52733-6ce0-6bda-8e49-a6908b4ff7dc@suse.com \ --to=jgross@suse.com \ --cc=boris.ostrovsky@oracle.com \ --cc=jhubbard@nvidia.com \ --cc=jrdr.linux@gmail.com \ --cc=linux-kernel@vger.kernel.org \ --cc=sstabellini@kernel.org \ --cc=xadimgnik@gmail.com \ --cc=xen-devel@lists.xenproject.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.