Re: [git pull] vfs part 2

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

On Wed, Jul 01, 2015 at 07:44:08PM +0100, Al Viro wrote:
> Mismatched reply could also be a possibility, but only if we end up with
> sending more than one request with the same tag without waiting for response
> for the first one.

... and I think I see what's going on.  Tags are 16bit.  Suppose the
server stalls for some reason *and* we keep piling the requests up.
New tags keep being grabbed by this:

        tag = P9_NOTAG;
        if (type != P9_TVERSION) {
                tag = p9_idpool_get(c->tagpool);
                if (tag < 0)
                        return ERR_PTR(-ENOMEM);
        }
tag is int here.  Then we pass tag to
        req = p9_tag_alloc(c, tag, req_size);
and that's what sets req->tc->tag.  OK, but... The argument of p9_tag_alloc()
in u16, so after 2^16 pending requests we'll wrap around.  p9_idpool_get()
will happily return values greater than 65535 - it's using idr and it's
used (with different pools) for 16bit tags and 32bit FIDs.

Now, p9_tag_alloc(c, 65539, max_size) will return the same req we'd got from
p9_tag_alloc(c, 3, max_size).  And we are fucked - as far as the server is
concerned, we'd just sent another request with tag 3.  And on the client
there are two threads waiting for responses on the same p9_req_t.  Both
happen to be TWRITE.  Response to the first request arrives and we happen
to let the second thread go at it first.  Voila - the first request had
been for page-sized write() and got successfully handled.  The _second_ one
had been short and is very surprised to see confirmation of 4Kb worth of
data having been written.

It should be easy to confirm - in p9_client_prepare_req() add
		if (WARN_ON_ONCE(tag != (u16)tag)) {
			p9_idpool_put(tag, c->tagpool);
			return ERR_PTR(-ENOMEM);
		}
right after
                tag = p9_idpool_get(c->tagpool);
                if (tag < 0)
                        return ERR_PTR(-ENOMEM);

and see if it triggers.  I'm not sure if failing with ENOMEM is the
right response (another variant is to sleep there until the pile
gets cleaned or until we get killed), and WARN_ON_ONCE() is definitely
not for the real work, but it will do for confirming that this is what
we are hitting.