Re: pipe_release oops.

[Al Viro <viro@ZenIV.linux.org.uk>]

On Tue, Mar 12, 2013 at 01:09:16PM -0700, Linus Torvalds wrote:
> On Tue, Mar 12, 2013 at 12:43 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > Umm...  How about the following, then?  I think it makes the whole thing
> > simpler and saner...  NOTE: this got only a light beating and we'd
> > just seen an example of long-standing breakage in that area; I'd really
> > like to see it tortured by Dave's scripts before it gets merged into
> > mainline.
> 
> Looks ok to me.
> 
> But it's very hard to see the changes when they are joined by code
> movement, so either I'd almost like to see it split into two ("pure
> movement" followed by "clean up"),

FWIW, that delta came from git diff HEAD^^ ;-)  Internally, it's a couple
of commits; see vfs.git#pipe for details.

> or I'd like to feel a lot safer by
> having somebody beat on named pipes with some app that actually uses
> them.

Definitely.  Well, one such app is sysvinit, so aforementioned light beating
did include that to some extent, but it certainly needs more testing.

> They are rather seldom used, it's easy to break them and not
> even notice. For example, we have that whole "r/w_counter" logic that
> is subtle (and mis-documented, I notice). It's not a "count of
> readers/writers", it's a "*sequence* count of readers/writers having
> come in", and it's needed for the whole "oh, we're waiting for a
> writer, and one came in, but disappeared before we noticed, but we can
> see that it was there from the sequence number".

... and moreover, it's interacting with poll()/select().  I _think_ it's
OK (note that we only step into that thing when we open an extra descriptor
via /proc/<pid>/fd/<pipe_fd>), but...

> So the whole FIFO code is trivial from the standpoint of sharing all
> the IO code with pipes, but it's nontrivial in having some very
> specific semantics at open time, and it's seldom actually used, and
> easy to get wrong.
> 
> So anything like this needs to be either "obviously no semantic
> changes", or needs some real fifo testing.

Agreed.