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

[Andrew Morton]

On Fri, 5 Jul 2024 15:55:23 -0700 Andrew Morton <akpm at linux-foundation.org> wrote:

> On Fri, 5 Jul 2024 22:11:14 +0000 "Kasireddy, Vivek" <vivek.kasireddy at 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 at 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().

-ENOREPLY.  I'll drop the above fixup.