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

[Zoltan Kiss <zoltan.kiss@citrix.com>]

Paul mentioned the lkml links doesn't work at the moment, but a short 
summary of why rsp is important: it gives us the maximum possible amount 
of requests should be on the ring (number of slots which are not 
consumed-but-not-responded), so even if the frontend lies to us through 
increasing req_prod to crazy values, backend can avoid consuming slots 
which are still consumed-but-not-responded.
Here is my explanation from that another thread:

"The backend doesn't see what the guest does with the responses, and
that's OK, it's the guest's problem, after netback increased
rsp_prod_pvt it doesn't really care. But as soon as the guest start
placing new requests after rsp_prod_pvt, or just increasing req_prod so
req_prod-rsp_prod_pvt > XEN_NETIF_TX_RING_SIZE, it becomes an issue.
So far this xenvif_tx_pending_slots_available indirectly saved us from
consuming requests overwriting still pending requests, but the guest
could still overwrote our responses. But again, that's still the guests
problem, we had the original request saved in the pending ring data. If
the guest went too far, build_gops killed the vif when req_prod-req_cons 
 > XEN_NETIF_TX_RING_SIZE
Indirect above means it only happened because the pending ring had the
same size, and the used slots has a 1-to-1 mapping for slots between
rsp_prod_pvt and req_cons. So xenvif_tx_pending_slots_available also
means (req_cons - rsp_prod_pvt) + XEN_NETBK_LEGACY_SLOTS_MAX <
XEN_NETIF_TX_RING_SIZE (does this look familiar?
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.
But the current RING_HAS_UNCONSUMED_REQUESTS fortunately saves us here,
let me explain it through an example:
rsp_prod_pvt = 0
req_cons = 253
req_prod = 258

Therefore:
req = 5
rsp = 3

So in xenvif_tx_build_gops work_to_do will be 3, and in
xenvif_count_requests we bail out when we see that the packet actually
exceeds that.
So in the end, we are safe here, but we shouldn't change that macro I
suggested to refactor"

On 03/04/14 10:38, Tim Deegan wrote:
> [CC's clipped]
>
> At 17:38 +0100 on 13 Mar (1394728686), Tim Deegan wrote:
>> [RFC] xen/public/ring.h: simplify RING_HAS_UNCONSUMED_REQUESTS()
>>
>> Remove a useless (though harmless) extra check.
>
> What shall we do about this?  AFAICT, Jan and Paul are in favour of the
> change; Zoltan is against it.  I am wavering.  Keir, any opinion?
>
> Tim.
>
>> 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@citrix.com>
>> Signed-off-by: Tim Deegan <tim@xen.org>
>> Cc: Keir Fraser <keir@xen.org>
>> Cc: Jan Beulich <JBeulich@suse.com>
>> Cc: Ian Campbell <Ian.Campbell@citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Cc: David Vrabel <david.vrabel@citrix.com>
>> Cc: Alan Somers <alans@spectralogic.com>
>> Cc: John Suykerbuyk <johns@spectralogic.com>
>> Cc: Manuel Bouyer <bouyer@NetBSD.org>
>> Cc: Paul Durrant <Paul.Durrant@citrix.com>
>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>
>> ---
>> 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@lists.xen.org
>> http://lists.xen.org/xen-devel
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>