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

[annie li <annie.li@xxxxxxxxxx>]

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