| Message ID | 20241008-duften-formel-251f967602d5@brauner (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fcntl: make F_DUPFD_QUERY associative | expand |
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
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?
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.
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.
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(+)