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

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

[Annie Li]

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.

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)
 {
-	struct mmu_update      *mmu = np->rx_mmu;
-	struct multicall_entry *mcl = np->rx_mcl;
-	struct sk_buff_head free_list;
 	struct sk_buff *skb;
-	unsigned long mfn;
-	int xfer = 0, noxfer = 0, unused = 0;
 	int id, ref;
 
-	dev_warn(&np->netdev->dev, "%s: fix me for copying receiver.\n",
-			 __func__);
-	return;
-
-	skb_queue_head_init(&free_list);
-
 	spin_lock_bh(&np->rx_lock);
 
 	for (id = 0; id < NET_RX_RING_SIZE; id++) {
 		ref = np->grant_rx_ref[id];
-		if (ref == GRANT_INVALID_REF) {
-			unused++;
+		if (ref == GRANT_INVALID_REF)
 			continue;
-		}
 
 		skb = np->rx_skbs[id];
-		mfn = gnttab_end_foreign_transfer_ref(ref);
+		gnttab_end_foreign_access_ref(ref, 0);
 		gnttab_release_grant_reference(&np->gref_rx_head, ref);
 		np->grant_rx_ref[id] = GRANT_INVALID_REF;
 
-		if (0 == mfn) {
-			skb_shinfo(skb)->nr_frags = 0;
-			dev_kfree_skb(skb);
-			noxfer++;
-			continue;
-		}
-
-		if (!xen_feature(XENFEAT_auto_translated_physmap)) {
-			/* Remap the page. */
-			const struct page *page =
-				skb_frag_page(&skb_shinfo(skb)->frags[0]);
-			unsigned long pfn = page_to_pfn(page);
-			void *vaddr = page_address(page);
-
-			MULTI_update_va_mapping(mcl, (unsigned long)vaddr,
-						mfn_pte(mfn, PAGE_KERNEL),
-						0);
-			mcl++;
-			mmu->ptr = ((u64)mfn << PAGE_SHIFT)
-				| MMU_MACHPHYS_UPDATE;
-			mmu->val = pfn;
-			mmu++;
-
-			set_phys_to_machine(pfn, mfn);
-		}
-		__skb_queue_tail(&free_list, skb);
-		xfer++;
-	}
-
-	dev_info(&np->netdev->dev, "%s: %d xfer, %d noxfer, %d unused\n",
-		 __func__, xfer, noxfer, unused);
-
-	if (xfer) {
-		if (!xen_feature(XENFEAT_auto_translated_physmap)) {
-			/* Do all the remapping work and M2P updates. */
-			MULTI_mmu_update(mcl, np->rx_mmu, mmu - np->rx_mmu,
-					 NULL, DOMID_SELF);
-			mcl++;
-			HYPERVISOR_multicall(np->rx_mcl, mcl - np->rx_mcl);
-		}
+		kfree_skb(skb);
 	}
 
-	__skb_queue_purge(&free_list);
-
 	spin_unlock_bh(&np->rx_lock);
 }
 
-- 
1.7.6.5

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

[David Miller]

From: Annie Li <Annie.li@oracle.com>
Date: Fri, 10 Jan 2014 06:48:38 +0800

> 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.
> 
> Signed-off-by: Annie Li <Annie.li@oracle.com>

If a Xen networking driver would review this I'd appreciate it.

Thanks.

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

[annie li]

On 2014/1/15 7:28, David Miller wrote:
> From: Annie Li <Annie.li@oracle.com>
> Date: Fri, 10 Jan 2014 06:48:38 +0800
>
>> 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.
>>
>> Signed-off-by: Annie Li <Annie.li@oracle.com>
> If a Xen networking driver would review this I'd appreciate it.
>

I will re-post a new patch with improved comments. This patch mainly 
fixes leaking grant issue in netfront, and improvement on 
gnttab_end_foreign_access_ref and gnttab_end_foreign_access can be 
implemented in a separate patch.

Thanks
Annie

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

[Wei Liu]

On Fri, Jan 10, 2014 at 06:48:38AM +0800, 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.
> 

This path seldom gets call -- not that many people unload xen-netfront
driver. If Annie has tested this patch and it works as expected I think
it's fine.

I'm not netfront maintainer but I'm happy to add
Acked-by: Wei Liu <wei.liu2@citrix.com>
if Annie confirms she's tested this patch.

Wei.

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

[Andrew Bennieston]

On 15/01/14 10:07, Wei Liu wrote:
> On Fri, Jan 10, 2014 at 06:48:38AM +0800, 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.
>>
>
> This path seldom gets call -- not that many people unload xen-netfront
> driver. If Annie has tested this patch and it works as expected I think
> it's fine.
>
In XenServer we have seen a number of cases where unplugging and 
replugging VIFs results in leakage of grant references, eventually 
leading to a case where you cannot plug a VIF (after ~ 400 such cycles)...

It's worth pointing out, as far as this patch is concerned, that 
gnttab_end_foreign_access() can fail, which is not taken into account here.

Andrew.

> I'm not netfront maintainer but I'm happy to add
> Acked-by: Wei Liu <wei.liu2@citrix.com>
> if Annie confirms she's tested this patch.
>
> Wei.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

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

[Wei Liu]

On Wed, Jan 15, 2014 at 11:02:55AM +0000, Andrew Bennieston wrote:
> On 15/01/14 10:07, Wei Liu wrote:
> >On Fri, Jan 10, 2014 at 06:48:38AM +0800, 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.
> >>
> >
> >This path seldom gets call -- not that many people unload xen-netfront
> >driver. If Annie has tested this patch and it works as expected I think
> >it's fine.
> >
> In XenServer we have seen a number of cases where unplugging and
> replugging VIFs results in leakage of grant references, eventually
> leading to a case where you cannot plug a VIF (after ~ 400 such
> cycles)...
> 

OK, this makes sense.

> It's worth pointing out, as far as this patch is concerned, that
> gnttab_end_foreign_access() can fail, which is not taken into
> account here.
> 

How? gnttab_end_foreign_access doesn't return any error. The gref which
cannot be freed right away will be added to a deferred list and handle
later.

Wei.

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

[annie li]

On 2014-1-15 19:02, Andrew Bennieston wrote:
> On 15/01/14 10:07, Wei Liu wrote:
>> On Fri, Jan 10, 2014 at 06:48:38AM +0800, 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.
>>>
>>
>> This path seldom gets call -- not that many people unload xen-netfront
>> driver. If Annie has tested this patch and it works as expected I think
>> it's fine.
>>
> In XenServer we have seen a number of cases where unplugging and 
> replugging VIFs results in leakage of grant references, eventually 
> leading to a case where you cannot plug a VIF (after ~ 400 such 
> cycles)...
>
> It's worth pointing out, as far as this patch is concerned, that 
> gnttab_end_foreign_access() can fail, 

Just like what Wei mentioned, it is gnttab_end_foreign_access_ref here, 
right?

> which is not taken into account here.

Good point, gnttab_end_foreign_access_ref fails for grant which is in use.

Thanks
Annie
>
> Andrew.
>
>> I'm not netfront maintainer but I'm happy to add
>> Acked-by: Wei Liu <wei.liu2@citrix.com>
>> if Annie confirms she's tested this patch.
>>
>> Wei.
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>>
>

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

[Andrew Bennieston]

On 15/01/14 14:15, annie li wrote:
>
> On 2014-1-15 19:02, Andrew Bennieston wrote:
>> On 15/01/14 10:07, Wei Liu wrote:
>>> On Fri, Jan 10, 2014 at 06:48:38AM +0800, 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.
>>>>
>>>
>>> This path seldom gets call -- not that many people unload xen-netfront
>>> driver. If Annie has tested this patch and it works as expected I think
>>> it's fine.
>>>
>> In XenServer we have seen a number of cases where unplugging and
>> replugging VIFs results in leakage of grant references, eventually
>> leading to a case where you cannot plug a VIF (after ~ 400 such
>> cycles)...
>>
>> It's worth pointing out, as far as this patch is concerned, that
>> gnttab_end_foreign_access() can fail,
>
> Just like what Wei mentioned, it is gnttab_end_foreign_access_ref here,
> right?
Yes, sorry - I forgot to type the _ref part!
>
>> which is not taken into account here.
>
> Good point, gnttab_end_foreign_access_ref fails for grant which is in use.
>
> Thanks
> Annie
>>
>> Andrew.
>>
>>> I'm not netfront maintainer but I'm happy to add
>>> Acked-by: Wei Liu <wei.liu2@citrix.com>
>>> if Annie confirms she's tested this patch.
>>>
>>> Wei.
>>>
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xen.org
>>> http://lists.xen.org/xen-devel
>>>
>>
>

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

[David Vrabel]

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...

> 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...

>  		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.

While checking blkfront for how it handles this, it also doesn't appear
to do the right thing either.

David

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

[Wei Liu]

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.

Wei.

> While checking blkfront for how it handles this, it also doesn't appear
> to do the right thing either.
> 
> David

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

[David Vrabel]

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

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

[annie li]

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@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.

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

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

[Wei Liu]

On Wed, Jan 15, 2014 at 10:17:08PM +0800, annie li wrote:
[...]
> >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.

This is probably not a very good idea as skb_free does a lot more things
than simply freeing pages. You're still leaking something (skb: state,
head etc.) with this approach.

> 2. Add a similar deferred way of gnttab_end_foreign_access in
> gnttab_end_foreign_access_ref.
> 
> Thanks
> Annie
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Vrabel]

On 15/01/14 14:17, annie li wrote:
> 
> 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.

Something like the following (untested!) patch is what I'm thinking of.

Fixups to existing drivers (except netfront) are included but may not be
quite correct.

8<----------
>From 76c254c8e020f4427e8f37c867622f0bfd5ac85f Mon Sep 17 00:00:00 2001
From: David Vrabel <david.vrabel@citrix.com>
Date: Wed, 15 Jan 2014 15:04:52 +0000
Subject: [PATCH] HACK! xen/gnttab: make gnttab_end_foreign_access() more
useful

This is UNTESTED and is an example of the sort of change I'm looking
for.

Freeing the page in gnttab_end_foreign_access() means it cannot be
used where the pages are freed in some other way (e.g., as part of a
kfree_skb()).

Leave the freeing of the page to the caller.  If the page still has
foreign users, take an additional reference before adding it to the
deferred list.

Hack all the users of the call to do something resembling the right
thing.  Not quite sure on the blkfront changes.
---
 drivers/block/xen-blkfront.c    |   22 +++++++++++++---------
 drivers/char/tpm/xen-tpmfront.c |    3 +--
 drivers/pci/xen-pcifront.c      |    3 +--
 drivers/xen/grant-table.c       |   19 +++++++++++--------
 include/xen/grant_table.h       |   11 ++++++-----
 5 files changed, 32 insertions(+), 26 deletions(-)

diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index aa846a4..a586496 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -504,7 +504,7 @@ static void gnttab_handle_deferred(unsigned long unused)
 			if (entry->page) {
 				pr_debug("freeing g.e. %#x (pfn %#lx)\n",
 					 entry->ref, page_to_pfn(entry->page));
-				__free_page(entry->page);
+				put_page(entry->page);
 			} else
 				pr_info("freeing g.e. %#x\n", entry->ref);
 			kfree(entry);
@@ -555,15 +555,18 @@ static void gnttab_add_deferred(grant_ref_t ref,
bool readonly,
 }

 void gnttab_end_foreign_access(grant_ref_t ref, int readonly,
-			       unsigned long page)
+			       struct page *page)
 {
-	if (gnttab_end_foreign_access_ref(ref, readonly)) {
+	if (gnttab_end_foreign_access_ref(ref, readonly))
 		put_free_entry(ref);
-		if (page != 0)
-			free_page(page);
-	} else
-		gnttab_add_deferred(ref, readonly,
-				    page ? virt_to_page(page) : NULL);
+	else {
+		/*
+		 * Take a reference to the page so it's not freed
+		 * while the foreign domain still has access to it.
+		 */
+		get_page(page);
+		gnttab_add_deferred(ref, readonly, page);
+	}
 }
 EXPORT_SYMBOL_GPL(gnttab_end_foreign_access);

diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
index 694dcaf..ffa3ce6 100644
--- a/include/xen/grant_table.h
+++ b/include/xen/grant_table.h
@@ -91,13 +91,14 @@ bool gnttab_trans_grants_available(void);
 int gnttab_end_foreign_access_ref(grant_ref_t ref, int readonly);

 /*
- * Eventually end access through the given grant reference, and once that
- * access has been ended, free the given page too.  Access will be ended
- * immediately iff the grant entry is not in use, otherwise it will happen
- * some time later.  page may be 0, in which case no freeing will occur.
+ * Eventually end access through the given grant reference, if the
+ * page is still in use an additional reference is taken and released
+ * when access has ended.  Access will be ended immediately iff the
+ * grant entry is not in use, otherwise it will happen some time
+ * later.
  */
 void gnttab_end_foreign_access(grant_ref_t ref, int readonly,
-			       unsigned long page);
+			       struct page *page);

 int gnttab_grant_foreign_transfer(domid_t domid, unsigned long pfn);

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index c4a4c90..45a2a01 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -931,14 +931,16 @@ static void blkif_free(struct blkfront_info *info,
int suspend)
 	if (!list_empty(&info->grants)) {
 		list_for_each_entry_safe(persistent_gnt, n,
 		                         &info->grants, node) {
+			struct page *page = pfn_to_page(persistent_gnt->pfn);
+
 			list_del(&persistent_gnt->node);
 			if (persistent_gnt->gref != GRANT_INVALID_REF) {
 				gnttab_end_foreign_access(persistent_gnt->gref,
-				                          0, 0UL);
+				                          0, page);
 				info->persistent_gnts_c--;
 			}
 			if (info->feature_persistent)
-				__free_page(pfn_to_page(persistent_gnt->pfn));
+				__free_page(page);
 			kfree(persistent_gnt);
 		}
 	}
@@ -970,10 +972,13 @@ static void blkif_free(struct blkfront_info *info,
int suspend)
 		       info->shadow[i].req.u.indirect.nr_segments :
 		       info->shadow[i].req.u.rw.nr_segments;
 		for (j = 0; j < segs; j++) {
+			struct page *page;
+
 			persistent_gnt = info->shadow[i].grants_used[j];
-			gnttab_end_foreign_access(persistent_gnt->gref, 0, 0UL);
+			page = pfn_to_page(persistent_gnt->pfn);
+			gnttab_end_foreign_access(persistent_gnt->gref, 0, page);
 			if (info->feature_persistent)
-				__free_page(pfn_to_page(persistent_gnt->pfn));
+				__free_page(page);
 			kfree(persistent_gnt);
 		}

@@ -1010,10 +1015,11 @@ free_shadow:
 	/* Free resources associated with old device channel. */
 	if (info->ring_ref != GRANT_INVALID_REF) {
 		gnttab_end_foreign_access(info->ring_ref, 0,
-					  (unsigned long)info->ring.sring);
+					  virt_to_page(info->ring.sring));
 		info->ring_ref = GRANT_INVALID_REF;
 		info->ring.sring = NULL;
 	}
+	free_page((unsigned long)info->ring.sring);
 	if (info->irq)
 		unbind_from_irqhandler(info->irq, info);
 	info->evtchn = info->irq = 0;
@@ -1053,7 +1059,7 @@ static void blkif_completion(struct blk_shadow *s,
struct blkfront_info *info,
 	}
 	/* Add the persistent grant into the list of free grants */
 	for (i = 0; i < nseg; i++) {
-		if (gnttab_query_foreign_access(s->grants_used[i]->gref)) {
+		if (gnttab_end_foreign_access_ref(s->grants_used[i]->gref, 0)) {
 			/*
 			 * If the grant is still mapped by the backend (the
 			 * backend has chosen to make this grant persistent)
@@ -1072,14 +1078,13 @@ static void blkif_completion(struct blk_shadow
*s, struct blkfront_info *info,
 			 * so it will not be picked again unless we run out of
 			 * persistent grants.
 			 */
-			gnttab_end_foreign_access(s->grants_used[i]->gref, 0, 0UL);
 			s->grants_used[i]->gref = GRANT_INVALID_REF;
 			list_add_tail(&s->grants_used[i]->node, &info->grants);
 		}
 	}
 	if (s->req.operation == BLKIF_OP_INDIRECT) {
 		for (i = 0; i < INDIRECT_GREFS(nseg); i++) {
-			if (gnttab_query_foreign_access(s->indirect_grants[i]->gref)) {
+			if (gnttab_end_foreign_access_ref(s->indirect_grants[i]->gref, 0)) {
 				if (!info->feature_persistent)
 					pr_alert_ratelimited("backed has not unmapped grant: %u\n",
 							     s->indirect_grants[i]->gref);
@@ -1088,7 +1093,6 @@ static void blkif_completion(struct blk_shadow *s,
struct blkfront_info *info,
 			} else {
 				struct page *indirect_page;

-				gnttab_end_foreign_access(s->indirect_grants[i]->gref, 0, 0UL);
 				/*
 				 * Add the used indirect page back to the list of
 				 * available pages for indirect grefs.
diff --git a/drivers/char/tpm/xen-tpmfront.c
b/drivers/char/tpm/xen-tpmfront.c
index c8ff4df..00d1132 100644
--- a/drivers/char/tpm/xen-tpmfront.c
+++ b/drivers/char/tpm/xen-tpmfront.c
@@ -315,8 +315,7 @@ static void ring_free(struct tpm_private *priv)
 	if (priv->ring_ref)
 		gnttab_end_foreign_access(priv->ring_ref, 0,
 				(unsigned long)priv->shr);
-	else
-		free_page((unsigned long)priv->shr);
+	free_page((unsigned long)priv->shr);

 	if (priv->chip && priv->chip->vendor.irq)
 		unbind_from_irqhandler(priv->chip->vendor.irq, priv);
diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
index f7197a7..253a129 100644
--- a/drivers/pci/xen-pcifront.c
+++ b/drivers/pci/xen-pcifront.c
@@ -759,8 +759,7 @@ static void free_pdev(struct pcifront_device *pdev)
 	if (pdev->gnt_ref != INVALID_GRANT_REF)
 		gnttab_end_foreign_access(pdev->gnt_ref, 0 /* r/w page */,
 					  (unsigned long)pdev->sh_info);
-	else
-		free_page((unsigned long)pdev->sh_info);
+	free_page((unsigned long)pdev->sh_info);

 	dev_set_drvdata(&pdev->xdev->dev, NULL);

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

[annie li]

On 2014/1/15 23:13, David Vrabel wrote:
> On 15/01/14 14:17, annie li wrote:
>> 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.
> Something like the following (untested!) patch is what I'm thinking of.
>
> Fixups to existing drivers (except netfront) are included but may not be
> quite correct.

Part of it implements the 1 above, if gnttab_end_foreign_access_ref 
fails and then use gnttab_end_foreign_access to end the grant. Another 
is splitting __free_page from gnttab_end_foreign_access. This is 
improvement for current grant end access, and all drivers that involve 
gnttab_end_foreign_access needs to do corresponding change.
I think it can be a separate patch from my clean up patch which fixes 
grant leaking in netfront.

Thanks
Annie
>
> 8<----------
>  From 76c254c8e020f4427e8f37c867622f0bfd5ac85f Mon Sep 17 00:00:00 2001
> From: David Vrabel <david.vrabel@citrix.com>
> Date: Wed, 15 Jan 2014 15:04:52 +0000
> Subject: [PATCH] HACK! xen/gnttab: make gnttab_end_foreign_access() more
> useful
>
> This is UNTESTED and is an example of the sort of change I'm looking
> for.
>
> Freeing the page in gnttab_end_foreign_access() means it cannot be
> used where the pages are freed in some other way (e.g., as part of a
> kfree_skb()).
>
> Leave the freeing of the page to the caller.  If the page still has
> foreign users, take an additional reference before adding it to the
> deferred list.
>
> Hack all the users of the call to do something resembling the right
> thing.  Not quite sure on the blkfront changes.
> ---
>   drivers/block/xen-blkfront.c    |   22 +++++++++++++---------
>   drivers/char/tpm/xen-tpmfront.c |    3 +--
>   drivers/pci/xen-pcifront.c      |    3 +--
>   drivers/xen/grant-table.c       |   19 +++++++++++--------
>   include/xen/grant_table.h       |   11 ++++++-----
>   5 files changed, 32 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
> index aa846a4..a586496 100644
> --- a/drivers/xen/grant-table.c
> +++ b/drivers/xen/grant-table.c
> @@ -504,7 +504,7 @@ static void gnttab_handle_deferred(unsigned long unused)
>   			if (entry->page) {
>   				pr_debug("freeing g.e. %#x (pfn %#lx)\n",
>   					 entry->ref, page_to_pfn(entry->page));
> -				__free_page(entry->page);
> +				put_page(entry->page);
>   			} else
>   				pr_info("freeing g.e. %#x\n", entry->ref);
>   			kfree(entry);
> @@ -555,15 +555,18 @@ static void gnttab_add_deferred(grant_ref_t ref,
> bool readonly,
>   }
>
>   void gnttab_end_foreign_access(grant_ref_t ref, int readonly,
> -			       unsigned long page)
> +			       struct page *page)
>   {
> -	if (gnttab_end_foreign_access_ref(ref, readonly)) {
> +	if (gnttab_end_foreign_access_ref(ref, readonly))
>   		put_free_entry(ref);
> -		if (page != 0)
> -			free_page(page);
> -	} else
> -		gnttab_add_deferred(ref, readonly,
> -				    page ? virt_to_page(page) : NULL);
> +	else {
> +		/*
> +		 * Take a reference to the page so it's not freed
> +		 * while the foreign domain still has access to it.
> +		 */
> +		get_page(page);
> +		gnttab_add_deferred(ref, readonly, page);
> +	}
>   }
>   EXPORT_SYMBOL_GPL(gnttab_end_foreign_access);
>
> diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
> index 694dcaf..ffa3ce6 100644
> --- a/include/xen/grant_table.h
> +++ b/include/xen/grant_table.h
> @@ -91,13 +91,14 @@ bool gnttab_trans_grants_available(void);
>   int gnttab_end_foreign_access_ref(grant_ref_t ref, int readonly);
>
>   /*
> - * Eventually end access through the given grant reference, and once that
> - * access has been ended, free the given page too.  Access will be ended
> - * immediately iff the grant entry is not in use, otherwise it will happen
> - * some time later.  page may be 0, in which case no freeing will occur.
> + * Eventually end access through the given grant reference, if the
> + * page is still in use an additional reference is taken and released
> + * when access has ended.  Access will be ended immediately iff the
> + * grant entry is not in use, otherwise it will happen some time
> + * later.
>    */
>   void gnttab_end_foreign_access(grant_ref_t ref, int readonly,
> -			       unsigned long page);
> +			       struct page *page);
>
>   int gnttab_grant_foreign_transfer(domid_t domid, unsigned long pfn);
>
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index c4a4c90..45a2a01 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -931,14 +931,16 @@ static void blkif_free(struct blkfront_info *info,
> int suspend)
>   	if (!list_empty(&info->grants)) {
>   		list_for_each_entry_safe(persistent_gnt, n,
>   		                         &info->grants, node) {
> +			struct page *page = pfn_to_page(persistent_gnt->pfn);
> +
>   			list_del(&persistent_gnt->node);
>   			if (persistent_gnt->gref != GRANT_INVALID_REF) {
>   				gnttab_end_foreign_access(persistent_gnt->gref,
> -				                          0, 0UL);
> +				                          0, page);
>   				info->persistent_gnts_c--;
>   			}
>   			if (info->feature_persistent)
> -				__free_page(pfn_to_page(persistent_gnt->pfn));
> +				__free_page(page);
>   			kfree(persistent_gnt);
>   		}
>   	}
> @@ -970,10 +972,13 @@ static void blkif_free(struct blkfront_info *info,
> int suspend)
>   		       info->shadow[i].req.u.indirect.nr_segments :
>   		       info->shadow[i].req.u.rw.nr_segments;
>   		for (j = 0; j < segs; j++) {
> +			struct page *page;
> +
>   			persistent_gnt = info->shadow[i].grants_used[j];
> -			gnttab_end_foreign_access(persistent_gnt->gref, 0, 0UL);
> +			page = pfn_to_page(persistent_gnt->pfn);
> +			gnttab_end_foreign_access(persistent_gnt->gref, 0, page);
>   			if (info->feature_persistent)
> -				__free_page(pfn_to_page(persistent_gnt->pfn));
> +				__free_page(page);
>   			kfree(persistent_gnt);
>   		}
>
> @@ -1010,10 +1015,11 @@ free_shadow:
>   	/* Free resources associated with old device channel. */
>   	if (info->ring_ref != GRANT_INVALID_REF) {
>   		gnttab_end_foreign_access(info->ring_ref, 0,
> -					  (unsigned long)info->ring.sring);
> +					  virt_to_page(info->ring.sring));
>   		info->ring_ref = GRANT_INVALID_REF;
>   		info->ring.sring = NULL;
>   	}
> +	free_page((unsigned long)info->ring.sring);
>   	if (info->irq)
>   		unbind_from_irqhandler(info->irq, info);
>   	info->evtchn = info->irq = 0;
> @@ -1053,7 +1059,7 @@ static void blkif_completion(struct blk_shadow *s,
> struct blkfront_info *info,
>   	}
>   	/* Add the persistent grant into the list of free grants */
>   	for (i = 0; i < nseg; i++) {
> -		if (gnttab_query_foreign_access(s->grants_used[i]->gref)) {
> +		if (gnttab_end_foreign_access_ref(s->grants_used[i]->gref, 0)) {
>   			/*
>   			 * If the grant is still mapped by the backend (the
>   			 * backend has chosen to make this grant persistent)
> @@ -1072,14 +1078,13 @@ static void blkif_completion(struct blk_shadow
> *s, struct blkfront_info *info,
>   			 * so it will not be picked again unless we run out of
>   			 * persistent grants.
>   			 */
> -			gnttab_end_foreign_access(s->grants_used[i]->gref, 0, 0UL);
>   			s->grants_used[i]->gref = GRANT_INVALID_REF;
>   			list_add_tail(&s->grants_used[i]->node, &info->grants);
>   		}
>   	}
>   	if (s->req.operation == BLKIF_OP_INDIRECT) {
>   		for (i = 0; i < INDIRECT_GREFS(nseg); i++) {
> -			if (gnttab_query_foreign_access(s->indirect_grants[i]->gref)) {
> +			if (gnttab_end_foreign_access_ref(s->indirect_grants[i]->gref, 0)) {
>   				if (!info->feature_persistent)
>   					pr_alert_ratelimited("backed has not unmapped grant: %u\n",
>   							     s->indirect_grants[i]->gref);
> @@ -1088,7 +1093,6 @@ static void blkif_completion(struct blk_shadow *s,
> struct blkfront_info *info,
>   			} else {
>   				struct page *indirect_page;
>
> -				gnttab_end_foreign_access(s->indirect_grants[i]->gref, 0, 0UL);
>   				/*
>   				 * Add the used indirect page back to the list of
>   				 * available pages for indirect grefs.
> diff --git a/drivers/char/tpm/xen-tpmfront.c
> b/drivers/char/tpm/xen-tpmfront.c
> index c8ff4df..00d1132 100644
> --- a/drivers/char/tpm/xen-tpmfront.c
> +++ b/drivers/char/tpm/xen-tpmfront.c
> @@ -315,8 +315,7 @@ static void ring_free(struct tpm_private *priv)
>   	if (priv->ring_ref)
>   		gnttab_end_foreign_access(priv->ring_ref, 0,
>   				(unsigned long)priv->shr);
> -	else
> -		free_page((unsigned long)priv->shr);
> +	free_page((unsigned long)priv->shr);
>
>   	if (priv->chip && priv->chip->vendor.irq)
>   		unbind_from_irqhandler(priv->chip->vendor.irq, priv);
> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> index f7197a7..253a129 100644
> --- a/drivers/pci/xen-pcifront.c
> +++ b/drivers/pci/xen-pcifront.c
> @@ -759,8 +759,7 @@ static void free_pdev(struct pcifront_device *pdev)
>   	if (pdev->gnt_ref != INVALID_GRANT_REF)
>   		gnttab_end_foreign_access(pdev->gnt_ref, 0 /* r/w page */,
>   					  (unsigned long)pdev->sh_info);
> -	else
> -		free_page((unsigned long)pdev->sh_info);
> +	free_page((unsigned long)pdev->sh_info);
>
>   	dev_set_drvdata(&pdev->xdev->dev, NULL);
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[annie li]

On 2014-1-15 19:20, 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...

Right. This does not prevent backend from mappings.
Maybe my description is not clear. What I mean here is based on old 
2.6.18 netfront which uses "copying_receiver" to tell netback whether rx 
requires grant copy. Probably changing "grant copy" above into "grant 
access" vs "grant transfer" is more precise. And the 
"gnttab_end_foreign_transfer_ref" is the unnecessary code kept from old 
netfront.

>
>> 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...
>
>>   		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.

Yes, it is possible. But doing kfree_skb is right thing from netfront 
point of view.

>
> Since only a buggy backend would result in this, leaking the skbs and
> grant refs would be acceptable here.

This is the same thing for tx side which uses similar process.

>    I would also print an error.

You mean add some print log here? Is it necessary?

Thanks
Annie
>
> While checking blkfront for how it handles this, it also doesn't appear
> to do the right thing either.
>
> David
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html