The Linux Kernel Mailing List
 help / color / mirror / Atom feed
* [PATCH v2] fs: add NULL check in drop_buffers() to prevent null-ptr-deref
@ 2025-12-08 19:03 Deepakkumar Karn
  2025-12-08 19:30 ` Darrick J. Wong
  0 siblings, 1 reply; 11+ messages in thread
From: Deepakkumar Karn @ 2025-12-08 19:03 UTC (permalink / raw)
  To: Alexander Viro
  Cc: Christian Brauner, Jan Kara, linux-fsdevel, linux-kernel,
	syzbot+e07658f51ca22ab65b4e, syzkaller-bugs, Deepakkumar Karn

drop_buffers() dereferences the buffer_head pointer returned by
folio_buffers() without checking for NULL. This leads to a null pointer
dereference when called from try_to_free_buffers() on a folio with no
buffers attached. This happens when filemap_release_folio() is called on
a folio belonging to a mapping with AS_RELEASE_ALWAYS set but without
release_folio address_space operation defined. In such case,
folio_needs_release() returns true because of AS_RELEASE_ALWAYS flag,
the folio has no private buffer data, causing the try_to_free_buffers()
with a folio that has no buffers.

Adding NULL check for the buffer_head pointer and return false early if
no buffers are attached to the folio.

Reported-by: syzbot+e07658f51ca22ab65b4e@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=e07658f51ca22ab65b4e
Fixes: 6439476311a6 ("fs: Convert drop_buffers() to use a folio")
Signed-off-by: Deepakkumar Karn <dkarn@redhat.com>
---
 fs/buffer.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/buffer.c b/fs/buffer.c
index 838c0c571022..fa5de0cdf540 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -2893,6 +2893,10 @@ drop_buffers(struct folio *folio, struct buffer_head **buffers_to_free)
 	struct buffer_head *head = folio_buffers(folio);
 	struct buffer_head *bh;
 
+	/* In cases of folio without buffer_head*/
+	if (!head)
+		return false;
+
 	bh = head;
 	do {
 		if (buffer_busy(bh))
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] fs: add NULL check in drop_buffers() to prevent null-ptr-deref
  2025-12-08 19:03 [PATCH v2] fs: add NULL check in drop_buffers() to prevent null-ptr-deref Deepakkumar Karn
@ 2025-12-08 19:30 ` Darrick J. Wong
  2025-12-08 20:13   ` Deepakkumar Karn
  0 siblings, 1 reply; 11+ messages in thread
From: Darrick J. Wong @ 2025-12-08 19:30 UTC (permalink / raw)
  To: Deepakkumar Karn
  Cc: Alexander Viro, Christian Brauner, Jan Kara, linux-fsdevel,
	linux-kernel, syzbot+e07658f51ca22ab65b4e, syzkaller-bugs

On Tue, Dec 09, 2025 at 12:33:07AM +0530, Deepakkumar Karn wrote:
> drop_buffers() dereferences the buffer_head pointer returned by
> folio_buffers() without checking for NULL. This leads to a null pointer
> dereference when called from try_to_free_buffers() on a folio with no
> buffers attached. This happens when filemap_release_folio() is called on
> a folio belonging to a mapping with AS_RELEASE_ALWAYS set but without
> release_folio address_space operation defined. In such case,

What user is that?  All the users of AS_RELEASE_ALWAYS in 6.18 appear to
supply a ->release_folio.  Is this some new thing in 6.19?

--D

> folio_needs_release() returns true because of AS_RELEASE_ALWAYS flag,
> the folio has no private buffer data, causing the try_to_free_buffers()
> with a folio that has no buffers.
> 
> Adding NULL check for the buffer_head pointer and return false early if
> no buffers are attached to the folio.
> 
> Reported-by: syzbot+e07658f51ca22ab65b4e@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=e07658f51ca22ab65b4e
> Fixes: 6439476311a6 ("fs: Convert drop_buffers() to use a folio")
> Signed-off-by: Deepakkumar Karn <dkarn@redhat.com>
> ---
>  fs/buffer.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 838c0c571022..fa5de0cdf540 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -2893,6 +2893,10 @@ drop_buffers(struct folio *folio, struct buffer_head **buffers_to_free)
>  	struct buffer_head *head = folio_buffers(folio);
>  	struct buffer_head *bh;
>  
> +	/* In cases of folio without buffer_head*/
> +	if (!head)
> +		return false;
> +
>  	bh = head;
>  	do {
>  		if (buffer_busy(bh))
> -- 
> 2.52.0
> 
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] fs: add NULL check in drop_buffers() to prevent null-ptr-deref
  2025-12-08 19:30 ` Darrick J. Wong
@ 2025-12-08 20:13   ` Deepakkumar Karn
  2025-12-09 11:18     ` Jan Kara
  0 siblings, 1 reply; 11+ messages in thread
From: Deepakkumar Karn @ 2025-12-08 20:13 UTC (permalink / raw)
  To: djwong
  Cc: brauner, dkarn, jack, linux-fsdevel, linux-kernel,
	syzbot+e07658f51ca22ab65b4e, syzkaller-bugs, viro

On Mon, 8 Dec 2025 11:30:24 -0800, Darrick J. Wong wrote:
> > drop_buffers() dereferences the buffer_head pointer returned by
> > folio_buffers() without checking for NULL. This leads to a null pointer
> > dereference when called from try_to_free_buffers() on a folio with no
> > buffers attached. This happens when filemap_release_folio() is called on
> > a folio belonging to a mapping with AS_RELEASE_ALWAYS set but without
> > release_folio address_space operation defined. In such case,

> What user is that?  All the users of AS_RELEASE_ALWAYS in 6.18 appear to
> supply a ->release_folio.  Is this some new thing in 6.19?

AFS directories SET AS_RELEASE_ALWAYS but have not .release_folio.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] fs: add NULL check in drop_buffers() to prevent null-ptr-deref
  2025-12-08 20:13   ` Deepakkumar Karn
@ 2025-12-09 11:18     ` Jan Kara
  2025-12-09 16:30       ` Deepak Karn
  0 siblings, 1 reply; 11+ messages in thread
From: Jan Kara @ 2025-12-09 11:18 UTC (permalink / raw)
  To: Deepakkumar Karn
  Cc: djwong, brauner, jack, linux-fsdevel, linux-kernel,
	syzbot+e07658f51ca22ab65b4e, syzkaller-bugs, viro, David Howells,
	linux-afs

On Tue 09-12-25 01:43:33, Deepakkumar Karn wrote:
> On Mon, 8 Dec 2025 11:30:24 -0800, Darrick J. Wong wrote:
> > > drop_buffers() dereferences the buffer_head pointer returned by
> > > folio_buffers() without checking for NULL. This leads to a null pointer
> > > dereference when called from try_to_free_buffers() on a folio with no
> > > buffers attached. This happens when filemap_release_folio() is called on
> > > a folio belonging to a mapping with AS_RELEASE_ALWAYS set but without
> > > release_folio address_space operation defined. In such case,
> 
> > What user is that?  All the users of AS_RELEASE_ALWAYS in 6.18 appear to
> > supply a ->release_folio.  Is this some new thing in 6.19?
> 
> AFS directories SET AS_RELEASE_ALWAYS but have not .release_folio.

AFAICS AFS sets AS_RELEASE_ALWAYS only for symlinks but not for
directories? Anyway I agree AFS symlinks will have AS_RELEASE_ALWAYS but no
.release_folio callback. And this looks like a bug in AFS because AFAICT
there's no point in setting AS_RELEASE_ALWAYS when you don't have
.release_folio callback. Added relevant people to CC.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] fs: add NULL check in drop_buffers() to prevent null-ptr-deref
  2025-12-09 11:18     ` Jan Kara
@ 2025-12-09 16:30       ` Deepak Karn
  2025-12-10  9:55         ` Jan Kara
  0 siblings, 1 reply; 11+ messages in thread
From: Deepak Karn @ 2025-12-09 16:30 UTC (permalink / raw)
  To: Jan Kara
  Cc: djwong, brauner, linux-fsdevel, linux-kernel,
	syzbot+e07658f51ca22ab65b4e, syzkaller-bugs, viro, David Howells,
	linux-afs

On Tue, Dec 9, 2025 at 4:48 PM Jan Kara <jack@suse.cz> wrote:
>
> On Tue 09-12-25 01:43:33, Deepakkumar Karn wrote:
> > On Mon, 8 Dec 2025 11:30:24 -0800, Darrick J. Wong wrote:
> > > > drop_buffers() dereferences the buffer_head pointer returned by
> > > > folio_buffers() without checking for NULL. This leads to a null pointer
> > > > dereference when called from try_to_free_buffers() on a folio with no
> > > > buffers attached. This happens when filemap_release_folio() is called on
> > > > a folio belonging to a mapping with AS_RELEASE_ALWAYS set but without
> > > > release_folio address_space operation defined. In such case,
> >
> > > What user is that?  All the users of AS_RELEASE_ALWAYS in 6.18 appear to
> > > supply a ->release_folio.  Is this some new thing in 6.19?
> >
> > AFS directories SET AS_RELEASE_ALWAYS but have not .release_folio.
>
> AFAICS AFS sets AS_RELEASE_ALWAYS only for symlinks but not for
> directories? Anyway I agree AFS symlinks will have AS_RELEASE_ALWAYS but no
> .release_folio callback. And this looks like a bug in AFS because AFAICT
> there's no point in setting AS_RELEASE_ALWAYS when you don't have
> .release_folio callback. Added relevant people to CC.
>
>                                                                 Honza

Thank you for your response Jan. As you suggested, the bug is in AFS.
Can we include this current defensive check in drop_buffers() and I can submit
another patch to handle that bug of AFS we discussed?

Regards,
Deepakkumar Karn


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] fs: add NULL check in drop_buffers() to prevent null-ptr-deref
  2025-12-09 16:30       ` Deepak Karn
@ 2025-12-10  9:55         ` Jan Kara
  2025-12-10 15:29           ` Deepak Karn
  0 siblings, 1 reply; 11+ messages in thread
From: Jan Kara @ 2025-12-10  9:55 UTC (permalink / raw)
  To: Deepak Karn
  Cc: Jan Kara, djwong, brauner, linux-fsdevel, linux-kernel,
	syzbot+e07658f51ca22ab65b4e, syzkaller-bugs, viro, David Howells,
	linux-afs

On Tue 09-12-25 22:00:04, Deepak Karn wrote:
> On Tue, Dec 9, 2025 at 4:48 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Tue 09-12-25 01:43:33, Deepakkumar Karn wrote:
> > > On Mon, 8 Dec 2025 11:30:24 -0800, Darrick J. Wong wrote:
> > > > > drop_buffers() dereferences the buffer_head pointer returned by
> > > > > folio_buffers() without checking for NULL. This leads to a null pointer
> > > > > dereference when called from try_to_free_buffers() on a folio with no
> > > > > buffers attached. This happens when filemap_release_folio() is called on
> > > > > a folio belonging to a mapping with AS_RELEASE_ALWAYS set but without
> > > > > release_folio address_space operation defined. In such case,
> > >
> > > > What user is that?  All the users of AS_RELEASE_ALWAYS in 6.18 appear to
> > > > supply a ->release_folio.  Is this some new thing in 6.19?
> > >
> > > AFS directories SET AS_RELEASE_ALWAYS but have not .release_folio.
> >
> > AFAICS AFS sets AS_RELEASE_ALWAYS only for symlinks but not for
> > directories? Anyway I agree AFS symlinks will have AS_RELEASE_ALWAYS but no
> > .release_folio callback. And this looks like a bug in AFS because AFAICT
> > there's no point in setting AS_RELEASE_ALWAYS when you don't have
> > .release_folio callback. Added relevant people to CC.
> >
> >                                                                 Honza
> 
> Thank you for your response Jan. As you suggested, the bug is in AFS.
> Can we include this current defensive check in drop_buffers() and I can submit
> another patch to handle that bug of AFS we discussed?

I'm not strongly opposed to that (although try_to_free_buffers() would seem
like a tad bit better place) but overall I don't think it's a great idea as
it would hide bugs. But perhaps with WARN_ON_ONCE() (to catch sloppy
programming) it would be a sensible hardening.

Also I think mapping_set_release_always() should assert that
mapping->a_ops->release_folio is non-NULL to catch the problem early (once
you fix the AFS bug).

								Honza

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] fs: add NULL check in drop_buffers() to prevent null-ptr-deref
  2025-12-10  9:55         ` Jan Kara
@ 2025-12-10 15:29           ` Deepak Karn
  2025-12-10 17:23             ` Jan Kara
  0 siblings, 1 reply; 11+ messages in thread
From: Deepak Karn @ 2025-12-10 15:29 UTC (permalink / raw)
  To: Jan Kara
  Cc: djwong, brauner, linux-fsdevel, linux-kernel,
	syzbot+e07658f51ca22ab65b4e, syzkaller-bugs, viro, David Howells,
	linux-afs

On Wed, Dec 10, 2025 at 3:25 PM Jan Kara <jack@suse.cz> wrote:
>
> On Tue 09-12-25 22:00:04, Deepak Karn wrote:
> > On Tue, Dec 9, 2025 at 4:48 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Tue 09-12-25 01:43:33, Deepakkumar Karn wrote:
> > > > On Mon, 8 Dec 2025 11:30:24 -0800, Darrick J. Wong wrote:
> > > > > > drop_buffers() dereferences the buffer_head pointer returned by
> > > > > > folio_buffers() without checking for NULL. This leads to a null pointer
> > > > > > dereference when called from try_to_free_buffers() on a folio with no
> > > > > > buffers attached. This happens when filemap_release_folio() is called on
> > > > > > a folio belonging to a mapping with AS_RELEASE_ALWAYS set but without
> > > > > > release_folio address_space operation defined. In such case,
> > > >
> > > > > What user is that?  All the users of AS_RELEASE_ALWAYS in 6.18 appear to
> > > > > supply a ->release_folio.  Is this some new thing in 6.19?
> > > >
> > > > AFS directories SET AS_RELEASE_ALWAYS but have not .release_folio.
> > >
> > > AFAICS AFS sets AS_RELEASE_ALWAYS only for symlinks but not for
> > > directories? Anyway I agree AFS symlinks will have AS_RELEASE_ALWAYS but no
> > > .release_folio callback. And this looks like a bug in AFS because AFAICT
> > > there's no point in setting AS_RELEASE_ALWAYS when you don't have
> > > .release_folio callback. Added relevant people to CC.
> > >
> > >                                                                 Honza
> >
> > Thank you for your response Jan. As you suggested, the bug is in AFS.
> > Can we include this current defensive check in drop_buffers() and I can submit
> > another patch to handle that bug of AFS we discussed?
>
> I'm not strongly opposed to that (although try_to_free_buffers() would seem
> like a tad bit better place) but overall I don't think it's a great idea as
> it would hide bugs. But perhaps with WARN_ON_ONCE() (to catch sloppy
> programming) it would be a sensible hardening.
>

Thanks Jan for your response. As suggested, adding WARN_ON_ONCE()
will be more sensible.
I just wanted to clarify my understanding, you are suggesting adding
WARN_ON_ONCE()
in try_to_free_buffers() as this highlights the issue and also solves
the concern. If my
understanding is wrong please let me know, I will share the updated patch.

> Also I think mapping_set_release_always() should assert that
> mapping->a_ops->release_folio is non-NULL to catch the problem early (once
> you fix the AFS bug).
>

I will address this in another patch for AFS.

Regards,
Deepakkumar Karn


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] fs: add NULL check in drop_buffers() to prevent null-ptr-deref
  2025-12-10 15:29           ` Deepak Karn
@ 2025-12-10 17:23             ` Jan Kara
  2025-12-10 17:50               ` Deepak Karn
  0 siblings, 1 reply; 11+ messages in thread
From: Jan Kara @ 2025-12-10 17:23 UTC (permalink / raw)
  To: Deepak Karn
  Cc: Jan Kara, djwong, brauner, linux-fsdevel, linux-kernel,
	syzbot+e07658f51ca22ab65b4e, syzkaller-bugs, viro, David Howells,
	linux-afs

On Wed 10-12-25 20:59:00, Deepak Karn wrote:
> On Wed, Dec 10, 2025 at 3:25 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Tue 09-12-25 22:00:04, Deepak Karn wrote:
> > > On Tue, Dec 9, 2025 at 4:48 PM Jan Kara <jack@suse.cz> wrote:
> > > >
> > > > On Tue 09-12-25 01:43:33, Deepakkumar Karn wrote:
> > > > > On Mon, 8 Dec 2025 11:30:24 -0800, Darrick J. Wong wrote:
> > > > > > > drop_buffers() dereferences the buffer_head pointer returned by
> > > > > > > folio_buffers() without checking for NULL. This leads to a null pointer
> > > > > > > dereference when called from try_to_free_buffers() on a folio with no
> > > > > > > buffers attached. This happens when filemap_release_folio() is called on
> > > > > > > a folio belonging to a mapping with AS_RELEASE_ALWAYS set but without
> > > > > > > release_folio address_space operation defined. In such case,
> > > > >
> > > > > > What user is that?  All the users of AS_RELEASE_ALWAYS in 6.18 appear to
> > > > > > supply a ->release_folio.  Is this some new thing in 6.19?
> > > > >
> > > > > AFS directories SET AS_RELEASE_ALWAYS but have not .release_folio.
> > > >
> > > > AFAICS AFS sets AS_RELEASE_ALWAYS only for symlinks but not for
> > > > directories? Anyway I agree AFS symlinks will have AS_RELEASE_ALWAYS but no
> > > > .release_folio callback. And this looks like a bug in AFS because AFAICT
> > > > there's no point in setting AS_RELEASE_ALWAYS when you don't have
> > > > .release_folio callback. Added relevant people to CC.
> > > >
> > > >                                                                 Honza
> > >
> > > Thank you for your response Jan. As you suggested, the bug is in AFS.
> > > Can we include this current defensive check in drop_buffers() and I can submit
> > > another patch to handle that bug of AFS we discussed?
> >
> > I'm not strongly opposed to that (although try_to_free_buffers() would seem
> > like a tad bit better place) but overall I don't think it's a great idea as
> > it would hide bugs. But perhaps with WARN_ON_ONCE() (to catch sloppy
> > programming) it would be a sensible hardening.
> >
> 
> Thanks Jan for your response. As suggested, adding WARN_ON_ONCE() will be
> more sensible.
> I just wanted to clarify my understanding, you are suggesting adding
> WARN_ON_ONCE() in try_to_free_buffers() as this highlights the issue and
> also solves the concern. If my understanding is wrong please let me know,
> I will share the updated patch.

Yes, I meant something like:

	if (WARN_ON_ONCE(!folio_buffers(folio)))
		return true;

in try_to_free_buffers(). 

> > Also I think mapping_set_release_always() should assert that
> > mapping->a_ops->release_folio is non-NULL to catch the problem early (once
> > you fix the AFS bug).
> >
> 
> I will address this in another patch for AFS.

Thanks. As a separate patch please :)

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] fs: add NULL check in drop_buffers() to prevent null-ptr-deref
  2025-12-10 17:23             ` Jan Kara
@ 2025-12-10 17:50               ` Deepak Karn
  2025-12-11  9:15                 ` Jan Kara
  0 siblings, 1 reply; 11+ messages in thread
From: Deepak Karn @ 2025-12-10 17:50 UTC (permalink / raw)
  To: Jan Kara
  Cc: djwong, brauner, linux-fsdevel, linux-kernel,
	syzbot+e07658f51ca22ab65b4e, syzkaller-bugs, viro, David Howells,
	linux-afs

On Wed, Dec 10, 2025 at 10:54 PM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 10-12-25 20:59:00, Deepak Karn wrote:
> > On Wed, Dec 10, 2025 at 3:25 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Tue 09-12-25 22:00:04, Deepak Karn wrote:
> > > > On Tue, Dec 9, 2025 at 4:48 PM Jan Kara <jack@suse.cz> wrote:
> > > > >
> > > > > On Tue 09-12-25 01:43:33, Deepakkumar Karn wrote:
> > > > > > On Mon, 8 Dec 2025 11:30:24 -0800, Darrick J. Wong wrote:
> > > > > > > > drop_buffers() dereferences the buffer_head pointer returned by
> > > > > > > > folio_buffers() without checking for NULL. This leads to a null pointer
> > > > > > > > dereference when called from try_to_free_buffers() on a folio with no
> > > > > > > > buffers attached. This happens when filemap_release_folio() is called on
> > > > > > > > a folio belonging to a mapping with AS_RELEASE_ALWAYS set but without
> > > > > > > > release_folio address_space operation defined. In such case,
> > > > > >
> > > > > > > What user is that?  All the users of AS_RELEASE_ALWAYS in 6.18 appear to
> > > > > > > supply a ->release_folio.  Is this some new thing in 6.19?
> > > > > >
> > > > > > AFS directories SET AS_RELEASE_ALWAYS but have not .release_folio.
> > > > >
> > > > > AFAICS AFS sets AS_RELEASE_ALWAYS only for symlinks but not for
> > > > > directories? Anyway I agree AFS symlinks will have AS_RELEASE_ALWAYS but no
> > > > > .release_folio callback. And this looks like a bug in AFS because AFAICT
> > > > > there's no point in setting AS_RELEASE_ALWAYS when you don't have
> > > > > .release_folio callback. Added relevant people to CC.
> > > > >
> > > > >                                                                 Honza
> > > >
> > > > Thank you for your response Jan. As you suggested, the bug is in AFS.
> > > > Can we include this current defensive check in drop_buffers() and I can submit
> > > > another patch to handle that bug of AFS we discussed?
> > >
> > > I'm not strongly opposed to that (although try_to_free_buffers() would seem
> > > like a tad bit better place) but overall I don't think it's a great idea as
> > > it would hide bugs. But perhaps with WARN_ON_ONCE() (to catch sloppy
> > > programming) it would be a sensible hardening.
> > >
> >
> > Thanks Jan for your response. As suggested, adding WARN_ON_ONCE() will be
> > more sensible.
> > I just wanted to clarify my understanding, you are suggesting adding
> > WARN_ON_ONCE() in try_to_free_buffers() as this highlights the issue and
> > also solves the concern. If my understanding is wrong please let me know,
> > I will share the updated patch.
>
> Yes, I meant something like:
>
>         if (WARN_ON_ONCE(!folio_buffers(folio)))
>                 return true;
>
> in try_to_free_buffers().
>

Yes this is what I am also going for, just a minor change that I was
going to return false.

Reason:
From the flow it should return false whenever failed to free buffers /
operation
couldn't complete. Do you think otherwise.

> > > Also I think mapping_set_release_always() should assert that
> > > mapping->a_ops->release_folio is non-NULL to catch the problem early (once
> > > you fix the AFS bug).
> > >
> >
> > I will address this in another patch for AFS.
>
> Thanks. As a separate patch please :)
>
>                                                                 Honza

Yes.

Regards,
Deepak Karn


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] fs: add NULL check in drop_buffers() to prevent null-ptr-deref
  2025-12-10 17:50               ` Deepak Karn
@ 2025-12-11  9:15                 ` Jan Kara
  2025-12-11 12:50                   ` Deepak Karn
  0 siblings, 1 reply; 11+ messages in thread
From: Jan Kara @ 2025-12-11  9:15 UTC (permalink / raw)
  To: Deepak Karn
  Cc: Jan Kara, djwong, brauner, linux-fsdevel, linux-kernel,
	syzbot+e07658f51ca22ab65b4e, syzkaller-bugs, viro, David Howells,
	linux-afs

On Wed 10-12-25 23:20:23, Deepak Karn wrote:
> On Wed, Dec 10, 2025 at 10:54 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Wed 10-12-25 20:59:00, Deepak Karn wrote:
> > > On Wed, Dec 10, 2025 at 3:25 PM Jan Kara <jack@suse.cz> wrote:
> > > >
> > > > On Tue 09-12-25 22:00:04, Deepak Karn wrote:
> > > > > On Tue, Dec 9, 2025 at 4:48 PM Jan Kara <jack@suse.cz> wrote:
> > > > > >
> > > > > > On Tue 09-12-25 01:43:33, Deepakkumar Karn wrote:
> > > > > > > On Mon, 8 Dec 2025 11:30:24 -0800, Darrick J. Wong wrote:
> > > > > > > > > drop_buffers() dereferences the buffer_head pointer returned by
> > > > > > > > > folio_buffers() without checking for NULL. This leads to a null pointer
> > > > > > > > > dereference when called from try_to_free_buffers() on a folio with no
> > > > > > > > > buffers attached. This happens when filemap_release_folio() is called on
> > > > > > > > > a folio belonging to a mapping with AS_RELEASE_ALWAYS set but without
> > > > > > > > > release_folio address_space operation defined. In such case,
> > > > > > >
> > > > > > > > What user is that?  All the users of AS_RELEASE_ALWAYS in 6.18 appear to
> > > > > > > > supply a ->release_folio.  Is this some new thing in 6.19?
> > > > > > >
> > > > > > > AFS directories SET AS_RELEASE_ALWAYS but have not .release_folio.
> > > > > >
> > > > > > AFAICS AFS sets AS_RELEASE_ALWAYS only for symlinks but not for
> > > > > > directories? Anyway I agree AFS symlinks will have AS_RELEASE_ALWAYS but no
> > > > > > .release_folio callback. And this looks like a bug in AFS because AFAICT
> > > > > > there's no point in setting AS_RELEASE_ALWAYS when you don't have
> > > > > > .release_folio callback. Added relevant people to CC.
> > > > > >
> > > > > >                                                                 Honza
> > > > >
> > > > > Thank you for your response Jan. As you suggested, the bug is in AFS.
> > > > > Can we include this current defensive check in drop_buffers() and I can submit
> > > > > another patch to handle that bug of AFS we discussed?
> > > >
> > > > I'm not strongly opposed to that (although try_to_free_buffers() would seem
> > > > like a tad bit better place) but overall I don't think it's a great idea as
> > > > it would hide bugs. But perhaps with WARN_ON_ONCE() (to catch sloppy
> > > > programming) it would be a sensible hardening.
> > > >
> > >
> > > Thanks Jan for your response. As suggested, adding WARN_ON_ONCE() will be
> > > more sensible.
> > > I just wanted to clarify my understanding, you are suggesting adding
> > > WARN_ON_ONCE() in try_to_free_buffers() as this highlights the issue and
> > > also solves the concern. If my understanding is wrong please let me know,
> > > I will share the updated patch.
> >
> > Yes, I meant something like:
> >
> >         if (WARN_ON_ONCE(!folio_buffers(folio)))
> >                 return true;
> >
> > in try_to_free_buffers().
> >
> 
> Yes this is what I am also going for, just a minor change that I was
> going to return false.
> 
> Reason:
> From the flow it should return false whenever failed to free buffers /
> operation couldn't complete. Do you think otherwise.

Well, it's rather we should return false when the page cannot be freed
because we were unable to free the buffers. When folio_buffers() are NULL,
the page *can* be freed so we should be returning true in that case.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] fs: add NULL check in drop_buffers() to prevent null-ptr-deref
  2025-12-11  9:15                 ` Jan Kara
@ 2025-12-11 12:50                   ` Deepak Karn
  0 siblings, 0 replies; 11+ messages in thread
From: Deepak Karn @ 2025-12-11 12:50 UTC (permalink / raw)
  To: Jan Kara
  Cc: djwong, brauner, linux-fsdevel, linux-kernel,
	syzbot+e07658f51ca22ab65b4e, syzkaller-bugs, viro, David Howells,
	linux-afs

On Thu, Dec 11, 2025 at 2:46 PM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 10-12-25 23:20:23, Deepak Karn wrote:
> > On Wed, Dec 10, 2025 at 10:54 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Wed 10-12-25 20:59:00, Deepak Karn wrote:
> > > > On Wed, Dec 10, 2025 at 3:25 PM Jan Kara <jack@suse.cz> wrote:
> > > > >
> > > > > On Tue 09-12-25 22:00:04, Deepak Karn wrote:
> > > > > > On Tue, Dec 9, 2025 at 4:48 PM Jan Kara <jack@suse.cz> wrote:
> > > > > > >
> > > > > > > On Tue 09-12-25 01:43:33, Deepakkumar Karn wrote:
> > > > > > > > On Mon, 8 Dec 2025 11:30:24 -0800, Darrick J. Wong wrote:
> > > > > > > > > > drop_buffers() dereferences the buffer_head pointer returned by
> > > > > > > > > > folio_buffers() without checking for NULL. This leads to a null pointer
> > > > > > > > > > dereference when called from try_to_free_buffers() on a folio with no
> > > > > > > > > > buffers attached. This happens when filemap_release_folio() is called on
> > > > > > > > > > a folio belonging to a mapping with AS_RELEASE_ALWAYS set but without
> > > > > > > > > > release_folio address_space operation defined. In such case,
> > > > > > > >
> > > > > > > > > What user is that?  All the users of AS_RELEASE_ALWAYS in 6.18 appear to
> > > > > > > > > supply a ->release_folio.  Is this some new thing in 6.19?
> > > > > > > >
> > > > > > > > AFS directories SET AS_RELEASE_ALWAYS but have not .release_folio.
> > > > > > >
> > > > > > > AFAICS AFS sets AS_RELEASE_ALWAYS only for symlinks but not for
> > > > > > > directories? Anyway I agree AFS symlinks will have AS_RELEASE_ALWAYS but no
> > > > > > > .release_folio callback. And this looks like a bug in AFS because AFAICT
> > > > > > > there's no point in setting AS_RELEASE_ALWAYS when you don't have
> > > > > > > .release_folio callback. Added relevant people to CC.
> > > > > > >
> > > > > > >                                                                 Honza
> > > > > >
> > > > > > Thank you for your response Jan. As you suggested, the bug is in AFS.
> > > > > > Can we include this current defensive check in drop_buffers() and I can submit
> > > > > > another patch to handle that bug of AFS we discussed?
> > > > >
> > > > > I'm not strongly opposed to that (although try_to_free_buffers() would seem
> > > > > like a tad bit better place) but overall I don't think it's a great idea as
> > > > > it would hide bugs. But perhaps with WARN_ON_ONCE() (to catch sloppy
> > > > > programming) it would be a sensible hardening.
> > > > >
> > > >
> > > > Thanks Jan for your response. As suggested, adding WARN_ON_ONCE() will be
> > > > more sensible.
> > > > I just wanted to clarify my understanding, you are suggesting adding
> > > > WARN_ON_ONCE() in try_to_free_buffers() as this highlights the issue and
> > > > also solves the concern. If my understanding is wrong please let me know,
> > > > I will share the updated patch.
> > >
> > > Yes, I meant something like:
> > >
> > >         if (WARN_ON_ONCE(!folio_buffers(folio)))
> > >                 return true;
> > >
> > > in try_to_free_buffers().
> > >
> >
> > Yes this is what I am also going for, just a minor change that I was
> > going to return false.
> >
> > Reason:
> > From the flow it should return false whenever failed to free buffers /
> > operation couldn't complete. Do you think otherwise.
>
> Well, it's rather we should return false when the page cannot be freed
> because we were unable to free the buffers. When folio_buffers() are NULL,
> the page *can* be freed so we should be returning true in that case.
>
Thank you Jan for your response and clarifying this. I will submit an
updated patch.

Regards,
Deepakkumar Karn


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2025-12-11 12:51 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-12-08 19:03 [PATCH v2] fs: add NULL check in drop_buffers() to prevent null-ptr-deref Deepakkumar Karn
2025-12-08 19:30 ` Darrick J. Wong
2025-12-08 20:13   ` Deepakkumar Karn
2025-12-09 11:18     ` Jan Kara
2025-12-09 16:30       ` Deepak Karn
2025-12-10  9:55         ` Jan Kara
2025-12-10 15:29           ` Deepak Karn
2025-12-10 17:23             ` Jan Kara
2025-12-10 17:50               ` Deepak Karn
2025-12-11  9:15                 ` Jan Kara
2025-12-11 12:50                   ` Deepak Karn

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox