[PATCH] ufs: free the buffer head container in ubh_bforget

[PATCH] ufs: free the buffer head container in ubh_bforget

[Ali Ahmet Memis]

ubh_bforget() forgets the buffer heads referenced by a struct
ufs_buffer_head but never frees the container itself, unlike its
sibling ubh_brelse() which calls kfree() on the way out. The only
caller, free_full_branch(), allocates the container through ubh_bread()
while releasing an indirect block during truncate, so every fully
removed indirect block leaks one ufs_buffer_head. Truncating or
unlinking a large file then leaks one allocation per indirect block,
which kmemleak reports with a free_full_branch, ufs_truncate_blocks,
ufs_evict_inode backtrace.

Free the container after forgetting its buffers, mirroring ubh_brelse().

Luis Henriques posted a fix for this leak in 2018, but it was never
applied while fs/ufs had no active maintainer, and the leak is still
present.

Link: https://lore.kernel.org/all/20180705150415.25070-1-lhenriques@suse.com/
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
---
 fs/ufs/util.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/ufs/util.c b/fs/ufs/util.c
index dff6f7461..3c65fbef6 100644
--- a/fs/ufs/util.c
+++ b/fs/ufs/util.c
@@ -117,8 +117,10 @@ void ubh_bforget (struct ufs_buffer_head * ubh)
 	unsigned i;
 	if (!ubh) 
 		return;
-	for ( i = 0; i < ubh->count; i++ ) if ( ubh->bh[i] ) 
-		bforget (ubh->bh[i]);
+	for (i = 0; i < ubh->count; i++)
+		if (ubh->bh[i])
+			bforget(ubh->bh[i]);
+	kfree(ubh);
 }
  
 int ubh_buffer_dirty (struct ufs_buffer_head * ubh)
-- 
2.54.0

Re: [PATCH] ufs: free the buffer head container in ubh_bforget

[Luis Henriques]

On Sat, Aug 01 2026, Ali Ahmet Memis wrote:

> ubh_bforget() forgets the buffer heads referenced by a struct
> ufs_buffer_head but never frees the container itself, unlike its
> sibling ubh_brelse() which calls kfree() on the way out. The only
> caller, free_full_branch(), allocates the container through ubh_bread()
> while releasing an indirect block during truncate, so every fully
> removed indirect block leaks one ufs_buffer_head. Truncating or
> unlinking a large file then leaks one allocation per indirect block,
> which kmemleak reports with a free_full_branch, ufs_truncate_blocks,
> ufs_evict_inode backtrace.
>
> Free the container after forgetting its buffers, mirroring ubh_brelse().
>
> Luis Henriques posted a fix for this leak in 2018, but it was never
> applied while fs/ufs had no active maintainer, and the leak is still
> present.

It's been a while, and I don't even remember why I was running xfstests
against this filesystem :-)

But the fix still looks OK.  My original patch also dropped the 'if', but
that's just a minor detail.  I don't know who's using this ufs driver
these days -- each BSD has it's own thing, and it's likely to be risky to
mount a filesystem in rw mode.

Cheers,
-- 
Luís


> Link: https://lore.kernel.org/all/20180705150415.25070-1-lhenriques@suse.com/
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
> ---
>  fs/ufs/util.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ufs/util.c b/fs/ufs/util.c
> index dff6f7461..3c65fbef6 100644
> --- a/fs/ufs/util.c
> +++ b/fs/ufs/util.c
> @@ -117,8 +117,10 @@ void ubh_bforget (struct ufs_buffer_head * ubh)
>  	unsigned i;
>  	if (!ubh) 
>  		return;
> -	for ( i = 0; i < ubh->count; i++ ) if ( ubh->bh[i] ) 
> -		bforget (ubh->bh[i]);
> +	for (i = 0; i < ubh->count; i++)
> +		if (ubh->bh[i])
> +			bforget(ubh->bh[i]);
> +	kfree(ubh);
>  }
>   
>  int ubh_buffer_dirty (struct ufs_buffer_head * ubh)
> -- 
> 2.54.0
>

[PATCH v2] ufs: free the buffer head container in ubh_bforget

[Ali Ahmet Memis]

ubh_bforget() forgets the buffer heads referenced by a struct
ufs_buffer_head but never frees the container itself, unlike its sibling
ubh_brelse() which calls kfree() on the way out. The only caller,
free_full_branch(), allocates the container through ubh_bread() while
releasing an indirect block during truncate, so every fully removed
indirect block leaks one ufs_buffer_head. Truncating or unlinking a
large file then leaks one allocation per indirect block, which kmemleak
reports with a free_full_branch, ufs_truncate_blocks, ufs_evict_inode
backtrace.

Free the container after forgetting its buffers, mirroring ubh_brelse().
Drop the NULL test in the loop as well, which Luis already noted is
redundant because bforget() ignores a NULL buffer head; ubh_brelse() has
no such test either, so the two now match.

Luis Henriques posted this fix in 2018. It never got a single reply and
was never applied, while fs/ufs had no active maintainer, and the leak is
still present.

Link: https://lore.kernel.org/all/20180705150415.25070-1-lhenriques@suse.com/
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Ali Ahmet Memis <ali@iusegentoo.com>
---
v2: drop the NULL test in the loop, as Luis pointed out. That makes this
    the same change as his 2018 patch; if you would rather it went in
    under his authorship I am happy to resend it that way, his call.
    Whitespace on the touched lines modernised to keep checkpatch quiet.

v1: https://lore.kernel.org/all/20260801024119.12667-1-ali@iusegentoo.com/

 fs/ufs/util.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/ufs/util.c b/fs/ufs/util.c
index dff6f74618de..603d80b93066 100644
--- a/fs/ufs/util.c
+++ b/fs/ufs/util.c
@@ -117,8 +117,9 @@ void ubh_bforget (struct ufs_buffer_head * ubh)
 	unsigned i;
 	if (!ubh) 
 		return;
-	for ( i = 0; i < ubh->count; i++ ) if ( ubh->bh[i] ) 
-		bforget (ubh->bh[i]);
+	for (i = 0; i < ubh->count; i++)
+		bforget(ubh->bh[i]);
+	kfree(ubh);
 }
  
 int ubh_buffer_dirty (struct ufs_buffer_head * ubh)

base-commit: 2d2338c93da79b3bfe4b6099a931d9468d539952
-- 
2.55.0

Re: [PATCH] ufs: free the buffer head container in ubh_bforget

[Ali Ahmet Memis]

On Sun, Aug 02 2026, Luis Henriques wrote:

> But the fix still looks OK.  My original patch also dropped the 'if',
> but that's just a minor detail.

It is not only cosmetic, and your own commit message already gave the
reason: bforget() is a no-op for a NULL buffer head. ubh_brelse() has no
such test either, so dropping it is what actually makes the two functions
match, which is what the changelog claims. v2 is posted with that
changed:

  https://lore.kernel.org/all/20260802114130.6261-1-ali@iusegentoo.com/

That makes v2 the same change as your 2018 patch. If you would rather it
went in under your authorship, say so and I will resend it that way. Your
posting got no replies at all back then, so it stalled rather than being
turned down.

> I don't know who's using this ufs driver these days -- each BSD has
> it's own thing, and it's likely to be risky to mount a filesystem in
> rw mode.

It is risky, and I have been finding out how much. I sent a series
yesterday for cases where fs/ufs mishandles filesystems that are
perfectly valid rather than crafted, among them a short symlink carrying
extended attributes: because the fast symlink test looks at i_blocks
instead of i_size, the link target is taken for a block pointer array,
readlink walks off the device and unlink hands the target bytes to
ufs_free_fragments().

  https://lore.kernel.org/all/20260801225530.148386-1-ali@iusegentoo.com/

So the read-write path could use more attention rather than less, at
least while it is still in the tree and mountable.

If v2 looks right to you, an Acked-by would help it move along. The
diagnosis was yours.

Apologies if this and the v2 reach you twice or late: I took your address
from the 2018 posting and mail to it bounced, so your copies did not go
out with the rest.

Thanks for looking at this after so long.

--
Ali