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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <52D67677.4050407@citrix.com>
Date:	Wed, 15 Jan 2014 11:52:23 +0000
From:	David Vrabel <david.vrabel@...rix.com>
To:	Wei Liu <wei.liu2@...rix.com>
CC:	Annie Li <Annie.li@...cle.com>, <xen-devel@...ts.xen.org>,
	<netdev@...r.kernel.org>, <ian.campbell@...rix.com>
Subject: Re: [Xen-devel] [PATCH net-next] xen-netfront: clean up code in xennet_release_rx_bufs

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@...cle.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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html