| Message ID | 1593054160-12628-1-git-send-email-jrdr.linux@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] xen/privcmd: Corrected error handling path and mark pages dirty | expand |
On 6/24/20 11:02 PM, 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. There are two unrelated changes here (improving error path and marking pages dirty), they should be handled by separate patches. (I assume marking pages dirty is something you want to go to stable tree since otherwise there is no reason for making this change) > > 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, I don't think so. > 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 i; > + int errno = 0, page_count = 0; No need for error, really --- you can return the value immediately. > > for (i = 0; i < num; i++) { > unsigned int requested; > - int pinned; > > + *pinned += page_count; I'd move this lower, after a successful call to get_user_pages_fast() (and then you won't need to initialize it) > 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; > + } > > - nr_pages -= pinned; > - pages += pinned; > + nr_pages -= page_count; > + pages += page_count; > } > > - return 0; > + return errno; > } > > 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); > + put_page(pages[i]); > } This won't compile. -boris
On Fri, Jun 26, 2020 at 5:01 AM Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote: > > On 6/24/20 11:02 PM, 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. > > > There are two unrelated changes here (improving error path and marking > pages dirty), they should be handled by separate patches. Sure, will do it in v2. > > > (I assume marking pages dirty is something you want to go to stable tree > since otherwise there is no reason for making this change) > > > > > > 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, > > > I don't think so. I compile test it against ARM and it was fine. Against which ARCH is it failing ? > > > > 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 i; > > + int errno = 0, page_count = 0; > > > No need for error, really --- you can return the value immediately. yes, this is unnecessary. > > > > > > for (i = 0; i < num; i++) { > > unsigned int requested; > > - int pinned; > > > > + *pinned += page_count; > > > I'd move this lower, after a successful call to get_user_pages_fast() > (and then you won't need to initialize it) Ok. > > > > 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; > > + } > > > > - nr_pages -= pinned; > > - pages += pinned; > > + nr_pages -= page_count; > > + pages += page_count; > > } > > > > - return 0; > > + return errno; > > } > > > > 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); > > + put_page(pages[i]); > > } > > > This won't compile. > > > -boris > > >
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
On Fri, Jun 26, 2020 at 11:22 AM Jürgen Groß <jgross@suse.com> wrote: > > 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]? I fixed it in compile branch, but missed it in patch creation work space at the time of posting. I think, this is the compile error Boris was pointing to. Sorry about it. I will fix it and post the v2. > > > + 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
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 i; + int errno = 0, page_count = 0; 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; + } - nr_pages -= pinned; - pages += pinned; + nr_pages -= page_count; + pages += page_count; } - return 0; + return errno; } 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); + 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);
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(-)