lkml.org 
[lkml]   [2015]   [Jul]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [git pull] vfs part 2
    On Thu, 2 Jul 2015 04:20:42 +0100
    Al Viro <viro@ZenIV.linux.org.uk> 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.

    --
    Jeff Layton <jlayton@poochiereds.net>


    \
     
     \ /
      Last update: 2015-07-02 14:41    [W:3.959 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site