[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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




On 2014-1-15 19:52, David Vrabel wrote:
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@xxxxxxxxxx>
---
  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.

I am thinking of two ways, and they can be implemented in new patches.
1. If gnttab_end_foreign_access_ref succeeds, then kfree_skb is called to free skb. Otherwise, using gnttab_end_foreign_access to release ref and pages. 2. Add a similar deferred way of gnttab_end_foreign_access in gnttab_end_foreign_access_ref.

Thanks
Annie



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.