Re: [Xen-devel] [PATCH net-next] xen-netfront: clean up code in xennet_release_rx_bufs

[David Vrabel <david.vrabel@citrix.com>]

On 15/01/14 11:42, Wei Liu wrote:
> On Wed, Jan 15, 2014 at 11:20:49AM +0000, David Vrabel wrote:
>> On 09/01/14 22:48, Annie Li wrote:
>>> Current netfront only grants pages for grant copy, not for grant transfer, so
>>> remove corresponding transfer code and add receiving copy code in
>>> xennet_release_rx_bufs.
>>
>> While netfront only supports a copying backend, I don't see anything
>> preventing the backend from retaining mappings to netfront's Rx buffers...
>>
> 
> Correct.
> 
>>> Signed-off-by: Annie Li <Annie.li@oracle.com>
>>> ---
>>>  drivers/net/xen-netfront.c |   60 ++-----------------------------------------
>>>  1 files changed, 3 insertions(+), 57 deletions(-)
>>>
>>> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
>>> index e59acb1..692589e 100644
>>> --- a/drivers/net/xen-netfront.c
>>> +++ b/drivers/net/xen-netfront.c
>>> @@ -1134,78 +1134,24 @@ static void xennet_release_tx_bufs(struct netfront_info *np)
>>>  
>>>  static void xennet_release_rx_bufs(struct netfront_info *np)
>>>  {
>> [...]
>>> -		mfn = gnttab_end_foreign_transfer_ref(ref);
>>> +		gnttab_end_foreign_access_ref(ref, 0);
>>
>> ... the gnttab_end_foreign_access_ref() may then fail and...
>>
> 
> Oh, I see. Andrew was actually referencing this function. Yes, it can
> fail. Since he omitted "_ref" I looked at the other function when I
> replied to him...
> 
>>>  		gnttab_release_grant_reference(&np->gref_rx_head, ref);
>>>  		np->grant_rx_ref[id] = GRANT_INVALID_REF;
>> [...]
>>> +		kfree_skb(skb);
>>
>> ... this could then potentially free pages that the backend still has
>> mapped.  If the pages are then reused, this would leak information to
>> the backend.
>>
>> Since only a buggy backend would result in this, leaking the skbs and
>> grant refs would be acceptable here.  I would also print an error.
>>
> 
> How about using gnttab_end_foreign_access. The deferred queue looks like
> a right solution -- pending page won't get freed until gref is
> quiescent.

This is more like the correct approach but I don't think it still quite
right.  The skb owns the pages so we don't want
gnttab_end_foreign_access() to free them as freeing the skb will attempt
to free them again.

Having gnttab_end_foreign_access() do a free just looks odd to me, the
free isn't paired with any alloc in the grant table code.

It seems more logical to me that granting access takes an additional
page ref, and then ending access releases that ref.

David