Re: [PATCH v16 3/9] mm/gup: Introduce memfd_pin_folios() for pinning memfd folios

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 5 Jul 2024 22:11:14 +0000 "Kasireddy, Vivek" <vivek.kasireddy@intel.com> wrote:

> Hi Andrew and SJ, 
> 
> > 
> > >
> > > I didn't look deep into the patch, so unsure if that's a valid fix, though.
> > > May I ask your thoughts?
> > 
> > Perhaps we should propagate the errno which was returned by
> > try_grab_folio()?
> > 
> > I'll do it this way.  Vivek, please check and let us know?
> Yeah, memfd_pin_folios() doesn't need the fast version, so replacing with
> the slow version (try_grab_folio) should be fine. And, as you suggest,
> propagating the errno returned by try_grab_folio() would be the right thing
> to do instead of explicitly setting errno to -EINVAL. Either way, this change is
> Acked-by: Vivek Kasireddy <vivek.kasireddy@intel.com>

Cool, thanks.

We could do this to propagate the try_grab_folio() return value:

--- a/mm/gup.c~mm-gup-introduce-memfd_pin_folios-for-pinning-memfd-folios-fix-fix
+++ a/mm/gup.c
@@ -3848,6 +3848,8 @@ long memfd_pin_folios(struct file *memfd
 
 			next_idx = 0;
 			for (i = 0; i < nr_found; i++) {
+				int ret2;
+
 				/*
 				 * As there can be multiple entries for a
 				 * given folio in the batch returned by
@@ -3860,10 +3862,10 @@ long memfd_pin_folios(struct file *memfd
 					continue;
 
 				folio = page_folio(&fbatch.folios[i]->page);
-
-				if (try_grab_folio(folio, 1, FOLL_PIN)) {
+				ret2 = try_grab_folio(folio, 1, FOLL_PIN);
+				if (ret2) {
 					folio_batch_release(&fbatch);
-					ret = -EINVAL;
+					ret = ret2;
 					goto err;
 				}
 

But try_grab_folio can return that weird -EREMOTEIO.  The
try_grab_folio() kerneldoc doesn't even mention that - it incorrectly
claims that only -ENOMEM can be returned. (needs fixing!).

And if memfd_pin_folios() returns -EREMOTEIO then I expect
udmabuf_ioctl() will return -EREMOTEIO to userspace.  And userspace
will wonder "what the hell is that".  If there's a udmabuf_ioctl
manpage then will that explain this errno?  And similar concerns for
future callers of memfd_pin_folios().