Re: [PATCH 02/19] buffer: allow a buffer_head to point at memory outside the page cache

Agreed, and done - but not in this patch, which is worth flagging because i=
t
is not obvious.

folio_mapping() is doing two jobs here.  The one you are objecting to is th=
e
swap cache mapping.  The other is that it returns NULL for a slab folio, an=
d
that is the only reason this path is safe for jbd2's shadow buffers today -
they are the slab-backed buffers this series is getting rid of, and they do
reach buffer_set_crypto_ctx() through __bh_submit().

So doing it in this patch would leave one commit where shadow buffers still
sit on slab folios, and this code reads folio->mapping directly, i.e., hand=
s
fscrypt a struct kmem_cache and dereferences ->host on it.  Fine at the end
of the series, broken in the middle of it.

v2 does it as its own patch 4, after the jbd2 change.

Chao

On Tue, Aug 4, 2026 at 10:07=E2=80=AFAM Matthew Wilcox <[email protected]=
> wrote:
>
> On Tue, Aug 04, 2026 at 10:07:39AM +0200, Jan Kara wrote:
> > > Yes, agreed.  I think it's fine to add in some 'if (!bh->b_folio)'
> > > tests, but only where they're needed for the occasional consumer.  Th=
is
> > > really is a rare (but legitimate) case.
> >
> > Yes. If we were to write this code from scratch, I'd rather push for
> > forming and submitting the bio directly instead of trying to bend the b=
h
> > layer for these temporary bhs. Eventually we might want to do this clea=
nup
> > anyway (since bhs don't really buy us much in this path) but not in thi=
s
> > patch set.
>
> I think that would be better, but it is out of scope.
>
> > > > >  static void buffer_set_crypto_ctx(struct bio *bio, const struct =
buffer_head *bh,
> > > > >                                   gfp_t gfp_mask)
> > > > >  {
> > > > > -       const struct address_space *mapping =3D folio_mapping(bh-=
>b_folio);
> > > > > +       const struct address_space *mapping;
> > > > >
> > > > >         /*
> > > > >          * The ext4 journal (jbd2) can submit a buffer_head it di=
rectly created
> > > > > -        * for a non-pagecache page.  fscrypt doesn't care about =
these.
> > > > > +        * for memory that is not in the page cache at all.  fscr=
ypt doesn't
> > > > > +        * care about these.
> > > > >          */
> > > > > +       if (!bh->b_folio)
> > > > > +               return;
> > > > > +       mapping =3D folio_mapping(bh->b_folio);
> > > > >         if (!mapping)
> > > > >                 return;
> > >
> > > Do we want to call folio_mapping() here?  The only case where I can s=
ee
> > > this being useful is if we attach a buffer_head to an anonymous folio=
,
> > > and I don't see a good reason to do that.  I think this can just be
> > >
> > >     mapping =3D bh->b_folio->mapping;
> >
> > For all I know yes, bh->b_folio->mapping should be enough here. But I
> > thought folio_mapping() is kind of preferred to open-coding?
>
> It rather depends on the path we're talking about.  Looking at the
> implementation of folio_mapping(), it's actually mildly dangerous
> to call folio_mapping() in this path.  If we were ever to point a
> buffer_head at some memory on a page in swap cache, we'd get a pointer
> to a swap_address_space.  I think we'd safely crash in that case because
> mapping->host would be NULL, but in other contexts doing things to a
> swap mapping would be quite bad.
>
> So I think this should just be folio->mapping; don't risk getting a
> pointer to a swap mapping.