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

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

On 24/03/14 13:52, Paul Durrant wrote:
>> -----Original Message-----
>> From: Zoltan Kiss
>> Sent: 24 March 2014 12:24
>> To: Jan Beulich; Zoltan Kiss; Tim (Xen.org)
>> Cc: Wei Liu; Ian Campbell; Stefano Stabellini; Keir (Xen.org); freebsd-
>> xen@freebsd.org; Alan Somers; Paul Durrant; David Vrabel; xen-
>> devel@lists.xenproject.org; Boris Ostrovsky; John Suykerbuyk; Manuel
>> Bouyer; Roger Pau Monne
>> Subject: Re: [Xen-devel] [PATCH RFC] xen/public/ring.h: simplify
>> RING_HAS_UNCONSUMED_REQUESTS()
>>
>> On 24/03/14 07:38, Jan Beulich wrote:
>>>>>> On 22.03.14 at 18:14, <tim@xen.org> wrote:
>>>> At 14:18 +0000 on 22 Mar (1395494283), Zoltan Kiss wrote:
>>>>> 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.
>>> With s/will/may/ I'm not sure that's a reason to withdraw the patch:
>>> The generic macros in ring.h imo shouldn't dictate any particular
>>> protection policy beyond protecting the ring itself. I.e. I'd think if
>>> netback need protection beyond the one provided by ring.h macros,
>>> it should take care to implement them itself.
>> It's not "may", it is a "will". In case of Linux netback for sure, but I
>> think it's reasonable for any backend to rely on the fact that the ring
>> macros protect them from abusive frontends. Protecting a backend to read
>> requests from the [rsp_prod_pvt, req_cons] range is a sensible thing to
>> do in the ring macros.
> I disagree. That's not what the name of the macro implies; it implies a range for req_prod - req_cons. The backend is responsible for its own rsp_prod_pvt value and should make sure it's safe. If, to that end, it wants to have its own variant of the macro then that's reasonable, but adding such a clause to the macro in the generic header is (as has been proven) confusing.
My opinion is that the name of the macro implies the caller wants to 
know if there are unconsumed requests on the ring (and it doesn't imply 
it, but the return value is actually how many unconsumed requests you 
have). Returning req_prod - req_cons seems to be logical at first (and 
second and ...) thought, but - and that's not implied in the name 
indeed, but I think it's sensible, and should be documented in a comment 
- if you want to protect the backend from abusive frontends, you should 
check if req_prod is not overrunning rsp_prod_pvt. You could check 
rsp_prod, but rsp_prod_pvt is better, because the latter should be 
always equal to or ahead of the former, and from the backend point of 
view the responses between the two (if any) doesn't matter.
It's not about the backend making sure it's rsp_prod_pvt is valid or 
not, it's about protection from a frontend. And I think it should be a 
general thing for the ring users to have this safety check, rather than 
something backend-specific, as all of them could be affected.

Zoli
>
>    Paul
>
>> Also, RING_FINAL_CHECK_FOR_REQUESTS relies on this macro, removing
>> this
>> protection may cause other issues, e.g. netback keeps the NAPI instance
>> spinning while it's not consuming any requests.
>>
>> Zoli