On Thu, 2 Jul 2015 08:00:26 -0400
Jeff Layton <jlayton@xxxxxxxxxxxxxxx> wrote:
> On Thu, 2 Jul 2015 04:20:42 +0100
> Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> > 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.
>
> ISTM that pd_idpool_get ought to be using idr_alloc_cyclic instead.
> That should ensure that it's only allocating values from within the
> given range.
>
Erm...and why is it passing in '0' to idr_alloc for the end value if it
can't deal with more than 16 bits? That seems like a plain old bug...
The other stuff you've noted should also be fixed of course, but the
IDR usage here could use a little rework.
--
Jeff Layton <jlayton@xxxxxxxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
