[PATCH v2] fs: always return zero on success from replace_fd()

[PATCH v2] fs: always return zero on success from replace_fd()

[Thomas Weißschuh]

replace_fd() returns the number of the new file descriptor through the
return value of do_dup2(). However its callers never care about the
specific number. In fact the caller in receive_fd_replace() treats any
non-zero return value as an error and therefore never calls
__receive_sock() for most file descriptors, which is a bug.

To fix the bug in receive_fd_replace() and to avoid the same issue
happening in future callers, signal success through a plain zero.

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Link: https://lore.kernel.org/lkml/20250801220215.GS222315@ZenIV/
Fixes: 173817151b15 ("fs: Expand __receive_fd() to accept existing fd")
Fixes: 42eb0d54c08a ("fs: split receive_fd_replace from __receive_fd")
Cc: stable@vger.kernel.org
Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
---
Changes in v2:
- Move the fix to replace_fd() (Al)
- Link to v1: https://lore.kernel.org/r/20250801-fix-receive_fd_replace-v1-1-d46d600c74d6@linutronix.de
---
Untested, it stuck out while reading the code.
---
 fs/file.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/file.c b/fs/file.c
index 6d2275c3be9c6967d16c75d1b6521f9b58980926..f8a271265913951d755a5db559938d589219c4f2 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -1330,7 +1330,10 @@ int replace_fd(unsigned fd, struct file *file, unsigned flags)
 	err = expand_files(files, fd);
 	if (unlikely(err < 0))
 		goto out_unlock;
-	return do_dup2(files, file, fd, flags);
+	err = do_dup2(files, file, fd, flags);
+	if (err < 0)
+		goto out_unlock;
+	err = 0;
 
 out_unlock:
 	spin_unlock(&files->file_lock);

---
base-commit: d2eedaa3909be9102d648a4a0a50ccf64f96c54f
change-id: 20250801-fix-receive_fd_replace-7fdd5ce6532d

Best regards,
-- 
Thomas Weißschuh <thomas.weissschuh@linutronix.de>

Re: [PATCH v2] fs: always return zero on success from replace_fd()

[Christian Brauner]

On Mon, Aug 04, 2025 at 10:40:17AM +0200, Thomas Weißschuh wrote:
> replace_fd() returns the number of the new file descriptor through the
> return value of do_dup2(). However its callers never care about the
> specific number. In fact the caller in receive_fd_replace() treats any

To be pedantic as stated this isn't true: While they don't care about it
in their error handling they very much care about what specific file
descriptor number is used. Try and assign unexpected fd numbers for
coredumping and see what CVEs you can get out of it...

I'll take the patch but I'll amend it to just use a guard:

diff --git a/fs/file.c b/fs/file.c
index 6d2275c3be9c..858a55dbd5d8 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -1318,7 +1318,7 @@ __releases(&files->file_lock)
 int replace_fd(unsigned fd, struct file *file, unsigned flags)
 {
        int err;
-       struct files_struct *files = current->files;
+       struct files_struct *files;

        if (!file)
                return close_fd(fd);
@@ -1326,15 +1326,17 @@ int replace_fd(unsigned fd, struct file *file, unsigned flags)
        if (fd >= rlimit(RLIMIT_NOFILE))
                return -EBADF;

-       spin_lock(&files->file_lock);
+       files = current->files;
+
+       guard(spinlock)(&files->file_lock);
        err = expand_files(files, fd);
        if (unlikely(err < 0))
-               goto out_unlock;
-       return do_dup2(files, file, fd, flags);
+               return err;
+       err = do_dup2(files, file, fd, flags);
+       if (err < 0)
+               return err;

-out_unlock:
-       spin_unlock(&files->file_lock);
-       return err;
+       return 0;
 }

 /**

scoped_guard() works too but bloats the patch.

> non-zero return value as an error and therefore never calls
> __receive_sock() for most file descriptors, which is a bug.
> 
> To fix the bug in receive_fd_replace() and to avoid the same issue
> happening in future callers, signal success through a plain zero.
> 
> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> Link: https://lore.kernel.org/lkml/20250801220215.GS222315@ZenIV/
> Fixes: 173817151b15 ("fs: Expand __receive_fd() to accept existing fd")
> Fixes: 42eb0d54c08a ("fs: split receive_fd_replace from __receive_fd")
> Cc: stable@vger.kernel.org
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> ---
> Changes in v2:
> - Move the fix to replace_fd() (Al)
> - Link to v1: https://lore.kernel.org/r/20250801-fix-receive_fd_replace-v1-1-d46d600c74d6@linutronix.de
> ---
> Untested, it stuck out while reading the code.
> ---
>  fs/file.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/file.c b/fs/file.c
> index 6d2275c3be9c6967d16c75d1b6521f9b58980926..f8a271265913951d755a5db559938d589219c4f2 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -1330,7 +1330,10 @@ int replace_fd(unsigned fd, struct file *file, unsigned flags)
>  	err = expand_files(files, fd);
>  	if (unlikely(err < 0))
>  		goto out_unlock;
> -	return do_dup2(files, file, fd, flags);
> +	err = do_dup2(files, file, fd, flags);
> +	if (err < 0)
> +		goto out_unlock;
> +	err = 0;
>  
>  out_unlock:
>  	spin_unlock(&files->file_lock);
> 
> ---
> base-commit: d2eedaa3909be9102d648a4a0a50ccf64f96c54f
> change-id: 20250801-fix-receive_fd_replace-7fdd5ce6532d
> 
> Best regards,
> -- 
> Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> 

Re: [PATCH v2] fs: always return zero on success from replace_fd()

[Al Viro]

On Mon, Aug 04, 2025 at 02:33:13PM +0200, Christian Brauner wrote:

> +       guard(spinlock)(&files->file_lock);
>         err = expand_files(files, fd);
>         if (unlikely(err < 0))
> -               goto out_unlock;
> -       return do_dup2(files, file, fd, flags);
> +               return err;
> +       err = do_dup2(files, file, fd, flags);
> +       if (err < 0)
> +               return err;
> 
> -out_unlock:
> -       spin_unlock(&files->file_lock);
> -       return err;
> +       return 0;
>  }

NAK.  This is broken - do_dup2() drops ->file_lock.  And that's why I
loathe the guard() - it's too easy to get confused *and* assume that
it will DTRT, no need to check carefully.

Re: [PATCH v2] fs: always return zero on success from replace_fd()

[Thomas Weißschuh]

On Mon, Aug 04, 2025 at 04:52:29PM +0100, Al Viro wrote:
> On Mon, Aug 04, 2025 at 02:33:13PM +0200, Christian Brauner wrote:
> 
> > +       guard(spinlock)(&files->file_lock);
> >         err = expand_files(files, fd);
> >         if (unlikely(err < 0))
> > -               goto out_unlock;
> > -       return do_dup2(files, file, fd, flags);
> > +               return err;
> > +       err = do_dup2(files, file, fd, flags);
> > +       if (err < 0)
> > +               return err;
> > 
> > -out_unlock:
> > -       spin_unlock(&files->file_lock);
> > -       return err;
> > +       return 0;
> >  }
> 
> NAK.  This is broken - do_dup2() drops ->file_lock.  And that's why I
> loathe the guard() - it's too easy to get confused *and* assume that
> it will DTRT, no need to check carefully.

To be fair I also got this wrong in my original v2 patch not using guard().
I'll send a fixed version tomorrow.


Thomas

Re: [PATCH v2] fs: always return zero on success from replace_fd()

[Christian Brauner]

> I'll send a fixed version tomorrow.

Thanks! I appreciate it.

Re: [PATCH v2] fs: always return zero on success from replace_fd()

[Al Viro]

On Mon, Aug 04, 2025 at 04:52:29PM +0100, Al Viro wrote:
> On Mon, Aug 04, 2025 at 02:33:13PM +0200, Christian Brauner wrote:
> 
> > +       guard(spinlock)(&files->file_lock);
> >         err = expand_files(files, fd);
> >         if (unlikely(err < 0))
> > -               goto out_unlock;
> > -       return do_dup2(files, file, fd, flags);
> > +               return err;
> > +       err = do_dup2(files, file, fd, flags);
> > +       if (err < 0)
> > +               return err;
> > 
> > -out_unlock:
> > -       spin_unlock(&files->file_lock);
> > -       return err;
> > +       return 0;
> >  }
> 
> NAK.  This is broken - do_dup2() drops ->file_lock.  And that's why I
> loathe the guard() - it's too easy to get confused *and* assume that
> it will DTRT, no need to check carefully.

Note, BTW, that in actual replacing case do_dup2() has blocking
operations (closing the replaced reference) after dropping ->file_lock,
so making it locking-neutral would not be easy; doable (have it
return the old reference in the replacing case and adjust the callers
accordingly), but it's seriously not pretty (NULL/address of old file/ERR_PTR()
for return value, boilerplate in callers, etc.).  Having do_dup2() called
without ->file_lock and taking it inside is not an option - we could pull
expand_files() in there, but lookup of oldfd in actual dup2(2)/dup3(2) has
to be done within the same ->file_lock scope where it is inserted into the
table.

Sure, all things equal it's better to have functions locking-neutral, but
it's not always the best approach.  And while __free() allows for "we'd
passed the object to somebody else, it's not ours to consume anymore",
guard() does not.

Re: [PATCH v2] fs: always return zero on success from replace_fd()

[Christian Brauner]

On Mon, Aug 04, 2025 at 04:52:29PM +0100, Al Viro wrote:
> On Mon, Aug 04, 2025 at 02:33:13PM +0200, Christian Brauner wrote:
> 
> > +       guard(spinlock)(&files->file_lock);
> >         err = expand_files(files, fd);
> >         if (unlikely(err < 0))
> > -               goto out_unlock;
> > -       return do_dup2(files, file, fd, flags);
> > +               return err;
> > +       err = do_dup2(files, file, fd, flags);
> > +       if (err < 0)
> > +               return err;
> > 
> > -out_unlock:
> > -       spin_unlock(&files->file_lock);
> > -       return err;
> > +       return 0;
> >  }
> 
> NAK.  This is broken - do_dup2() drops ->file_lock.  And that's why I

Right, I missed that. Just say it's broken. You don't need to throw
around NAKs pointlessly. It's pretty clear when to drop ptaches without
throwing the meat cleaver through the room.

> loathe the guard() - it's too easy to get confused *and* assume that

The calling conventions of do_dup2() are terrible. The only reason it
drops file_lock itself instead of leaving it to the two callers that
have to acquire it anyway is because it wants to call filp_close() if
there's already a file on that fd.

And really the side-effect of dropping a lock implicitly is nasty
especially when the function doesn't even indicate that it does that in
it's name.

And guards are great.

Re: [PATCH v2] fs: always return zero on success from replace_fd()

[Al Viro]

On Tue, Aug 05, 2025 at 01:55:59PM +0200, Christian Brauner wrote:

> The calling conventions of do_dup2() are terrible. The only reason it
> drops file_lock itself instead of leaving it to the two callers that
> have to acquire it anyway is because it wants to call filp_close() if
> there's already a file on that fd.

Alternative calling conventions end up being nastier - I've tried.

> And really the side-effect of dropping a lock implicitly is nasty
> especially when the function doesn't even indicate that it does that in
> it's name.
> 
> And guards are great.

They do no allow to express things like "foo() consumes lock X".
From time to time, we *do* need that, and when that happens guards
become a menace.

Another case is
	lock
	if (lock-dependent condition)
		some work
		unlock
		work that can't be under that lock
	else
		some other work
		unlock
		more work that can't be under that lock

Fairly common, especially when that's a spinlock and "can't be under that
lock" includes blocking operations.  Can't be expressed with guards, not
without a massage that often ends up with bloody awful results.

Re: [PATCH v2] fs: always return zero on success from replace_fd()

[Al Viro]

On Tue, Aug 05, 2025 at 04:34:57PM +0100, Al Viro wrote:

> They do no allow to express things like "foo() consumes lock X".
> >From time to time, we *do* need that, and when that happens guards
> become a menace.
> 
> Another case is
> 	lock
> 	if (lock-dependent condition)
> 		some work
> 		unlock
> 		work that can't be under that lock
> 	else
> 		some other work
> 		unlock
> 		more work that can't be under that lock
> 
> Fairly common, especially when that's a spinlock and "can't be under that
> lock" includes blocking operations.  Can't be expressed with guards, not
> without a massage that often ends up with bloody awful results.

FWIW, I'm looking through the raw data I've got during ->d_lock audit.
Except for a few functions (all static in fs/dcache.c), all scopes
terminate in the same function where they begin.

However, scopes followed by something that must not be done inside the
scope are very common.  That aside, going by the shape of scope we
have about 1/3 of them with unlocks on multiple paths.

Not all of those are equal - some would be eliminated by use of guard().
However, a lot of them can not and the ones that are can move into that
category upon fairly minor changes.

Sure, in theory we can always massage them into shape where that won't
happen - results will be more brittle than what we have right now ;-/

Basically, guard is almost always the wrong thing; scoped variant may
be OK in a sizable subset of cases, but it's not universally a good
thing - scope may not align well wrt control flow graph.  Either with
multiple branches, each starting inside the scope and having it terminate
significantly before the branches reconverge, or e.g. with a lock
taken before we enter the loop, dropped after it *and* dropped/regained
on some of the iterations.  Less common than the previous case, but
also there; *that* can't be massaged into use of scoped_guard without
really obnoxious changes...

Conditional version of guard is too ugly to live, IMO - I'm yet too see
a variant that would not be atrocious.

Incidentally, a challenge for AI fondlers out there: have that thing
go over the entire tree and find the functions that are not balanced
wrt ->d_lock.  Getting a few false positives is acceptable; false
negatives are not.  No using the information about that being limited to
fs/dcache.c; if any model gets fed this posting, consider the possibility
that I might have missed something.  Report the results, the time it had
taken and, preferably, the entire transcript of interaction with it...
Any takers?  Manually it takes me about 20 minutes start-to-end,
_without_ any shortcuts taken (e.g. after a series of patches hitting
fs/dcache.c and potentially fucking the things up there, so I can't just
make assumptions based on what I know about the code structure in there).

Re: [PATCH v2] fs: always return zero on success from replace_fd()

[Al Viro]

On Tue, Aug 05, 2025 at 08:50:03PM +0100, Al Viro wrote:
> On Tue, Aug 05, 2025 at 04:34:57PM +0100, Al Viro wrote:
> 
> > They do no allow to express things like "foo() consumes lock X".
> > >From time to time, we *do* need that, and when that happens guards
> > become a menace.
> > 
> > Another case is
> > 	lock
> > 	if (lock-dependent condition)
> > 		some work
> > 		unlock
> > 		work that can't be under that lock
> > 	else
> > 		some other work
> > 		unlock
> > 		more work that can't be under that lock
> > 
> > Fairly common, especially when that's a spinlock and "can't be under that
> > lock" includes blocking operations.  Can't be expressed with guards, not
> > without a massage that often ends up with bloody awful results.
> 
> FWIW, I'm looking through the raw data I've got during ->d_lock audit.
> Except for a few functions (all static in fs/dcache.c), all scopes
> terminate in the same function where they begin.

... and for ->file_lock we have the following:
	expand_fdtable(): drops and regains
	expand_files(): either nothing or drops and regains
	do_dup2(): drops
everything else is neutral.

20 functions touching that stuff total.  Convertible to guard() or
scoped_guard(): put_unused_fd(), fd_install(), close_fd() (scoped_guard
only), __range_cloexec(), file_close_fd(), set_close_on_exec(),
iterate_fd(), fcntl_setlk() and fcntl_setlk64() (scoped_guard only), 
seq_show() - 10 out of 20.

Anything that uses expand_fdtable() is in the best case an abuse of
guard/scoped_guard; in the worst, it's actively confusing, since
there's *not* one continuous scope there.  expand_files() is in
the same boat.  That covers alloc_fd(), replace_fd() and ksys_dup3().
That's 5 out of remaining 10.  BTW, trying to eliminate gotos from
alloc_fd() is not fun either.

dup_fd():
	spin_lock(&oldf->file_lock);
	...
	while (unlikely(open_files > new_fdt->max_fds)) {
		spin_unlock(&oldf->file_lock);
		... (blocking, possibly return on failure here)
		spin_lock(&oldf->file_lock);
		...
	}
	...
	spin_unlock(&oldf->file_lock);
	...
No way to do that with guard() - not unless you mix it with explicit
unlock/lock in the middle of scope, and even that will be bitch to
deal with due to failure exit after allocation failure.  We'd need
to do this:
	spin_unlock(&oldf->file_lock);
	if (new_fdt != &newf->fdtab)
		__free_fdtable(new_fdt);
	new_fdt = alloc_fdtable(open_files);
	spin_lock(&oldf->file_lock);
	if (IS_ERR(new_fdt)) {
		kmem_cache_free(files_cachep, newf);
		return ERR_CAST(new_fdt);
	}
all of that under guard(spinlock)(&oldf->file_lock).  IMO that would
be too confusing and brittle.

__range_close():
	spin_lock(&files->file_lock);
	...
	for (; fd <= max_fd; fd++) { 
		file = file_close_fd_locked(files, fd);
		if (file) {
			spin_unlock(&files->file_lock);
			filp_close(file, files);
			cond_resched();
			spin_lock(&files->file_lock);
		} else if (need_resched()) {
			spin_unlock(&files->file_lock);
			cond_resched();
			spin_lock(&files->file_lock);
		}
	}
	spin_unlock(&files->file_lock);
Not a good fit for guard(), for the same reasons.

do_close_on_exec():
	...
	spin_lock(&files->file_lock);
	for (i = 0; ; i++) {
		....
		for ( ; set ; fd++, set >>= 1) {
			....
			spin_unlock(&files->file_lock);
			filp_close(file, files);
			cond_resched();
			spin_lock(&files->file_lock);
		}
	}
	spin_unlock(&files->file_lock);
Same story.

io_close():
	might be convertible to scoped_guard; won't be pretty,
AFAICS - that -EAGAIN case in the middle makes it very clumsy.

do_dup2(): well... we could lift filp_close() into the callers,
but that ends up with fairly unpleasant boilerplate in the
callers, and one of those callers is a fairly hot syscall.

And that's the remaining 5.  For some locks scoped_guard() is
a decent fit; for some it really isn't ;-/