[PATCH v2] xfs: zero newly allocated btree root space

[PATCH v2] xfs: zero newly allocated btree root space

[Yousef Alhouseen]

xfs_broot_realloc() preserves the existing in-inode btree root while
growing its allocation, but leaves the added bytes uninitialized.  The
inode log formatter copies if_broot_bytes bytes into the journal, so those
bytes reach the log record and its CRC calculation before every location
has necessarily been overwritten by btree updates.

Request __GFP_ZERO for the initial allocation and every subsequent
allocation or reallocation, as required by krealloc() semantics.  This
keeps stale heap contents out of the filesystem log without a separate
memset after each growth.

Fixes: 6c1c55ac3c05 ("xfs: refactor the inode fork memory allocation functions")
Suggested-by: Darrick J. Wong <djwong@kernel.org>
Reported-by: syzbot+97f2c05378c5d68dcb8c@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=97f2c05378c5d68dcb8c
Cc: stable@vger.kernel.org
Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
Changes in v2:
- Use __GFP_ZERO instead of an explicit memset after krealloc().
- Apply __GFP_ZERO consistently across the allocation lifetime.

 fs/xfs/libxfs/xfs_inode_fork.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index 606a36526ce2..dc05540fa85b 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -384,7 +384,8 @@ xfs_broot_alloc(
 	ASSERT(ifp->if_broot == NULL);
 
 	ifp->if_broot = kmalloc(new_size,
-				GFP_KERNEL | __GFP_NOLOCKDEP | __GFP_NOFAIL);
+				GFP_KERNEL | __GFP_NOLOCKDEP | __GFP_NOFAIL |
+				__GFP_ZERO);
 	ifp->if_broot_bytes = new_size;
 	return ifp->if_broot;
 }
@@ -417,7 +418,8 @@ xfs_broot_realloc(
 	if (ifp->if_broot_bytes > 0 && ifp->if_broot_bytes > new_size) {
 		struct xfs_btree_block	*old_broot = ifp->if_broot;
 
-		ifp->if_broot = kmalloc(new_size, GFP_KERNEL | __GFP_NOFAIL);
+		ifp->if_broot = kmalloc(new_size,
+					GFP_KERNEL | __GFP_NOFAIL | __GFP_ZERO);
 		ifp->if_broot_bytes = new_size;
 		memcpy(ifp->if_broot, old_broot, new_size);
 		kfree(old_broot);
@@ -429,7 +431,7 @@ xfs_broot_realloc(
 	 * object.
 	 */
 	ifp->if_broot = krealloc(ifp->if_broot, new_size,
-			GFP_KERNEL | __GFP_NOFAIL);
+			GFP_KERNEL | __GFP_NOFAIL | __GFP_ZERO);
 	ifp->if_broot_bytes = new_size;
 	return ifp->if_broot;
 }
-- 
2.54.0

Re: [PATCH v2] xfs: zero newly allocated btree root space

[Darrick J. Wong]

On Tue, Jun 30, 2026 at 12:06:21PM +0200, Yousef Alhouseen wrote:
> xfs_broot_realloc() preserves the existing in-inode btree root while
> growing its allocation, but leaves the added bytes uninitialized.  The
> inode log formatter copies if_broot_bytes bytes into the journal, so those
> bytes reach the log record and its CRC calculation before every location
> has necessarily been overwritten by btree updates.
> 
> Request __GFP_ZERO for the initial allocation and every subsequent
> allocation or reallocation, as required by krealloc() semantics.  This
> keeps stale heap contents out of the filesystem log without a separate
> memset after each growth.
> 
> Fixes: 6c1c55ac3c05 ("xfs: refactor the inode fork memory allocation functions")
> Suggested-by: Darrick J. Wong <djwong@kernel.org>
> Reported-by: syzbot+97f2c05378c5d68dcb8c@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=97f2c05378c5d68dcb8c

I wonder, did you figure out exactly *which* code path was leaving
if_broot partially uninitialized?  Somewhere out there, someone will get
cranky at the reduced performance that comes from zeroing (especially on
krealloc) when most of the codepaths will immediately zero/set the
buffer anyway.

> Cc: stable@vger.kernel.org
> Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>

In the long run I'm willing to take a small performance hit of having
many layered protections as is reasonably performant to avoid spilling
kernel memory contents to disk, though, so

Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

(others may disagree)

--D

> ---
> Changes in v2:
> - Use __GFP_ZERO instead of an explicit memset after krealloc().
> - Apply __GFP_ZERO consistently across the allocation lifetime.
> 
>  fs/xfs/libxfs/xfs_inode_fork.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 606a36526ce2..dc05540fa85b 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -384,7 +384,8 @@ xfs_broot_alloc(
>  	ASSERT(ifp->if_broot == NULL);
>  
>  	ifp->if_broot = kmalloc(new_size,
> -				GFP_KERNEL | __GFP_NOLOCKDEP | __GFP_NOFAIL);
> +				GFP_KERNEL | __GFP_NOLOCKDEP | __GFP_NOFAIL |
> +				__GFP_ZERO);
>  	ifp->if_broot_bytes = new_size;
>  	return ifp->if_broot;
>  }
> @@ -417,7 +418,8 @@ xfs_broot_realloc(
>  	if (ifp->if_broot_bytes > 0 && ifp->if_broot_bytes > new_size) {
>  		struct xfs_btree_block	*old_broot = ifp->if_broot;
>  
> -		ifp->if_broot = kmalloc(new_size, GFP_KERNEL | __GFP_NOFAIL);
> +		ifp->if_broot = kmalloc(new_size,
> +					GFP_KERNEL | __GFP_NOFAIL | __GFP_ZERO);
>  		ifp->if_broot_bytes = new_size;
>  		memcpy(ifp->if_broot, old_broot, new_size);
>  		kfree(old_broot);
> @@ -429,7 +431,7 @@ xfs_broot_realloc(
>  	 * object.
>  	 */
>  	ifp->if_broot = krealloc(ifp->if_broot, new_size,
> -			GFP_KERNEL | __GFP_NOFAIL);
> +			GFP_KERNEL | __GFP_NOFAIL | __GFP_ZERO);
>  	ifp->if_broot_bytes = new_size;
>  	return ifp->if_broot;
>  }
> -- 
> 2.54.0
> 

Re: [PATCH v2] xfs: zero newly allocated btree root space

[Yousef Alhouseen]

Yes. The KMSAN report traces the allocation through
xfs_bmap_extents_to_btree(), which calls xfs_bmap_broot_realloc(...,
1). With no existing root, that reaches krealloc(NULL, new_size). The
conversion initializes the block header, first key, and end-anchored
pointer, but does not initialize the layout/alignment gap within the
full if_broot_bytes allocation. XFS_ILOG_DBROOT later copies that
complete byte count into the log.

So the observed bytes are root-layout slack from the extents-to-btree
conversion, rather than record storage that a later insertion should
have filled.

Thanks for the review.

On Tue, 30 Jun 2026 09:11:13 -0700, "Darrick J. Wong" <djwong@kernel.org> wrote:
> On Tue, Jun 30, 2026 at 12:06:21PM +0200, Yousef Alhouseen wrote:
> > xfs_broot_realloc() preserves the existing in-inode btree root while
> > growing its allocation, but leaves the added bytes uninitialized. The
> > inode log formatter copies if_broot_bytes bytes into the journal, so those
> > bytes reach the log record and its CRC calculation before every location
> > has necessarily been overwritten by btree updates.
> >
> > Request __GFP_ZERO for the initial allocation and every subsequent
> > allocation or reallocation, as required by krealloc() semantics. This
> > keeps stale heap contents out of the filesystem log without a separate
> > memset after each growth.
> >
> > Fixes: 6c1c55ac3c05 ("xfs: refactor the inode fork memory allocation functions")
> > Suggested-by: Darrick J. Wong <djwong@kernel.org>
> > Reported-by: syzbot+97f2c05378c5d68dcb8c@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=97f2c05378c5d68dcb8c
>
> I wonder, did you figure out exactly *which* code path was leaving
> if_broot partially uninitialized? Somewhere out there, someone will get
> cranky at the reduced performance that comes from zeroing (especially on
> krealloc) when most of the codepaths will immediately zero/set the
> buffer anyway.
>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
>
> In the long run I'm willing to take a small performance hit of having
> many layered protections as is reasonably performant to avoid spilling
> kernel memory contents to disk, though, so
>
> Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
>
> (others may disagree)
>
> --D
>
> > ---
> > Changes in v2:
> > - Use __GFP_ZERO instead of an explicit memset after krealloc().
> > - Apply __GFP_ZERO consistently across the allocation lifetime.
> >
> > fs/xfs/libxfs/xfs_inode_fork.c | 8 +++++---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> > index 606a36526ce2..dc05540fa85b 100644
> > --- a/fs/xfs/libxfs/xfs_inode_fork.c
> > +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> > @@ -384,7 +384,8 @@ xfs_broot_alloc(
> > ASSERT(ifp->if_broot == NULL);
> >
> > ifp->if_broot = kmalloc(new_size,
> > - GFP_KERNEL | __GFP_NOLOCKDEP | __GFP_NOFAIL);
> > + GFP_KERNEL | __GFP_NOLOCKDEP | __GFP_NOFAIL |
> > + __GFP_ZERO);
> > ifp->if_broot_bytes = new_size;
> > return ifp->if_broot;
> > }
> > @@ -417,7 +418,8 @@ xfs_broot_realloc(
> > if (ifp->if_broot_bytes > 0 && ifp->if_broot_bytes > new_size) {
> > struct xfs_btree_block *old_broot = ifp->if_broot;
> >
> > - ifp->if_broot = kmalloc(new_size, GFP_KERNEL | __GFP_NOFAIL);
> > + ifp->if_broot = kmalloc(new_size,
> > + GFP_KERNEL | __GFP_NOFAIL | __GFP_ZERO);
> > ifp->if_broot_bytes = new_size;
> > memcpy(ifp->if_broot, old_broot, new_size);
> > kfree(old_broot);
> > @@ -429,7 +431,7 @@ xfs_broot_realloc(
> > * object.
> > */
> > ifp->if_broot = krealloc(ifp->if_broot, new_size,
> > - GFP_KERNEL | __GFP_NOFAIL);
> > + GFP_KERNEL | __GFP_NOFAIL | __GFP_ZERO);
> > ifp->if_broot_bytes = new_size;
> > return ifp->if_broot;
> > }
> > --
> > 2.54.0
> >

Re: [PATCH v2] xfs: zero newly allocated btree root space

[Christoph Hellwig]

On Tue, Jun 30, 2026 at 01:39:35PM -0700, Yousef Alhouseen wrote:
> Yes. The KMSAN report traces the allocation through
> xfs_bmap_extents_to_btree(), which calls xfs_bmap_broot_realloc(...,
> 1). With no existing root, that reaches krealloc(NULL, new_size). The
> conversion initializes the block header, first key, and end-anchored
> pointer, but does not initialize the layout/alignment gap within the
> full if_broot_bytes allocation. XFS_ILOG_DBROOT later copies that
> complete byte count into the log.
> 
> So the observed bytes are root-layout slack from the extents-to-btree
> conversion, rather than record storage that a later insertion should
> have filled.

This almost sounds like we should explicitly zero this case just for that
particular case.  Although Darrick was kinda heading in a different
direction and I don't want to start a fight here.

Re: [PATCH v2] xfs: zero newly allocated btree root space

[Darrick J. Wong]

On Wed, Jul 01, 2026 at 03:56:17AM -0700, Christoph Hellwig wrote:
> On Tue, Jun 30, 2026 at 01:39:35PM -0700, Yousef Alhouseen wrote:
> > Yes. The KMSAN report traces the allocation through
> > xfs_bmap_extents_to_btree(), which calls xfs_bmap_broot_realloc(...,
> > 1). With no existing root, that reaches krealloc(NULL, new_size). The
> > conversion initializes the block header, first key, and end-anchored
> > pointer, but does not initialize the layout/alignment gap within the
> > full if_broot_bytes allocation. XFS_ILOG_DBROOT later copies that
> > complete byte count into the log.
> > 
> > So the observed bytes are root-layout slack from the extents-to-btree
> > conversion, rather than record storage that a later insertion should
> > have filled.
> 
> This almost sounds like we should explicitly zero this case just for that
> particular case.  Although Darrick was kinda heading in a different
> direction and I don't want to start a fight here.

I'm fine with either solution; I simply prefer the one that fixes all
the current and future "oops we forgot to zero a hidden padding" bugs in
one action.

Carlos? :D

--D

Re: [PATCH v2] xfs: zero newly allocated btree root space

[Christoph Hellwig]

On Wed, Jul 01, 2026 at 08:52:34AM -0700, Darrick J. Wong wrote:
> > This almost sounds like we should explicitly zero this case just for that
> > particular case.  Although Darrick was kinda heading in a different
> > direction and I don't want to start a fight here.
> 
> I'm fine with either solution; I simply prefer the one that fixes all
> the current and future "oops we forgot to zero a hidden padding" bugs in
> one action.

Maybe we should avoid creating hidden zero padding to start with? :)
But this is a small allocation, and most of it gets overwritten
while the cache is still hot, so I can live with the unconditional
zeroing.  Still feels a bit odd to reach for the big hammer.

Re: [PATCH v2] xfs: zero newly allocated btree root space

[Darrick J. Wong]

On Thu, Jul 02, 2026 at 04:05:06AM -0700, Christoph Hellwig wrote:
> On Wed, Jul 01, 2026 at 08:52:34AM -0700, Darrick J. Wong wrote:
> > > This almost sounds like we should explicitly zero this case just for that
> > > particular case.  Although Darrick was kinda heading in a different
> > > direction and I don't want to start a fight here.
> > 
> > I'm fine with either solution; I simply prefer the one that fixes all
> > the current and future "oops we forgot to zero a hidden padding" bugs in
> > one action.
> 
> Maybe we should avoid creating hidden zero padding to start with? :)
> But this is a small allocation, and most of it gets overwritten
> while the cache is still hot, so I can live with the unconditional
> zeroing.  Still feels a bit odd to reach for the big hammer.

<shrug> If this is truly the only place where we forget to initialize
if_broot fully then I'm ok with just doing that and not going for
GFP_ZERO.  If we have an army of KASAN syzbotters whacking away at the
system, then I could reevaluate my resistance to whack-a-mole.

--D