| Message ID | 1600007555-11650-1-git-send-email-jrdr.linux@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/gup.c: Handling ERR within unpin_user_pages() | expand |
On Sun, Sep 13, 2020 at 08:02:35PM +0530, Souptick Joarder wrote: > It is possible that a buggy caller of unpin_user_pages() > (specially in error handling path) may end up calling it with > npages < 0 which is unnecessary. > @@ -328,6 +328,9 @@ void unpin_user_pages(struct page **pages, unsigned long npages) > { > unsigned long index; > > + if (WARN_ON_ONCE(npages < 0)) > + return; But npages is unsigned long. So it can't be less than zero.
On Sun, Sep 13, 2020 at 8:25 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Sun, Sep 13, 2020 at 08:02:35PM +0530, Souptick Joarder wrote: > > It is possible that a buggy caller of unpin_user_pages() > > (specially in error handling path) may end up calling it with > > npages < 0 which is unnecessary. > > @@ -328,6 +328,9 @@ void unpin_user_pages(struct page **pages, unsigned long npages) > > { > > unsigned long index; > > > > + if (WARN_ON_ONCE(npages < 0)) > > + return; > > But npages is unsigned long. So it can't be less than zero. Sorry, I missed it. Then, it means if npages is assigned with -ERRNO by caller, unpin_user_pages() may end up calling a big loop, which is unnecessary.
On Mon, Sep 14, 2020 at 07:20:34AM +0530, Souptick Joarder wrote: > On Sun, Sep 13, 2020 at 8:25 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > On Sun, Sep 13, 2020 at 08:02:35PM +0530, Souptick Joarder wrote: > > > It is possible that a buggy caller of unpin_user_pages() > > > (specially in error handling path) may end up calling it with > > > npages < 0 which is unnecessary. > > > @@ -328,6 +328,9 @@ void unpin_user_pages(struct page **pages, unsigned long npages) > > > { > > > unsigned long index; > > > > > > + if (WARN_ON_ONCE(npages < 0)) > > > + return; > > > > But npages is unsigned long. So it can't be less than zero. > > Sorry, I missed it. > > Then, it means if npages is assigned with -ERRNO by caller, unpin_user_pages() > may end up calling a big loop, which is unnecessary. How will a caller allocate memory of the right size and still manage to call with the wrong npages? Do you have an example of a broken caller? Jason
On Mon, Sep 14, 2020 at 7:38 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > On Mon, Sep 14, 2020 at 07:20:34AM +0530, Souptick Joarder wrote: > > On Sun, Sep 13, 2020 at 8:25 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > On Sun, Sep 13, 2020 at 08:02:35PM +0530, Souptick Joarder wrote: > > > > It is possible that a buggy caller of unpin_user_pages() > > > > (specially in error handling path) may end up calling it with > > > > npages < 0 which is unnecessary. > > > > @@ -328,6 +328,9 @@ void unpin_user_pages(struct page **pages, unsigned long npages) > > > > { > > > > unsigned long index; > > > > > > > > + if (WARN_ON_ONCE(npages < 0)) > > > > + return; > > > > > > But npages is unsigned long. So it can't be less than zero. > > > > Sorry, I missed it. > > > > Then, it means if npages is assigned with -ERRNO by caller, unpin_user_pages() > > may end up calling a big loop, which is unnecessary. > > How will a caller allocate memory of the right size and still manage > to call with the wrong npages? Do you have an example of a broken caller? These are two broken callers which might end up calling unpin_user_pages() with -ERRNO. drivers/rapidio/devices/rio_mport_cdev.c#L952 drivers/misc/mic/scif/scif_rma.c#L1399 They both are in error handling paths, so might not have any serious impact. But theoretically possible.
On 9/14/20 1:52 PM, Souptick Joarder wrote: > On Mon, Sep 14, 2020 at 7:38 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: >> >> On Mon, Sep 14, 2020 at 07:20:34AM +0530, Souptick Joarder wrote: >>> On Sun, Sep 13, 2020 at 8:25 PM Matthew Wilcox <willy@infradead.org> wrote: >>>> >>>> On Sun, Sep 13, 2020 at 08:02:35PM +0530, Souptick Joarder wrote: >>>>> It is possible that a buggy caller of unpin_user_pages() >>>>> (specially in error handling path) may end up calling it with >>>>> npages < 0 which is unnecessary. >>>>> @@ -328,6 +328,9 @@ void unpin_user_pages(struct page **pages, unsigned long npages) >>>>> { >>>>> unsigned long index; >>>>> >>>>> + if (WARN_ON_ONCE(npages < 0)) >>>>> + return; >>>> >>>> But npages is unsigned long. So it can't be less than zero. >>> >>> Sorry, I missed it. >>> >>> Then, it means if npages is assigned with -ERRNO by caller, unpin_user_pages() >>> may end up calling a big loop, which is unnecessary. >> >> How will a caller allocate memory of the right size and still manage >> to call with the wrong npages? Do you have an example of a broken caller? > > These are two broken callers which might end up calling unpin_user_pages() > with -ERRNO. > drivers/rapidio/devices/rio_mport_cdev.c#L952 > drivers/misc/mic/scif/scif_rma.c#L1399 > > They both are in error handling paths, so might not have any serious impact. > But theoretically possible. > Eventually, I settled on fixing up the callers so that they match the gup/pup API better. In other words, gup/pup has signed int for both input and return value, and the callers need to handle that perfectly. So let's fix up the callers. thanks,
On Tue, Sep 15, 2020 at 02:22:33AM +0530, Souptick Joarder wrote: > On Mon, Sep 14, 2020 at 7:38 PM Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > On Mon, Sep 14, 2020 at 07:20:34AM +0530, Souptick Joarder wrote: > > > On Sun, Sep 13, 2020 at 8:25 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > > > On Sun, Sep 13, 2020 at 08:02:35PM +0530, Souptick Joarder wrote: > > > > > It is possible that a buggy caller of unpin_user_pages() > > > > > (specially in error handling path) may end up calling it with > > > > > npages < 0 which is unnecessary. > > > > > @@ -328,6 +328,9 @@ void unpin_user_pages(struct page **pages, unsigned long npages) > > > > > { > > > > > unsigned long index; > > > > > > > > > > + if (WARN_ON_ONCE(npages < 0)) > > > > > + return; > > > > > > > > But npages is unsigned long. So it can't be less than zero. > > > > > > Sorry, I missed it. > > > > > > Then, it means if npages is assigned with -ERRNO by caller, unpin_user_pages() > > > may end up calling a big loop, which is unnecessary. > > > > How will a caller allocate memory of the right size and still manage > > to call with the wrong npages? Do you have an example of a broken caller? > > These are two broken callers which might end up calling unpin_user_pages() > with -ERRNO. > drivers/rapidio/devices/rio_mport_cdev.c#L952 The error here is that nr_pages should not be set to pinned if pinned is < 0. Why not fix the logic there? Because it is inherently dangerous to set an unsigned from a signed variable like that. > drivers/misc/mic/scif/scif_rma.c#L1399 Again this is a caller who is not properly checking error conditions. > > They both are in error handling paths, so might not have any serious impact. > But theoretically possible. Actually I think they might have serious problems so they both should be fixed. In the end this patch just can't work because npages can't be < 0 like Matthew said. Ira
diff --git a/mm/gup.c b/mm/gup.c index 0b5c308b..2e19bd6 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -328,6 +328,9 @@ void unpin_user_pages(struct page **pages, unsigned long npages) { unsigned long index; + if (WARN_ON_ONCE(npages < 0)) + return; + /* * TODO: this can be optimized for huge pages: if a series of pages is * physically contiguous and part of the same compound page, then a
It is possible that a buggy caller of unpin_user_pages() (specially in error handling path) may end up calling it with npages < 0 which is unnecessary. This can be fixed by adding extra check inside unpin_user_pages(). Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> Cc: John Hubbard <jhubbard@nvidia.com> --- mm/gup.c | 3 +++ 1 file changed, 3 insertions(+)