Re: [Xen-devel] [PATCH RFC] xen/public/ring.h: simplify RING_HAS_UNCONSUMED_REQUESTS()

[Tim Deegan <tim@xxxxxxx>]

At 14:18 +0000 on 22 Mar (1395494283), Zoltan Kiss wrote:
> Hi,
> 
> I think I might have an explanation why do we need this, see this mailing:
> 
> https://lkml.org/lkml/2014/3/20/710
> https://lkml.org/lkml/2014/3/21/111
> https://lkml.org/lkml/2014/3/21/390

Quoting from the third of these:

| But consuming overrunning requests after rsp_prod_pvt is a problem:
| - NAPI instance races with dealloc thread over the slots. The first 
| reads them as requests, the second writes them as responses
| - the NAPI instance overwrites used pending slots as well, so skb frag 
| release go wrong etc.

OK, so the backend needs to be careful not to follow the frontend into
overrun, not because of the ring itself being corrupted but because it
will mess up the backend's internal bookkeeping.

Fair enough -- I think this at least constitutes a reason why it might
be dangerous to remove the check from the public header.  I withdraw
the patch.  I'll make another patch (with a smaller distribution
list!) to add an appropriate comment.

Thanks!

Tim.

> On 13/03/14 16:38, Tim Deegan wrote:
> > [RFC] xen/public/ring.h: simplify RING_HAS_UNCONSUMED_REQUESTS()
> >
> > Remove a useless (though harmless) extra check.
> >
> > Code inspection
> > ---------------
> >
> > Ian Campbell and I looked at this today and can't find any case where
> > the existing 'rsp' test would be useful.  In today's ring.h,
> >
> > #define RING_HAS_UNCONSUMED_REQUESTS(_r) ({                             \
> >      unsigned int req = (_r)->sring->req_prod - (_r)->req_cons;          \
> >      unsigned int rsp = RING_SIZE(_r) -                                  \
> >          ((_r)->req_cons - (_r)->rsp_prod_pvt);                          \
> >      req < rsp ? req : rsp;                                              \
> > })
> >
> > 'req' is the number of requests that the F/E has published and we have
> > not yet consumed.  'rsp' is an odd fish, everything _except_ what we
> > might call requests locally in flight, that is requests we've consumed
> > but not produced a response for.  We could only think of two things we
> > might be trying to test for here:
> >
> > (a) req_cons runs ahead of rsp_prod_pvt, as would be expected from the
> >      way these rings normally work.  In that case, as Zoltan pointed
> >      out, rsp must be >= req, for a well-behaved frontend: otherwise
> >      we'd have req_prod > rsp_prod_pvt + RING_SIZE, implying that
> >      req_prod > rsp_cons + RING_SIZE, i.e. the frontend has overrun
> >      the ring.  I don't think this even usefully protects against a
> >      malicious/buggy frontend.
> >
> > (b) rsp_prod_pvt runs ahead of req_cons, which seems wrong but I'm
> >      told might happen in linux netback?  In that case, we might plausibly
> >      want to try to limit RING_HAS_UNCONSUMED_REQUESTS to max of
> >      (req_prod - req_cons) and (req_prod - rsp_prod_pvt), but that's
> >      not what this does.  Instead, rsp will underflow to
> >      RING_SIZE + (rsp_prod_pvt - req_cons), which will always be >= req.
> >
> > So in both of those cases, 'rsp' is always >= 'req' and is useless.
> >
> >
> > Code archaeology
> > ----------------
> >
> > In the original ring.h, the test was a boolean, as the name implies.
> > Cset 99812f40 ([NET] back: Add SG support) extended it to a count in
> > the obvious manner.  Looking at the original (0b788798):
> >
> > #define RING_HAS_UNCONSUMED_REQUESTS(_p, _r)                            \
> >     ( ((_r)->req_cons != (_r)->sring->req_prod ) &&                      \
> >       (((_r)->req_cons - (_r)->rsp_prod_pvt) !=                          \
> >        SRING_SIZE((_p), (_r)->sring)) )
> >
> > it seems to be testing for 'there are unconsumed requests _and_ we
> > have not got a full ring of consumed-but-not-responded requests'.
> > This also seems to be effectively harmless: if there are unconsumed
> > requests, we can't possibly have a ring full of c-b-n-r requests
> > unless the frontend's request producer has overrun its response
> > consumer.
> >
> > That code was introduced with no callers, but seems to have been
> > copied from the existing netback code, which switched to use it in
> > b242b208.  All later users of it in the xenolinux trees are either
> > brand new code or replacing a simple (req_cons - req_prod) test.  The
> > netback code goes back to Keir's original implementation, where it
> > looked like this, in inverted form (1de448f4):
> >
> >          /* Work to do? */
> >          i = netif->tx_req_cons;
> >          if ( (i == netif->tx->req_prod) ||
> >               ((i-netif->tx_resp_prod) == NETIF_TX_RING_SIZE) )
> >          {
> >              netif_put(netif);
> >              continue;
> >          }
> >
> > Again, I don't think this test is useful: it's only interesting if
> > netfront has overrun the ring, but it doesn't reliably detect that.
> >
> > So let's remove it.
> >
> > Suggested-by: Zoltan Kiss <zoltan.kiss@xxxxxxxxxx>
> > Signed-off-by: Tim Deegan <tim@xxxxxxx>
> > Cc: Keir Fraser <keir@xxxxxxx>
> > Cc: Jan Beulich <JBeulich@xxxxxxxx>
> > Cc: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
> > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > Cc: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> > Cc: David Vrabel <david.vrabel@xxxxxxxxxx>
> > Cc: Alan Somers <alans@xxxxxxxxxxxxxxxx>
> > Cc: John Suykerbuyk <johns@xxxxxxxxxxxxxxxx>
> > Cc: Manuel Bouyer <bouyer@xxxxxxxxxx>
> > Cc: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> > Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> >
> > ---
> > RFC because
> > - I might well be missing something here; and
> > - this is a change to a public header that could be in use in any
> >    number of implementations; since the extra test is very cheap, and
> >    seems to be harmess, we might consider leaving it in place.
> > - I haven't tested it at all yet.
> >
> > CC'ing a whole bunch of people whose code might be using this macro.
> >
> > diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h
> > index 73e13d7..d6e23f1 100644
> > --- a/xen/include/public/io/ring.h
> > +++ b/xen/include/public/io/ring.h
> > @@ -192,21 +192,8 @@ typedef struct __name##_back_ring __name##_back_ring_t
> >   #define RING_HAS_UNCONSUMED_RESPONSES(_r)                               \
> >       ((_r)->sring->rsp_prod - (_r)->rsp_cons)
> >
> > -#ifdef __GNUC__
> > -#define RING_HAS_UNCONSUMED_REQUESTS(_r) ({                             \
> > -    unsigned int req = (_r)->sring->req_prod - (_r)->req_cons;          \
> > -    unsigned int rsp = RING_SIZE(_r) -                                  \
> > -        ((_r)->req_cons - (_r)->rsp_prod_pvt);                          \
> > -    req < rsp ? req : rsp;                                              \
> > -})
> > -#else
> > -/* Same as above, but without the nice GCC ({ ... }) syntax. */
> >   #define RING_HAS_UNCONSUMED_REQUESTS(_r)                                \
> > -    ((((_r)->sring->req_prod - (_r)->req_cons) <                        \
> > -      (RING_SIZE(_r) - ((_r)->req_cons - (_r)->rsp_prod_pvt))) ?        \
> > -     ((_r)->sring->req_prod - (_r)->req_cons) :                         \
> > -     (RING_SIZE(_r) - ((_r)->req_cons - (_r)->rsp_prod_pvt)))
> > -#endif
> > +    ((_r)->sring->req_prod - (_r)->req_cons)
> >
> >   /* Direct access to individual ring elements, by index. */
> >   #define RING_GET_REQUEST(_r, _idx)                                      \
> >
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel