Re: [PATCH v5 0/3] close_range()

[Christian Brauner]

On Tue, Jun 02, 2020 at 05:08:22PM -0700, Linus Torvalds wrote:
> On Tue, Jun 2, 2020 at 4:33 PM Christian Brauner
> <christian.brauner@xxxxxxxxxx> wrote:
> > >
> > > And maybe this _did_ get mentioned last time, and I just don't find
> > > it. I also don't see anything like that in the patches, although the
> > > flags argument is there.
> >
> > I spent some good time digging and I couldn't find this mentioned
> > anywhere so maybe it just never got sent to the list?
> 
> It's entirely possible that it was just a private musing, and you
> re-opening this issue just resurrected the thought.
> 
> I'm not sure how simple it would be to implement, but looking at it it
> shouldn't be problematic to add a "max_fd" argument to unshare_fd()
> and dup_fd().
> 
> Although the range for unsharing is obviously reversed, so I'd suggest
> not trying to make "dup_fd()" take the exact range into account.
> 
> More like just making __close_range() do basically something like
> 
>         rcu_read_lock();
>         cur_max = files_fdtable(files)->max_fds;
>         rcu_read_unlock();
> 
>         if (flags & CLOSE_RANGE_UNSHARE) {
>                 unsigned int max_unshare_fd = ~0u;
>                 if (cur_max >= max_fd)
>                         max_unshare_fd = fd;
>                 unshare_fd(max_unsgare_fd);
>         }
> 
>         .. do the rest of __close_range() here ..
> 
> and all that "max_unsgare_fd" would do would be to limit the top end
> of the file descriptor table unsharing: we'd still do the exact range
> handling in __close_range() itself.
> 
> Because teaching unshare_fd() and dup_fd() about anything more complex
> than the above doesn't sound worth it, but adding a way to just avoid
> the unnecessary copy of any high file descriptors sounds simple
> enough.

Ok, here's what I have. (I think in your example above cur_max and
max_fd are switched or I might have missed your point completely.) I was
a little in doubt whether capping dup_fd() between NR_OPEN_DEFAULT and
open_files was a sane thing to do but I think it is. Torture testing
this with a proper test-suite and with all debugging options enabled
didn't yet find any obvious issues. Does the below look somewhat sane?: