Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 16 Mar 2020 18:35:18 +0000
From:   Al Viro <viro@zeniv.linux.org.uk>
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Ling Ma <ling.ma.program@gmail.com>, linux-kernel@vger.kernel.org,
        Ingo Molnar <mingo@kernel.org>, "ling.ma" <ling.ml@antfin.com>
Subject: Re: [RFC PATCH] locks:Remove spinlock in unshare_files
Message-ID: <20200316183518.GZ23230@ZenIV.linux.org.uk>
References: <20200313031017.71090-1-ling.ma.program@gmail.com>
 <CAOGi=dNniSgkUtJPfaYLAbR+_8DMFdU59mx7hf8Otj_VjDF_dQ@mail.gmail.com>
 <20200316133916.GD12561@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200316133916.GD12561@hirez.programming.kicks-ass.net>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Mar 16, 2020 at 02:39:16PM +0100, Peter Zijlstra wrote:

> > > diff --git a/kernel/fork.c b/kernel/fork.c
> > > index 60a1295..fe54600 100644
> > > --- a/kernel/fork.c
> > > +++ b/kernel/fork.c
> > > @@ -3041,9 +3041,7 @@ int unshare_files(struct files_struct **displaced)
> > >                 return error;
> > >         }
> > >         *displaced = task->files;
> > > -       task_lock(task);
> > > -       task->files = copy;
> > > -       task_unlock(task);
> > > +       WRITE_ONCE(task->files, copy);
> > >         return 0;
> > >  }
> 
> AFAICT this is completely and utterly buggered.
> 
> IFF task->files was lockless, like say RCU, then you'd still need
> smp_store_release(). But if we look at fs/file.c then everything uses
> task_lock() and removing it like the above is actively broken.

The problem is not fs/file.c; it's the code that does (read-only)
access to *other* threads' ->files.  procfs, SAK, some cgroup
shite (pardon the redundancy)...  All of those rely upon task_lock.

FWIW, having just grepped around, I'm worried about the crap io_uring
is pulling off - interplay with unshare(2) could be unpleasant.

In any case - task_lock in the code that assigns to ->files (and it's
not just unshare_files()) serves to protect the 3rd-party readers
(including get_files_struct()) from having the fucker taken apart
under them.  It's not just freeing the thing - it's the entire
close_files().

And no, we do *NOT* want to convert everything to get_files_struct() +
being clever in it.  I would rather have get_files_struct() taken
out and shot, TBH - the only real reason it hadn't been killed years
ago is the loop in proc_readfd_common()...

I'd prefer to have 3rd-party readers indicate their interest
in a way that would be distinguishable from normal references,
with close_files() waiting until all of those are gone.  One way
to do that would be
	* secondary counter in files_struct
	* rcu-delayed freeing of actual structure (not a problem)
	* rcu_read_lock in 3rd-party readers (among other things
it means that proc_readfd_common() would need to be rearchitected
a bit)
	* close_files() starting with subtraction of large constant
from the secondary counter and then spinning until it gets to
-<large constant>
	* 3rd-party readers (under rcu_read_lock()) fetching task->files,
bumping the secondary counter unless it's negative, doing their thing,
then decrementing the counter.