Re: [PATCH v3 10/11] make __set_open_fd() set cloexec state as well

[Christian Brauner <brauner@kernel.org>]

On Wed, Oct 09, 2024 at 11:50:36AM GMT, Steven Price wrote:
> On 07/10/2024 18:43, Al Viro wrote:
> > ->close_on_exec[] state is maintained only for opened descriptors;
> > as the result, anything that marks a descriptor opened has to
> > set its cloexec state explicitly.
> > 
> > As the result, all calls of __set_open_fd() are followed by
> > __set_close_on_exec(); might as well fold it into __set_open_fd()
> > so that cloexec state is defined as soon as the descriptor is
> > marked opened.
> > 
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > ---
> >  fs/file.c | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/file.c b/fs/file.c
> > index d8fccd4796a9..b63294ed85ec 100644
> > --- a/fs/file.c
> > +++ b/fs/file.c
> > @@ -248,12 +248,13 @@ static inline void __set_close_on_exec(unsigned int fd, struct fdtable *fdt,
> >  	}
> >  }
> >  
> > -static inline void __set_open_fd(unsigned int fd, struct fdtable *fdt)
> > +static inline void __set_open_fd(unsigned int fd, struct fdtable *fdt, bool set)
> >  {
> >  	__set_bit(fd, fdt->open_fds);
> >  	fd /= BITS_PER_LONG;
> 
> Here fd is being modified...
> 
> >  	if (!~fdt->open_fds[fd])
> >  		__set_bit(fd, fdt->full_fds_bits);
> > +	__set_close_on_exec(fd, fdt, set);
> 
> ... which means fd here isn't the same as the passed in value. So this
> call to __set_close_on_exec affects a different fd to the expected one.

Good spot. Al, I folded the below fix so from my end you don't have to
do anything unless you want to nitpick how to fix it. Local variable
looked ugly to me.

Pushed and running through tests now.

diff --git a/fs/file.c b/fs/file.c
index c039e21c1cd1..52654415f17e 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -354,12 +354,17 @@ static inline void __set_close_on_exec(unsigned int fd, struct fdtable *fdt,
        }
 }

-static inline void __set_open_fd(unsigned int fd, struct fdtable *fdt, bool set)
+static inline void __set_open(unsigned int fd, struct fdtable *fdt, bool set)
 {
        __set_bit(fd, fdt->open_fds);
        fd /= BITS_PER_LONG;
        if (!~fdt->open_fds[fd])
                __set_bit(fd, fdt->full_fds_bits);
+}
+
+static inline void __set_open_fd(unsigned int fd, struct fdtable *fdt, bool set)
+{
+       __set_open(fd, fdt, set);
        __set_close_on_exec(fd, fdt, set);
 }