On Fri, 5 Jul 2024 15:55:23 -0700 Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Fri, 5 Jul 2024 22:11:14 +0000 "Kasireddy, Vivek" <vivek.kasireddy@xxxxxxxxx> 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@xxxxxxxxx>
>
> 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.
