[PATCH] fcntl: make F_DUPFD_QUERY associative

[PATCH] fcntl: make F_DUPFD_QUERY associative

[Christian Brauner]

Currently when passing a closed file descriptor to
fcntl(fd, F_DUPFD_QUERY, fd_dup) the order matters:

    fd = open("/dev/null");
    fd_dup = dup(fd);

When we now close one of the file descriptors we get:

    (1) fcntl(fd, fd_dup) // -EBADF
    (2) fcntl(fd_dup, fd) // 0 aka not equal

depending on which file descriptor is passed first. That's not a huge
deal but it gives the api I slightly weird feel. Make it so that the
order doesn't matter by requiring that both file descriptors are valid:

(1') fcntl(fd, fd_dup) // -EBADF
(2') fcntl(fd_dup, fd) // -EBADF

Fixes: c62b758bae6a ("fcntl: add F_DUPFD_QUERY fcntl()")
Cc: <stable@vger.kernel.org>
Reported-by: Lennart Poettering <lennart@poettering.net>
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/fcntl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/fcntl.c b/fs/fcntl.c
index 22dd9dcce7ec..3d89de31066a 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -397,6 +397,9 @@ static long f_dupfd_query(int fd, struct file *filp)
 {
 	CLASS(fd_raw, f)(fd);
 
+	if (fd_empty(f))
+		return -EBADF;
+
 	/*
 	 * We can do the 'fdput()' immediately, as the only thing that
 	 * matters is the pointer value which isn't changed by the fdput.
-- 
2.45.2

Re: [PATCH] fcntl: make F_DUPFD_QUERY associative

[Lennart Poettering]

On Di, 08.10.24 13:30, Christian Brauner (brauner@kernel.org) wrote:

> Currently when passing a closed file descriptor to
> fcntl(fd, F_DUPFD_QUERY, fd_dup) the order matters:
>
>     fd = open("/dev/null");
>     fd_dup = dup(fd);
>
> When we now close one of the file descriptors we get:
>
>     (1) fcntl(fd, fd_dup) // -EBADF
>     (2) fcntl(fd_dup, fd) // 0 aka not equal
>
> depending on which file descriptor is passed first. That's not a huge
> deal but it gives the api I slightly weird feel. Make it so that the
> order doesn't matter by requiring that both file descriptors are valid:
>
> (1') fcntl(fd, fd_dup) // -EBADF
> (2') fcntl(fd_dup, fd) // -EBADF
>
> Fixes: c62b758bae6a ("fcntl: add F_DUPFD_QUERY fcntl()")
> Cc: <stable@vger.kernel.org>
> Reported-by: Lennart Poettering <lennart@poettering.net>
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
>  fs/fcntl.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index 22dd9dcce7ec..3d89de31066a 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -397,6 +397,9 @@ static long f_dupfd_query(int fd, struct file *filp)
>  {
>  	CLASS(fd_raw, f)(fd);
>
> +	if (fd_empty(f))
> +		return -EBADF;
> +
>  	/*
>  	 * We can do the 'fdput()' immediately, as the only thing that
>  	 * matters is the pointer value which isn't changed by the fdput.

Thanks! LGTM!

Reviewed-By: Lennart Poettering <lennart@poettering.net>

Lennart

--
Lennart Poettering, Berlin

Re: [PATCH] fcntl: make F_DUPFD_QUERY associative

[Jeff Layton]

On Tue, 2024-10-08 at 13:30 +0200, Christian Brauner wrote:
> Currently when passing a closed file descriptor to
> fcntl(fd, F_DUPFD_QUERY, fd_dup) the order matters:
> 
>     fd = open("/dev/null");
>     fd_dup = dup(fd);
> 
> When we now close one of the file descriptors we get:
> 
>     (1) fcntl(fd, fd_dup) // -EBADF
>     (2) fcntl(fd_dup, fd) // 0 aka not equal
> 
> depending on which file descriptor is passed first. That's not a huge
> deal but it gives the api I slightly weird feel. Make it so that the
> order doesn't matter by requiring that both file descriptors are valid:
> 
> (1') fcntl(fd, fd_dup) // -EBADF
> (2') fcntl(fd_dup, fd) // -EBADF
> 
> Fixes: c62b758bae6a ("fcntl: add F_DUPFD_QUERY fcntl()")
> Cc: <stable@vger.kernel.org>
> Reported-by: Lennart Poettering <lennart@poettering.net>
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
>  fs/fcntl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index 22dd9dcce7ec..3d89de31066a 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -397,6 +397,9 @@ static long f_dupfd_query(int fd, struct file *filp)
>  {
>  	CLASS(fd_raw, f)(fd);
>  
> +	if (fd_empty(f))
> +		return -EBADF;
> +
>  	/*
>  	 * We can do the 'fdput()' immediately, as the only thing that
>  	 * matters is the pointer value which isn't changed by the fdput.

Consistency is good, so:

    Reviewed-by: Jeff Layton <jlayton@kernel.org>

...that said, we should document that -EBADF means that at least one of
the fd's is bogus, but this API doesn't tell you which ones those are.
To figure that out, I guess you'd need to do something like issue
F_GETFD against each and see which ones return -EBADF?

Re: [PATCH] fcntl: make F_DUPFD_QUERY associative

[Christian Brauner]

On Tue, Oct 08, 2024 at 07:42:51AM GMT, Jeff Layton wrote:
> On Tue, 2024-10-08 at 13:30 +0200, Christian Brauner wrote:
> > Currently when passing a closed file descriptor to
> > fcntl(fd, F_DUPFD_QUERY, fd_dup) the order matters:
> > 
> >     fd = open("/dev/null");
> >     fd_dup = dup(fd);
> > 
> > When we now close one of the file descriptors we get:
> > 
> >     (1) fcntl(fd, fd_dup) // -EBADF
> >     (2) fcntl(fd_dup, fd) // 0 aka not equal
> > 
> > depending on which file descriptor is passed first. That's not a huge
> > deal but it gives the api I slightly weird feel. Make it so that the
> > order doesn't matter by requiring that both file descriptors are valid:
> > 
> > (1') fcntl(fd, fd_dup) // -EBADF
> > (2') fcntl(fd_dup, fd) // -EBADF
> > 
> > Fixes: c62b758bae6a ("fcntl: add F_DUPFD_QUERY fcntl()")
> > Cc: <stable@vger.kernel.org>
> > Reported-by: Lennart Poettering <lennart@poettering.net>
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > ---
> >  fs/fcntl.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/fs/fcntl.c b/fs/fcntl.c
> > index 22dd9dcce7ec..3d89de31066a 100644
> > --- a/fs/fcntl.c
> > +++ b/fs/fcntl.c
> > @@ -397,6 +397,9 @@ static long f_dupfd_query(int fd, struct file *filp)
> >  {
> >  	CLASS(fd_raw, f)(fd);
> >  
> > +	if (fd_empty(f))
> > +		return -EBADF;
> > +
> >  	/*
> >  	 * We can do the 'fdput()' immediately, as the only thing that
> >  	 * matters is the pointer value which isn't changed by the fdput.
> 
> Consistency is good, so:
> 
>     Reviewed-by: Jeff Layton <jlayton@kernel.org>
> 
> ...that said, we should document that -EBADF means that at least one of
> the fd's is bogus, but this API doesn't tell you which ones those are.
> To figure that out, I guess you'd need to do something like issue
> F_GETFD against each and see which ones return -EBADF?

It's actually worse because fcntl() can also give you EBADF if you have
an O_PATH file descriptor and you request an option that won't work on
an O_PATH file descriptor. It's complete nonsense.

So the most reliable way to figure out whether the fd is valid, is to
use a really really old fcntl() like idk F_GETFD and call it. Because
that should always work (ignoring really stupid things such as using
seccomp or an LSM to block F_GETFD) and if you get EBADF it must be
because the file descriptor isn't valid. Obviously that's racy if the
fdtable is shared but I don't think it's a big problem.

So if you get EBADF from F_DUPFD_QUERY and you really really need to
know whether the kernel supports it or any of the two fds was invalid
then yes, you need to follow this up with a F_GETFD. Again, racy but
won't matter most of the time.

Really, we should have returned something like EOPNOTSUPP from fcntl()
for the O_PATH case that would've meant that it's easy to detect new
flags.