pagemap: Add alert to mapping_set_release_always() for mapping with no release_folio

[PATCH] pagemap: Add alert to mapping_set_release_always() for mapping with no release_folio

Posted by Deepakkumar Karn 5 days, 12 hours ago

AS_RELEASE_ALWAYS tells the memory management code to always call
->release_folio() when releasing a folio, even if it has no private
data. Setting this flag without providing release_folio callback
leads to try_to_free_buffers() being called on folios without
buffer_heads, causing a NULL pointer dereference.

Add a VM_WARN_ONCE() alert to mapping_set_release_always() to catch
this programming error early and help prevent similar bugs in other
filesystems.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Deepakkumar Karn <dkarn@redhat.com>
---
 include/linux/pagemap.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 31a848485ad9..cc352e87ac2d 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -296,6 +296,9 @@ static inline bool mapping_release_always(const struct address_space *mapping)
 
 static inline void mapping_set_release_always(struct address_space *mapping)
 {
+	/* Alert while setting the flag with no release_folio callback */
+	VM_WARN_ONCE(!mapping->a_ops->release_folio,
+		     "Setting AS_RELEASE_ALWAYS with no release_folio");
 	set_bit(AS_RELEASE_ALWAYS, &mapping->flags);
 }
 
-- 
2.52.0

Re: [PATCH] pagemap: Add alert to mapping_set_release_always() for mapping with no release_folio

Posted by Matthew Wilcox 5 days, 11 hours ago

On Thu, Dec 11, 2025 at 01:31:04AM +0530, Deepakkumar Karn wrote:
>  static inline void mapping_set_release_always(struct address_space *mapping)
>  {
> +	/* Alert while setting the flag with no release_folio callback */

The comment is superfluous.

> +	VM_WARN_ONCE(!mapping->a_ops->release_folio,
> +		     "Setting AS_RELEASE_ALWAYS with no release_folio");

But you haven't said why we need to do this.  Surely the NULL pointer
splat is enough to tell you that you did something stupid?

>  	set_bit(AS_RELEASE_ALWAYS, &mapping->flags);
>  }
>  
> -- 
> 2.52.0
> 
>

Re: [PATCH] pagemap: Add alert to mapping_set_release_always() for mapping with no release_folio

Posted by Jan Kara 4 days, 23 hours ago

On Wed 10-12-25 21:36:43, Matthew Wilcox wrote:
> On Thu, Dec 11, 2025 at 01:31:04AM +0530, Deepakkumar Karn wrote:
> >  static inline void mapping_set_release_always(struct address_space *mapping)
> >  {
> > +	/* Alert while setting the flag with no release_folio callback */
> 
> The comment is superfluous.

Agreed.

> > +	VM_WARN_ONCE(!mapping->a_ops->release_folio,
> > +		     "Setting AS_RELEASE_ALWAYS with no release_folio");
> 
> But you haven't said why we need to do this.  Surely the NULL pointer
> splat is enough to tell you that you did something stupid?

Well, but this will tell it much earlier and it will directly point to the
place were you've done the mistake (instead of having to figure out why
drop_buffers() is crashing on you). So I think this assert makes sense to
ease debugging and as kind of self-reminding documentation :).

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] pagemap: Add alert to mapping_set_release_always() for mapping with no release_folio

Posted by Matthew Wilcox 3 days, 20 hours ago

On Thu, Dec 11, 2025 at 10:23:13AM +0100, Jan Kara wrote:
> On Wed 10-12-25 21:36:43, Matthew Wilcox wrote:
> > On Thu, Dec 11, 2025 at 01:31:04AM +0530, Deepakkumar Karn wrote:
> > >  static inline void mapping_set_release_always(struct address_space *mapping)
> > >  {
> > > +	/* Alert while setting the flag with no release_folio callback */
> > 
> > The comment is superfluous.
> 
> Agreed.
> 
> > > +	VM_WARN_ONCE(!mapping->a_ops->release_folio,
> > > +		     "Setting AS_RELEASE_ALWAYS with no release_folio");
> > 
> > But you haven't said why we need to do this.  Surely the NULL pointer
> > splat is enough to tell you that you did something stupid?
> 
> Well, but this will tell it much earlier and it will directly point to the
> place were you've done the mistake (instead of having to figure out why
> drop_buffers() is crashing on you). So I think this assert makes sense to
> ease debugging and as kind of self-reminding documentation :).

Oh.  So the real problem here is this:

        if (mapping && mapping->a_ops->release_folio)
                return mapping->a_ops->release_folio(folio, gfp);
        return try_to_free_buffers(folio);

We should have a block_release_folio(), change all the BH-based
filesystems to add it to their aops, and then change
filemap_release_folio() to do:

	if (mapping)
		return mapping->a_ops->release_folio(folio, gfp);
	return true;

(actually, can the !mapping case be hit?  surely this can't be called
for folios which have already been truncated?)

Then you get a nice NULL pointer dereference instead of calling into
try_to_free_buffers() which is as confusing as hell.

Re: [PATCH] pagemap: Add alert to mapping_set_release_always() for mapping with no release_folio

Posted by Jan Kara 3 days, 15 hours ago

On Fri 12-12-25 12:18:11, Matthew Wilcox wrote:
> On Thu, Dec 11, 2025 at 10:23:13AM +0100, Jan Kara wrote:
> > On Wed 10-12-25 21:36:43, Matthew Wilcox wrote:
> > > On Thu, Dec 11, 2025 at 01:31:04AM +0530, Deepakkumar Karn wrote:
> > > >  static inline void mapping_set_release_always(struct address_space *mapping)
> > > >  {
> > > > +	/* Alert while setting the flag with no release_folio callback */
> > > 
> > > The comment is superfluous.
> > 
> > Agreed.
> > 
> > > > +	VM_WARN_ONCE(!mapping->a_ops->release_folio,
> > > > +		     "Setting AS_RELEASE_ALWAYS with no release_folio");
> > > 
> > > But you haven't said why we need to do this.  Surely the NULL pointer
> > > splat is enough to tell you that you did something stupid?
> > 
> > Well, but this will tell it much earlier and it will directly point to the
> > place were you've done the mistake (instead of having to figure out why
> > drop_buffers() is crashing on you). So I think this assert makes sense to
> > ease debugging and as kind of self-reminding documentation :).
> 
> Oh.  So the real problem here is this:
> 
>         if (mapping && mapping->a_ops->release_folio)
>                 return mapping->a_ops->release_folio(folio, gfp);
>         return try_to_free_buffers(folio);
> 
> We should have a block_release_folio(), change all the BH-based
> filesystems to add it to their aops, and then change
> filemap_release_folio() to do:
> 
> 	if (mapping)
> 		return mapping->a_ops->release_folio(folio, gfp);
> 	return true;

OK, yes, this would work for me and I agree it looks like a nice cleanup.

> (actually, can the !mapping case be hit?  surely this can't be called
> for folios which have already been truncated?)

You'd think ;). There's some usage of try_to_free_buffers() from the dark
ages predating git era in jbd2 (back then jbd) which is specifically run
when we are done with journalling buffers on a page that was truncated -
see fs/jbd2/commit.c:release_buffer_page(). Also there's an interesting
case in migrate_folio_unmap() which calls try_to_free_buffers() for a
truncated page. All the other users seem to have a valid mapping.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] pagemap: Add alert to mapping_set_release_always() for mapping with no release_folio

Posted by Deepak Karn 3 days, 14 hours ago

On Fri, Dec 12, 2025 at 11:17 PM Jan Kara <jack@suse.cz> wrote:
>
> On Fri 12-12-25 12:18:11, Matthew Wilcox wrote:
> > On Thu, Dec 11, 2025 at 10:23:13AM +0100, Jan Kara wrote:
> > > On Wed 10-12-25 21:36:43, Matthew Wilcox wrote:
> > > > On Thu, Dec 11, 2025 at 01:31:04AM +0530, Deepakkumar Karn wrote:
> > > > >  static inline void mapping_set_release_always(struct address_space *mapping)
> > > > >  {
> > > > > +       /* Alert while setting the flag with no release_folio callback */
> > > >
> > > > The comment is superfluous.
> > >
> > > Agreed.
> > >
> > > > > +       VM_WARN_ONCE(!mapping->a_ops->release_folio,
> > > > > +                    "Setting AS_RELEASE_ALWAYS with no release_folio");
> > > >
> > > > But you haven't said why we need to do this.  Surely the NULL pointer
> > > > splat is enough to tell you that you did something stupid?
> > >
> > > Well, but this will tell it much earlier and it will directly point to the
> > > place were you've done the mistake (instead of having to figure out why
> > > drop_buffers() is crashing on you). So I think this assert makes sense to
> > > ease debugging and as kind of self-reminding documentation :).
> >
> > Oh.  So the real problem here is this:
> >
> >         if (mapping && mapping->a_ops->release_folio)
> >                 return mapping->a_ops->release_folio(folio, gfp);
> >         return try_to_free_buffers(folio);
> >
> > We should have a block_release_folio(), change all the BH-based
> > filesystems to add it to their aops, and then change
> > filemap_release_folio() to do:
> >
> >       if (mapping)
> >               return mapping->a_ops->release_folio(folio, gfp);
> >       return true;
>
> OK, yes, this would work for me and I agree it looks like a nice cleanup.
>
> > (actually, can the !mapping case be hit?  surely this can't be called
> > for folios which have already been truncated?)
>
> You'd think ;). There's some usage of try_to_free_buffers() from the dark
> ages predating git era in jbd2 (back then jbd) which is specifically run
> when we are done with journalling buffers on a page that was truncated -
> see fs/jbd2/commit.c:release_buffer_page(). Also there's an interesting
> case in migrate_folio_unmap() which calls try_to_free_buffers() for a
> truncated page. All the other users seem to have a valid mapping.
>

Thank you Matthew for the suggested change. Your suggestion seems to make more
sense to  cure the case. If my understanding is correct, with this
change the failure will
happens in filemap_release_folio(). The current change isn't solving
the case, as it
doesn't mandate FS to declare release.

As Jan mentioned, about the jbd2 and migrate_folio_unmap() cases
(honestly new learning
 for me, @Jan) this change should be okay. I will try and do some
tests with this. I will share
the updated patch.

Do let me know if I missed anything.

Regards,
Deepakkumar Karn

Re: [PATCH] pagemap: Add alert to mapping_set_release_always() for mapping with no release_folio

Posted by Deepak Karn 3 days, 23 hours ago

On Thu, Dec 11, 2025 at 2:53 PM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 10-12-25 21:36:43, Matthew Wilcox wrote:
> > On Thu, Dec 11, 2025 at 01:31:04AM +0530, Deepakkumar Karn wrote:
> > >  static inline void mapping_set_release_always(struct address_space *mapping)
> > >  {
> > > +   /* Alert while setting the flag with no release_folio callback */
> >
> > The comment is superfluous.
>
> Agreed.
>
> > > +   VM_WARN_ONCE(!mapping->a_ops->release_folio,
> > > +                "Setting AS_RELEASE_ALWAYS with no release_folio");
> >
> > But you haven't said why we need to do this.  Surely the NULL pointer
> > splat is enough to tell you that you did something stupid?
>
> Well, but this will tell it much earlier and it will directly point to the
> place were you've done the mistake (instead of having to figure out why
> drop_buffers() is crashing on you). So I think this assert makes sense to
> ease debugging and as kind of self-reminding documentation :).
>

Thank you Jan for your response, as you highlighted adding this change will help
with early warning, making it immediately clear during filesystem
initialization.
Also as Matthew stated, the comment is superfluous and doesn't serve
any purpose.
I will remove that comment in another version.

Regards,
Deepakkumar Karn