[PATCH] fs: Use a cleanup attribute in copy_fdtable()

[PATCH] fs: Use a cleanup attribute in copy_fdtable()

[Miquel Sabaté Solà]

This is a small cleanup in which by using the __free(kfree) cleanup
attribute we can avoid three labels to go to, and the code turns to be
more concise and easier to follow.

Signed-off-by: Miquel Sabaté Solà <mssola@mssola.com>
---
 fs/file.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 28743b742e3c..32b937a04003 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -161,7 +161,7 @@ static void copy_fdtable(struct fdtable *nfdt, struct fdtable *ofdt)
  */
 static struct fdtable *alloc_fdtable(unsigned int slots_wanted)
 {
-	struct fdtable *fdt;
+	struct fdtable *fdt __free(kfree) = NULL;
 	unsigned int nr;
 	void *data;
 
@@ -214,18 +214,20 @@ static struct fdtable *alloc_fdtable(unsigned int slots_wanted)
 
 	fdt = kmalloc(sizeof(struct fdtable), GFP_KERNEL_ACCOUNT);
 	if (!fdt)
-		goto out;
+		return ERR_PTR(-ENOMEM);
 	fdt->max_fds = nr;
 	data = kvmalloc_array(nr, sizeof(struct file *), GFP_KERNEL_ACCOUNT);
 	if (!data)
-		goto out_fdt;
+		return ERR_PTR(-ENOMEM);
 	fdt->fd = data;
 
 	data = kvmalloc(max_t(size_t,
 				 2 * nr / BITS_PER_BYTE + BITBIT_SIZE(nr), L1_CACHE_BYTES),
 				 GFP_KERNEL_ACCOUNT);
-	if (!data)
-		goto out_arr;
+	if (!data) {
+		kvfree(fdt->fd);
+		return ERR_PTR(-ENOMEM);
+	}
 	fdt->open_fds = data;
 	data += nr / BITS_PER_BYTE;
 	fdt->close_on_exec = data;
@@ -233,13 +235,6 @@ static struct fdtable *alloc_fdtable(unsigned int slots_wanted)
 	fdt->full_fds_bits = data;
 
 	return fdt;
-
-out_arr:
-	kvfree(fdt->fd);
-out_fdt:
-	kfree(fdt);
-out:
-	return ERR_PTR(-ENOMEM);
 }
 
 /*
-- 
2.51.0

Re: [PATCH] fs: Use a cleanup attribute in copy_fdtable()

[Al Viro]

On Sat, Oct 04, 2025 at 11:03:40PM +0200, Miquel Sabaté Solà wrote:
> This is a small cleanup in which by using the __free(kfree) cleanup
> attribute we can avoid three labels to go to, and the code turns to be
> more concise and easier to follow.

Have you tried to build and boot that?

That aside, it is not easier to follow in that form - especially since
kfree() is *not* the right destructor for the object in question.
Having part of destructor done via sodding __cleanup, with the rest
open-coded on various failure exits is confusing as hell.

RAII has its uses, but applied unidiomatically it ends up being a mess
that is harder to follow and reason about than the dreadful gotos it
replaces.

NAKed-by: Al Viro <viro@zeniv.linux.org.uk>

Re: [PATCH] fs: Use a cleanup attribute in copy_fdtable()

[Miquel Sabaté Solà]

[-- Attachment #1: Type: text/plain, Size: 1234 bytes --]

Al Viro @ 2025-10-04 22:19 +01:

> On Sat, Oct 04, 2025 at 11:03:40PM +0200, Miquel Sabaté Solà wrote:
>> This is a small cleanup in which by using the __free(kfree) cleanup
>> attribute we can avoid three labels to go to, and the code turns to be
>> more concise and easier to follow.
>
> Have you tried to build and boot that?

Yes, and it worked on my machine...

>
> That aside, it is not easier to follow in that form - especially since
> kfree() is *not* the right destructor for the object in question.
> Having part of destructor done via sodding __cleanup, with the rest
> open-coded on various failure exits is confusing as hell.
>
> RAII has its uses, but applied unidiomatically it ends up being a mess
> that is harder to follow and reason about than the dreadful gotos it
> replaces.

I agree that it would generally not be the right destructor for it, but
in the case of this function it ends up being equivalent. But I see
that, if in general that wouldn't be the proper way, declaring the
fdtable variable like that can be misleading, even if equivalent
here. Thus, defeating the purpose of this patch.

>
> NAKed-by: Al Viro <viro@zeniv.linux.org.uk>

Thanks for the review,
Miquel

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCH] fs: Use a cleanup attribute in copy_fdtable()

[Al Viro]

On Sun, Oct 05, 2025 at 07:37:50AM +0200, Miquel Sabaté Solà wrote:
> Al Viro @ 2025-10-04 22:19 +01:
> 
> > On Sat, Oct 04, 2025 at 11:03:40PM +0200, Miquel Sabaté Solà wrote:
> >> This is a small cleanup in which by using the __free(kfree) cleanup
> >> attribute we can avoid three labels to go to, and the code turns to be
> >> more concise and easier to follow.
> >
> > Have you tried to build and boot that?
> 
> Yes, and it worked on my machine...

Unfortunately, it ends up calling that kfree() on success as well as on failure.
Idiomatic way to avoid that would be
	return no_free_ptr(fdt);
but you've left bare
	return fdt;
in there, ending up with returning dangling pointers to the caller.  So as
soon as you get more than BITS_PER_LONG descriptors used by a process,
you'll get trouble.  In particular, bash(1) running as an interactive shell
would hit that - it has descriptor 255 opened...

Re: [PATCH] fs: Use a cleanup attribute in copy_fdtable()

[Miquel Sabaté Solà]

[-- Attachment #1: Type: text/plain, Size: 1289 bytes --]

Al Viro @ 2025-10-05 10:01 +01:

> On Sun, Oct 05, 2025 at 07:37:50AM +0200, Miquel Sabaté Solà wrote:
>> Al Viro @ 2025-10-04 22:19 +01:
>>
>> > On Sat, Oct 04, 2025 at 11:03:40PM +0200, Miquel Sabaté Solà wrote:
>> >> This is a small cleanup in which by using the __free(kfree) cleanup
>> >> attribute we can avoid three labels to go to, and the code turns to be
>> >> more concise and easier to follow.
>> >
>> > Have you tried to build and boot that?
>>
>> Yes, and it worked on my machine...
>
> Unfortunately, it ends up calling that kfree() on success as well as on failure.
> Idiomatic way to avoid that would be
> 	return no_free_ptr(fdt);
> but you've left bare
> 	return fdt;
> in there, ending up with returning dangling pointers to the caller.  So as
> soon as you get more than BITS_PER_LONG descriptors used by a process,
> you'll get trouble.  In particular, bash(1) running as an interactive shell
> would hit that - it has descriptor 255 opened...

Ugh, this is just silly from my end...

You are absolutely right. I don't know what the hell I was doing while
testing that prevented me from realizing this before, but as you say
it's quite obvious and I was just blind or something.

Sorry for the noise and thanks for your patience...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 897 bytes --]

Re: [PATCH] fs: Use a cleanup attribute in copy_fdtable()

[Al Viro]

On Sun, Oct 05, 2025 at 07:41:47PM +0200, Miquel Sabaté Solà wrote:
> Al Viro @ 2025-10-05 10:01 +01:
> 
> > On Sun, Oct 05, 2025 at 07:37:50AM +0200, Miquel Sabaté Solà wrote:
> >> Al Viro @ 2025-10-04 22:19 +01:
> >>
> >> > On Sat, Oct 04, 2025 at 11:03:40PM +0200, Miquel Sabaté Solà wrote:
> >> >> This is a small cleanup in which by using the __free(kfree) cleanup
> >> >> attribute we can avoid three labels to go to, and the code turns to be
> >> >> more concise and easier to follow.
> >> >
> >> > Have you tried to build and boot that?
> >>
> >> Yes, and it worked on my machine...
> >
> > Unfortunately, it ends up calling that kfree() on success as well as on failure.
> > Idiomatic way to avoid that would be
> > 	return no_free_ptr(fdt);
> > but you've left bare
> > 	return fdt;
> > in there, ending up with returning dangling pointers to the caller.  So as
> > soon as you get more than BITS_PER_LONG descriptors used by a process,
> > you'll get trouble.  In particular, bash(1) running as an interactive shell
> > would hit that - it has descriptor 255 opened...
> 
> Ugh, this is just silly from my end...
> 
> You are absolutely right. I don't know what the hell I was doing while
> testing that prevented me from realizing this before, but as you say
> it's quite obvious and I was just blind or something.
> 
> Sorry for the noise and thanks for your patience...

FWIW, the real low-level destructor (__free_fdtable()) *does* cope with ->fd
or ->open_fds left NULL, so theoretically we could replace kmalloc with
kzalloc in alloc_fdtable(), add use that thing via DEFINE_FREE()/__free(...);
I'm not sure if it's a good idea, though - at the very least, that property
of destructor would have to be spelled out with explanations, both in
__free_fdtable() and in alloc_fdtable().

Matter of taste, but IMO it's not worth bothering with - figuring out why
the damn thing is correct would take at least as much time and attention
from readers as the current variant does.

BTW, there's a chance to kill struct fdtable off - a project that got stalled
about a year ago (see https://lore.kernel.org/all/20240806010217.GL5334@ZenIV/
and subthread from there on for details) that just might end up eliminating
that double indirect.  I'm not saying that it's a reason not to do cleanups in
what exists right now, just a tangentially related thing that might be interesting
to resurrect...

Re: [PATCH] fs: Use a cleanup attribute in copy_fdtable()

[Miquel Sabaté Solà]

[-- Attachment #1: Type: text/plain, Size: 2685 bytes --]

Al Viro @ 2025-10-05 22:30 +01:

> On Sun, Oct 05, 2025 at 07:41:47PM +0200, Miquel Sabaté Solà wrote:
>> Al Viro @ 2025-10-05 10:01 +01:
>>
>> > On Sun, Oct 05, 2025 at 07:37:50AM +0200, Miquel Sabaté Solà wrote:
>> >> Al Viro @ 2025-10-04 22:19 +01:
>> >>
>> >> > On Sat, Oct 04, 2025 at 11:03:40PM +0200, Miquel Sabaté Solà wrote:
>> >> >> This is a small cleanup in which by using the __free(kfree) cleanup
>> >> >> attribute we can avoid three labels to go to, and the code turns to be
>> >> >> more concise and easier to follow.
>> >> >
>> >> > Have you tried to build and boot that?
>> >>
>> >> Yes, and it worked on my machine...
>> >
>> > Unfortunately, it ends up calling that kfree() on success as well as on failure.
>> > Idiomatic way to avoid that would be
>> > 	return no_free_ptr(fdt);
>> > but you've left bare
>> > 	return fdt;
>> > in there, ending up with returning dangling pointers to the caller.  So as
>> > soon as you get more than BITS_PER_LONG descriptors used by a process,
>> > you'll get trouble.  In particular, bash(1) running as an interactive shell
>> > would hit that - it has descriptor 255 opened...
>>
>> Ugh, this is just silly from my end...
>>
>> You are absolutely right. I don't know what the hell I was doing while
>> testing that prevented me from realizing this before, but as you say
>> it's quite obvious and I was just blind or something.
>>
>> Sorry for the noise and thanks for your patience...
>
> FWIW, the real low-level destructor (__free_fdtable()) *does* cope with ->fd
> or ->open_fds left NULL, so theoretically we could replace kmalloc with
> kzalloc in alloc_fdtable(), add use that thing via DEFINE_FREE()/__free(...);
> I'm not sure if it's a good idea, though - at the very least, that property
> of destructor would have to be spelled out with explanations, both in
> __free_fdtable() and in alloc_fdtable().
>
> Matter of taste, but IMO it's not worth bothering with - figuring out why
> the damn thing is correct would take at least as much time and attention
> from readers as the current variant does.

Agreed.

>
> BTW, there's a chance to kill struct fdtable off - a project that got stalled
> about a year ago (see https://lore.kernel.org/all/20240806010217.GL5334@ZenIV/
> and subthread from there on for details) that just might end up eliminating
> that double indirect.  I'm not saying that it's a reason not to do cleanups in
> what exists right now, just a tangentially related thing that might be interesting
> to resurrect...

That looks interesting indeed, I'll take a look at this topic whenever I
have some spare time.

Thanks!
Miquel

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 897 bytes --]