On Thu, Oct 17, 2024 at 9:35 AM Shakeel Butt <shakeel.butt@xxxxxxxxx> wrote: > > On Thu, Oct 17, 2024 at 11:18:34AM GMT, David Hildenbrand wrote: > > On 17.10.24 00:16, Andrii Nakryiko wrote: > > > From memfd_secret(2) manpage: > > > > > > The memory areas backing the file created with memfd_secret(2) are > > > visible only to the processes that have access to the file descriptor. > > > The memory region is removed from the kernel page tables and only the > > > page tables of the processes holding the file descriptor map the > > > corresponding physical memory. (Thus, the pages in the region can't be > > > accessed by the kernel itself, so that, for example, pointers to the > > > region can't be passed to system calls.) > > > > > > So folios backed by such secretmem files are not mapped into kernel > > > address space and shouldn't be accessed, in general. > > > > > > To make this a bit more generic of a fix and prevent regression in the > > > future for similar special mappings, do a generic check of whether the > > > folio we got is mapped with kernel_page_present(), as suggested in [1]. > > > This will handle secretmem, and any future special cases that use > > > a similar approach. > > > > > > Original report and repro can be found in [0]. > > > > > > [0] https://lore.kernel.org/bpf/ZwyG8Uro%2FSyTXAni@ly-workstation/ > > > [1] https://lore.kernel.org/bpf/CAJD7tkbpEMx-eC4A-z8Jm1ikrY_KJVjWO+mhhz1_fni4x+COKw@xxxxxxxxxxxxxx/ > > > > > > Reported-by: Yi Lai <yi1.lai@xxxxxxxxx> > > > Suggested-by: Yosry Ahmed <yosryahmed@xxxxxxxxxx> > > > Fixes: de3ec364c3c3 ("lib/buildid: add single folio-based file reader abstraction") > > > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx> > > > --- > > > lib/buildid.c | 5 ++++- > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/lib/buildid.c b/lib/buildid.c > > > index 290641d92ac1..90df64fd64c1 100644 > > > --- a/lib/buildid.c > > > +++ b/lib/buildid.c > > > @@ -5,6 +5,7 @@ > > > #include <linux/elf.h> > > > #include <linux/kernel.h> > > > #include <linux/pagemap.h> > > > +#include <linux/set_memory.h> > > > #define BUILD_ID 3 > > > @@ -74,7 +75,9 @@ static int freader_get_folio(struct freader *r, loff_t file_off) > > > filemap_invalidate_unlock_shared(r->file->f_mapping); > > > } > > > - if (IS_ERR(r->folio) || !folio_test_uptodate(r->folio)) { > > > + if (IS_ERR(r->folio) || > > > + !kernel_page_present(&r->folio->page) || > > > + !folio_test_uptodate(r->folio)) { > > > if (!IS_ERR(r->folio)) > > > folio_put(r->folio); > > > r->folio = NULL; > > > > As replied elsewhere, can't we take a look at the mapping? > > > > We do the same thing in gup_fast_folio_allowed() where we check > > secretmem_mapping(). > > Responded on the v1 but I think we can go with v1 of this work as > whoever will be working on unmapping folios from direct map will need to > fix gup_fast_folio_allowed(), they can fix this code as well. Also it > seems like some arch don't have kernel_page_present() and builds are > failing. > Yeah, we are lucky that BPF CI tested s390x and caught this issue. > Andrii, let's move forward with the v1 patch. Let me post v3 based on v1 (checking for secretmem_mapping()), but I'll change return code to -EFAULT, so in the future this can be rolled into generic error handling code path with no change in error code. > > > > > > > -- > > Cheers, > > > > David / dhildenb > >
