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().
