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

Correcting myself: it is not the kmem_cache pointer that lands at that
offset.  SLAB_MATCH(compound_info, slab_cache) puts slab_cache at word 1,
while ->mapping is word 3, so what a slab folio has there is slab_list.prev.
Not an address_space either way, so the reason for splitting the patch
stands, but I named the wrong field.

Best,
Chao

On Tue, Aug 4, 2026 at 7:06 PM Chris S <[email protected]> wrote:
>
> Agreed, and done - but not in this patch, which is worth flagging because it
> is not obvious.
>
> folio_mapping() is doing two jobs here.  The one you are objecting to is the
> swap cache mapping.  The other is that it returns NULL for a slab folio, and
> 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., hands
> 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 AM 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.  This
> > > > 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 bh
> > > layer for these temporary bhs. Eventually we might want to do this cleanup
> > > anyway (since bhs don't really buy us much in this path) but not in this
> > > 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 = folio_mapping(bh->b_folio);
> > > > > > +       const struct address_space *mapping;
> > > > > >
> > > > > >         /*
> > > > > >          * The ext4 journal (jbd2) can submit a buffer_head it directly created
> > > > > > -        * for a non-pagecache page.  fscrypt doesn't care about these.
> > > > > > +        * for memory that is not in the page cache at all.  fscrypt doesn't
> > > > > > +        * care about these.
> > > > > >          */
> > > > > > +       if (!bh->b_folio)
> > > > > > +               return;
> > > > > > +       mapping = folio_mapping(bh->b_folio);
> > > > > >         if (!mapping)
> > > > > >                 return;
> > > >
> > > > Do we want to call folio_mapping() here?  The only case where I can see
> > > > 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 = 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.