Re: [PATCH] fs: clear close-on-exec flag as part of put_unused_fd()

[Yann Droneaud <ydroneaud@opteya.com>]

Hi,

Le mercredi 11 décembre 2013 à 23:30 +0000, Al Viro a écrit :
> On Wed, Dec 11, 2013 at 11:36:35PM +0100, Mateusz Guzik wrote:
> 
> > >From my reading this will break at least the following:
> > fd = open(..., .. | O_CLOEXEC);
> > dup2(whatever, fd);
> > 
> > now fd has O_CLOEXEC even though it should not
> 
> Moreover, consider fork() done by a thread that shares descriptor
> table with somebody else.  Suppose it happens in the middle of
> open() with O_CLOEXEC being done by another thread.  We copy descriptor
> table after descriptor had been reserved (and marked close-on-exec),
> but before a reference to struct file has actually been inserted there.
> This code
>         for (i = open_files; i != 0; i--) {
>                 struct file *f = *old_fds++;
>                 if (f) {
>                         get_file(f);
>                 } else {
>                         /*    
>                          * The fd may be claimed in the fd bitmap but not yet
>                          * instantiated in the files array if a sibling thread
>                          * is partway through open().  So make sure that this
>                          * fd is available to the new process.
>                          */
>                         __clear_open_fd(open_files - i, new_fdt);
>                 }
>                 rcu_assign_pointer(*new_fds++, f);
>         }
>         spin_unlock(&oldf->file_lock);
> in dup_fd() will clear the corresponding bit in open_fds, leaving close_on_exec
> alone.  Currently that's fine (we will override whatever had been in
> close_on_exec when we reserve that descriptor again), but AFAICS with this
> patch it will break.
> 

That's a terrible subtle case. it will indeed break with the patch.

> Sure, it can be fixed up (ditto with dup2(), etc.), but what's the point?

It was only an attempt at making close-on-exec handling "simpler".

> Result will require more subtle reasoning to prove correctness and will
> be more prone to breakage.  Does that really yield visible performance
> improvements that would be worth the extra complexity?  After all, you
> trade some writes to close_on_exec on descriptor reservation for unconditional
> write on descriptor freeing; if anything, I would expect that you'll get
> minor _loss_ from that change, assuming they'll be measurable in the first
> place...

Since it's not so straightforward to get it correct, and the only
advantage I was trying to address is aesthetic, I will discard it.

Thanks a lot for the review and the comments.

Regards.

-- 
Yann Droneaud
OPTEYA