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

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

[Annie Li]

This patch implements two things:

* release grant reference and skb for rx path, this fixex resource leaking.
* clean up grant transfer code kept from old netfront(2.6.18) which grants
pages for access/map and transfer. But grant transfer is deprecated in current
netfront, so remove corresponding release code for transfer.

gnttab_end_foreign_access_ref may fail when the grant entry is currently used
for reading or writing. But this patch does not cover this and improvement for
this failure may be implemented in a separate patch.

Test has been run with this patch.

V2: improve patch comments.

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 v2] xen-netfront: clean up code in xennet_release_rx_bufs

[David Vrabel]

On 15/01/14 23:57, Annie Li wrote:
> This patch implements two things:
> 
> * release grant reference and skb for rx path, this fixex resource leaking.
> * clean up grant transfer code kept from old netfront(2.6.18) which grants
> pages for access/map and transfer. But grant transfer is deprecated in current
> netfront, so remove corresponding release code for transfer.
> 
> gnttab_end_foreign_access_ref may fail when the grant entry is currently used
> for reading or writing. But this patch does not cover this and improvement for
> this failure may be implemented in a separate patch.

I don't think replacing a resource leak with a security bug is a good idea.

If you would prefer not to fix the gnttab_end_foreign_access() call, I
think you can fix this in netfront by taking a reference to the page
before calling gnttab_end_foreign_access().  This will ensure the page
isn't freed until the subsequent kfree_skb(), or the gref is released by
the foreign domain (whichever is later).

David

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

[annie li]

On 2014-1-16 19:10, David Vrabel wrote:
> On 15/01/14 23:57, Annie Li wrote:
>> This patch implements two things:
>>
>> * release grant reference and skb for rx path, this fixex resource leaking.
>> * clean up grant transfer code kept from old netfront(2.6.18) which grants
>> pages for access/map and transfer. But grant transfer is deprecated in current
>> netfront, so remove corresponding release code for transfer.
>>
>> gnttab_end_foreign_access_ref may fail when the grant entry is currently used
>> for reading or writing. But this patch does not cover this and improvement for
>> this failure may be implemented in a separate patch.
> I don't think replacing a resource leak with a security bug is a good idea.
>
> If you would prefer not to fix the gnttab_end_foreign_access() call, I
> think you can fix this in netfront by taking a reference to the page
> before calling gnttab_end_foreign_access().  This will ensure the page
> isn't freed until the subsequent kfree_skb(), or the gref is released by
> the foreign domain (whichever is later).

What I thought is to split the implementation into two patches, this 
patch fixes the rx path resource leak(just like what tx path does), then 
a separate patch fixes gnttab_end_foreign_access_ref failure issue for 
both tx/rx through taking reference to the page before 
gnttab_end_foreign_access.
If you'd like they are posted together, I will create new patch for the 
latter and then post them.:-)

Thanks
Annie

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

[David Miller]

From: annie li <annie.li@oracle.com>
Date: Thu, 16 Jan 2014 21:42:19 +0800

> What I thought is to split the implementation into two patches, this
> patch fixes the rx path resource leak(just like what tx path does),
> then a separate patch fixes gnttab_end_foreign_access_ref failure
> issue for both tx/rx through taking reference to the page before
> gnttab_end_foreign_access.
> If you'd like they are posted together, I will create new patch for
> the latter and then post them.:-)

That would probably work best.

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

[annie li]

On 2014/1/16 19:10, David Vrabel wrote:
> On 15/01/14 23:57, Annie Li wrote:
>> This patch implements two things:
>>
>> * release grant reference and skb for rx path, this fixex resource leaking.
>> * clean up grant transfer code kept from old netfront(2.6.18) which grants
>> pages for access/map and transfer. But grant transfer is deprecated in current
>> netfront, so remove corresponding release code for transfer.
>>
>> gnttab_end_foreign_access_ref may fail when the grant entry is currently used
>> for reading or writing. But this patch does not cover this and improvement for
>> this failure may be implemented in a separate patch.
> I don't think replacing a resource leak with a security bug is a good idea.
>
> If you would prefer not to fix the gnttab_end_foreign_access() call, I
> think you can fix this in netfront by taking a reference to the page
> before calling gnttab_end_foreign_access().  This will ensure the page
> isn't freed until the subsequent kfree_skb(), or the gref is released by
> the foreign domain (whichever is later).

Taking a reference to the page before calling 
gnttab_end_foreign_access() delays the free work until kfree_skb(). 
Simply adding put_page before kfree_skb() does not make things different 
from gnttab_end_foreign_access_ref(), and the pages will be freed by 
kfree_skb(), problem will be hit in gnttab_handle_deferred() when 
freeing pages which already be freed.

So put_page is required in gnttab_end_foreign_access(), this will ensure 
either free is taken by kfree_skb or gnttab_handle_deferred. This 
involves changes in blkfront/pcifront/tpmfront(just like your patch), 
this way ensure page is released when ref is end.

Another solution I am thinking is calling gnttab_end_foreign_access() 
with page parameter as NULL, then gnttab_end_foreign_access will only do 
ending grant reference work and releasing page work is done by kfree_skb().

Thanks
Annie

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

[annie li]

On 2014/1/17 14:25, annie li wrote:
>
> On 2014/1/16 19:10, David Vrabel wrote:
>> On 15/01/14 23:57, Annie Li wrote:
>>> This patch implements two things:
>>>
>>> * release grant reference and skb for rx path, this fixex resource 
>>> leaking.
>>> * clean up grant transfer code kept from old netfront(2.6.18) which 
>>> grants
>>> pages for access/map and transfer. But grant transfer is deprecated 
>>> in current
>>> netfront, so remove corresponding release code for transfer.
>>>
>>> gnttab_end_foreign_access_ref may fail when the grant entry is 
>>> currently used
>>> for reading or writing. But this patch does not cover this and 
>>> improvement for
>>> this failure may be implemented in a separate patch.
>> I don't think replacing a resource leak with a security bug is a good 
>> idea.
>>
>> If you would prefer not to fix the gnttab_end_foreign_access() call, I
>> think you can fix this in netfront by taking a reference to the page
>> before calling gnttab_end_foreign_access().  This will ensure the page
>> isn't freed until the subsequent kfree_skb(), or the gref is released by
>> the foreign domain (whichever is later).
>
> Taking a reference to the page before calling 
> gnttab_end_foreign_access() delays the free work until kfree_skb(). 
> Simply adding put_page before kfree_skb() does not make things 
> different from gnttab_end_foreign_access_ref(), and the pages will be 
> freed by kfree_skb(), problem will be hit in gnttab_handle_deferred() 
> when freeing pages which already be freed.
>
> So put_page is required in gnttab_end_foreign_access(), this will 
> ensure either free is taken by kfree_skb or gnttab_handle_deferred. 
> This involves changes in blkfront/pcifront/tpmfront(just like your 
> patch), this way ensure page is released when ref is end.
>
> Another solution I am thinking is calling gnttab_end_foreign_access() 
> with page parameter as NULL, then gnttab_end_foreign_access will only 
> do ending grant reference work and releasing page work is done by 
> kfree_skb().

Not NULL above, it should be 0UL.

Thanks
Annie

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

[Wei Liu]

On Fri, Jan 17, 2014 at 02:25:40PM +0800, annie li wrote:
> 
> On 2014/1/16 19:10, David Vrabel wrote:
> >On 15/01/14 23:57, Annie Li wrote:
> >>This patch implements two things:
> >>
> >>* release grant reference and skb for rx path, this fixex resource leaking.
> >>* clean up grant transfer code kept from old netfront(2.6.18) which grants
> >>pages for access/map and transfer. But grant transfer is deprecated in current
> >>netfront, so remove corresponding release code for transfer.
> >>
> >>gnttab_end_foreign_access_ref may fail when the grant entry is currently used
> >>for reading or writing. But this patch does not cover this and improvement for
> >>this failure may be implemented in a separate patch.
> >I don't think replacing a resource leak with a security bug is a good idea.
> >
> >If you would prefer not to fix the gnttab_end_foreign_access() call, I
> >think you can fix this in netfront by taking a reference to the page
> >before calling gnttab_end_foreign_access().  This will ensure the page
> >isn't freed until the subsequent kfree_skb(), or the gref is released by
> >the foreign domain (whichever is later).
> 
> Taking a reference to the page before calling
> gnttab_end_foreign_access() delays the free work until kfree_skb().
> Simply adding put_page before kfree_skb() does not make things
> different from gnttab_end_foreign_access_ref(), and the pages will
> be freed by kfree_skb(), problem will be hit in
> gnttab_handle_deferred() when freeing pages which already be freed.
> 

I think David's idea is:

	get_page
	gnttab_end_foreign_access
	kfree_skb

The get_page is to offset put_page in gnttab_end_foreign_access. You
don't need to put page before kfree_skb.

Wei.

> So put_page is required in gnttab_end_foreign_access(), this will
> ensure either free is taken by kfree_skb or gnttab_handle_deferred.
> This involves changes in blkfront/pcifront/tpmfront(just like your
> patch), this way ensure page is released when ref is end.
> 
> Another solution I am thinking is calling
> gnttab_end_foreign_access() with page parameter as NULL, then
> gnttab_end_foreign_access will only do ending grant reference work
> and releasing page work is done by kfree_skb().
> 
> Thanks
> Annie

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

[annie li]

On 2014-1-17 20:08, Wei Liu wrote:
> On Fri, Jan 17, 2014 at 02:25:40PM +0800, annie li wrote:
>> On 2014/1/16 19:10, David Vrabel wrote:
>>> On 15/01/14 23:57, Annie Li wrote:
>>>> This patch implements two things:
>>>>
>>>> * release grant reference and skb for rx path, this fixex resource leaking.
>>>> * clean up grant transfer code kept from old netfront(2.6.18) which grants
>>>> pages for access/map and transfer. But grant transfer is deprecated in current
>>>> netfront, so remove corresponding release code for transfer.
>>>>
>>>> gnttab_end_foreign_access_ref may fail when the grant entry is currently used
>>>> for reading or writing. But this patch does not cover this and improvement for
>>>> this failure may be implemented in a separate patch.
>>> I don't think replacing a resource leak with a security bug is a good idea.
>>>
>>> If you would prefer not to fix the gnttab_end_foreign_access() call, I
>>> think you can fix this in netfront by taking a reference to the page
>>> before calling gnttab_end_foreign_access().  This will ensure the page
>>> isn't freed until the subsequent kfree_skb(), or the gref is released by
>>> the foreign domain (whichever is later).
>> Taking a reference to the page before calling
>> gnttab_end_foreign_access() delays the free work until kfree_skb().
>> Simply adding put_page before kfree_skb() does not make things
>> different from gnttab_end_foreign_access_ref(), and the pages will
>> be freed by kfree_skb(), problem will be hit in
>> gnttab_handle_deferred() when freeing pages which already be freed.
>>
> I think David's idea is:
>
> 	get_page
> 	gnttab_end_foreign_access
> 	kfree_skb
>
> The get_page is to offset put_page in gnttab_end_foreign_access. You
> don't need to put page before kfree_skb.

Yes, this is what I described as following about David's patch.

>> So put_page is required in gnttab_end_foreign_access(), this will
>> ensure either free is taken by kfree_skb or gnttab_handle_deferred.
>> This involves changes in blkfront/pcifront/tpmfront(just like your
>> patch), this way ensure page is released when ref is end.

But this would has some issue in netfront tx path. Netfront ends all 
grant reference of one skb first and then release this skb. If the 
gnttab_end_foreign_access_ref fails in gnttab_end_foreign_access(), this 
frag page and corresponding grant reference will be put in entry and 
release work will be done in the timer routine. If some frag pages of 
one skb is free in this timer routine, then dev_kfree_skb_irq will free 
pages which have been freed.
So I prefer following way I mentioned, suggestions?

>> Another solution I am thinking is calling
>> gnttab_end_foreign_access() with page parameter as NULL, then
>> gnttab_end_foreign_access will only do ending grant reference work
>> and releasing page work is done by kfree_skb().

Thanks
Annie

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

[Wei Liu]

On Fri, Jan 17, 2014 at 08:32:29PM +0800, annie li wrote:
> 
> On 2014-1-17 20:08, Wei Liu wrote:
> >On Fri, Jan 17, 2014 at 02:25:40PM +0800, annie li wrote:
> >>On 2014/1/16 19:10, David Vrabel wrote:
> >>>On 15/01/14 23:57, Annie Li wrote:
> >>>>This patch implements two things:
> >>>>
> >>>>* release grant reference and skb for rx path, this fixex resource leaking.
> >>>>* clean up grant transfer code kept from old netfront(2.6.18) which grants
> >>>>pages for access/map and transfer. But grant transfer is deprecated in current
> >>>>netfront, so remove corresponding release code for transfer.
> >>>>
> >>>>gnttab_end_foreign_access_ref may fail when the grant entry is currently used
> >>>>for reading or writing. But this patch does not cover this and improvement for
> >>>>this failure may be implemented in a separate patch.
> >>>I don't think replacing a resource leak with a security bug is a good idea.
> >>>
> >>>If you would prefer not to fix the gnttab_end_foreign_access() call, I
> >>>think you can fix this in netfront by taking a reference to the page
> >>>before calling gnttab_end_foreign_access().  This will ensure the page
> >>>isn't freed until the subsequent kfree_skb(), or the gref is released by
> >>>the foreign domain (whichever is later).
> >>Taking a reference to the page before calling
> >>gnttab_end_foreign_access() delays the free work until kfree_skb().
> >>Simply adding put_page before kfree_skb() does not make things
> >>different from gnttab_end_foreign_access_ref(), and the pages will
> >>be freed by kfree_skb(), problem will be hit in
> >>gnttab_handle_deferred() when freeing pages which already be freed.
> >>
> >I think David's idea is:
> >
> >	get_page
> >	gnttab_end_foreign_access
> >	kfree_skb
> >
> >The get_page is to offset put_page in gnttab_end_foreign_access. You
> >don't need to put page before kfree_skb.
> 
> Yes, this is what I described as following about David's patch.
> 
> >>So put_page is required in gnttab_end_foreign_access(), this will
> >>ensure either free is taken by kfree_skb or gnttab_handle_deferred.
> >>This involves changes in blkfront/pcifront/tpmfront(just like your
> >>patch), this way ensure page is released when ref is end.
> 
> But this would has some issue in netfront tx path. Netfront ends all

What issue with tx path? Your patch only touches rx skbs, doesn't it?

> grant reference of one skb first and then release this skb. If the
> gnttab_end_foreign_access_ref fails in gnttab_end_foreign_access(),
> this frag page and corresponding grant reference will be put in
> entry and release work will be done in the timer routine. If some

I understand up to this point.

> frag pages of one skb is free in this timer routine, then
> dev_kfree_skb_irq will free pages which have been freed.

Why is dev_kfree_skb_irq involved? It is used in tx path not rx path.
Even if we look at dev_kfree_skb_irq, it calls __kfree_skb for dropped
packet eventually, which should do the right thing if we don't mess up
ref counts.

Wei.

> So I prefer following way I mentioned, suggestions?
> 
> >>Another solution I am thinking is calling
> >>gnttab_end_foreign_access() with page parameter as NULL, then
> >>gnttab_end_foreign_access will only do ending grant reference work
> >>and releasing page work is done by kfree_skb().
> 
> Thanks
> Annie

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

[annie li]

On 2014-1-17 22:02, Wei Liu wrote:
> On Fri, Jan 17, 2014 at 08:32:29PM +0800, annie li wrote:
>> On 2014-1-17 20:08, Wei Liu wrote:
>>> On Fri, Jan 17, 2014 at 02:25:40PM +0800, annie li wrote:
>>>> On 2014/1/16 19:10, David Vrabel wrote:
>>>>> On 15/01/14 23:57, Annie Li wrote:
>>>>>> This patch implements two things:
>>>>>>
>>>>>> * release grant reference and skb for rx path, this fixex resource leaking.
>>>>>> * clean up grant transfer code kept from old netfront(2.6.18) which grants
>>>>>> pages for access/map and transfer. But grant transfer is deprecated in current
>>>>>> netfront, so remove corresponding release code for transfer.
>>>>>>
>>>>>> gnttab_end_foreign_access_ref may fail when the grant entry is currently used
>>>>>> for reading or writing. But this patch does not cover this and improvement for
>>>>>> this failure may be implemented in a separate patch.
>>>>> I don't think replacing a resource leak with a security bug is a good idea.
>>>>>
>>>>> If you would prefer not to fix the gnttab_end_foreign_access() call, I
>>>>> think you can fix this in netfront by taking a reference to the page
>>>>> before calling gnttab_end_foreign_access().  This will ensure the page
>>>>> isn't freed until the subsequent kfree_skb(), or the gref is released by
>>>>> the foreign domain (whichever is later).
>>>> Taking a reference to the page before calling
>>>> gnttab_end_foreign_access() delays the free work until kfree_skb().
>>>> Simply adding put_page before kfree_skb() does not make things
>>>> different from gnttab_end_foreign_access_ref(), and the pages will
>>>> be freed by kfree_skb(), problem will be hit in
>>>> gnttab_handle_deferred() when freeing pages which already be freed.
>>>>
>>> I think David's idea is:
>>>
>>> 	get_page
>>> 	gnttab_end_foreign_access
>>> 	kfree_skb
>>>
>>> The get_page is to offset put_page in gnttab_end_foreign_access. You
>>> don't need to put page before kfree_skb.
>> Yes, this is what I described as following about David's patch.
>>
>>>> So put_page is required in gnttab_end_foreign_access(), this will
>>>> ensure either free is taken by kfree_skb or gnttab_handle_deferred.
>>>> This involves changes in blkfront/pcifront/tpmfront(just like your
>>>> patch), this way ensure page is released when ref is end.
>> But this would has some issue in netfront tx path. Netfront ends all
> What issue with tx path? Your patch only touches rx skbs, doesn't it?

No, I am trying to implement 2 patches. One is my original patch which 
fix rx leaking, another is to improve gnttab_end_foreign_access, it 
would involve not only tx path, but also blkfront/pcifront/tpmfront 
since they use gnttab_end_foreign_access in their source code.

>
>> grant reference of one skb first and then release this skb. If the
>> gnttab_end_foreign_access_ref fails in gnttab_end_foreign_access(),
>> this frag page and corresponding grant reference will be put in
>> entry and release work will be done in the timer routine. If some
> I understand up to this point.
>
>> frag pages of one skb is free in this timer routine, then
>> dev_kfree_skb_irq will free pages which have been freed.
> Why is dev_kfree_skb_irq involved? It is used in tx path not rx path.

This is involved in second patch as David suggested, it ensures page 
would be released when grant access is end and avoid situation where 
page is released but grant reference is still mapped.

> Even if we look at dev_kfree_skb_irq, it calls __kfree_skb for dropped
> packet eventually, which should do the right thing if we don't mess up
> ref counts.

I think you are right, I mixed it with get_skb just now. Either 
__kfree_skb or gnttab_end_foreign_access() does the free work.

Thanks
Annie
>
> Wei.
>
>> So I prefer following way I mentioned, suggestions?
>>
>>>> Another solution I am thinking is calling
>>>> gnttab_end_foreign_access() with page parameter as NULL, then
>>>> gnttab_end_foreign_access will only do ending grant reference work
>>>> and releasing page work is done by kfree_skb().
>> Thanks
>> Annie

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

[David Vrabel]

On 17/01/14 15:43, annie li wrote:
> 
> No, I am trying to implement 2 patches.

I don't understand the need for two patches here, particularly when
the first patch introduces a security issue.  You can fold the following 
(untested) patch into your v2 patch and give it a try?

Thanks.

David

8<----------------------
xen-netfront: prevent unsafe reuse of rx buf pages after uninit

---
 drivers/net/xen-netfront.c |   21 +++++++++++++++++----
 1 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 692589e..47aa599 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -1134,19 +1134,32 @@ static void xennet_release_tx_bufs(struct netfront_info *np)
 
 static void xennet_release_rx_bufs(struct netfront_info *np)
 {
-	struct sk_buff *skb;
 	int id, ref;
 
 	spin_lock_bh(&np->rx_lock);
 
 	for (id = 0; id < NET_RX_RING_SIZE; id++) {
+		struct sk_buff *skb;
+		skb_frag_t *frag;
+		const struct page *page;
+
+		skb = np->rx_skbs[id];
+		if (!skb)
+			continue;
+
 		ref = np->grant_rx_ref[id];
 		if (ref == GRANT_INVALID_REF)
 			continue;
 
-		skb = np->rx_skbs[id];
-		gnttab_end_foreign_access_ref(ref, 0);
-		gnttab_release_grant_reference(&np->gref_rx_head, ref);
+		frag = &skb_shinfo(skb)->frags[0];
+		page = skb_frag_page(frag);
+
+		/* gnttab_end_foreign_access() needs a page ref until
+		 * foreign access is ended (which may be deferred).
+		 */
+		get_page(page);
+
+		gnttab_end_foreign_access(ref, 0, page);
 		np->grant_rx_ref[id] = GRANT_INVALID_REF;
 
 		kfree_skb(skb);
-- 
1.7.2.5

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

[annie li]

On 2014/1/18 1:50, David Vrabel wrote:
> On 17/01/14 15:43, annie li wrote:
>> No, I am trying to implement 2 patches.
> I don't understand the need for two patches here, particularly when
> the first patch introduces a security issue.

This is basically connected with personal taste. I am thinking that my 
original patch is removing unnecessary code for grant transfer and then 
keep rx release consistent with tx path, the security issue you 
mentioned exist in current tx too. The second one is to change 
gnttab_end_foreign_access and netfront tx/rx, blkfront, etc. But if you 
like to merge them together, I can do that.

Thanks
Annie
> You can fold the following
> (untested) patch into your v2 patch and give it a try?
>
> Thanks.
>
> David
>
> 8<----------------------
> xen-netfront: prevent unsafe reuse of rx buf pages after uninit
>
> ---
>   drivers/net/xen-netfront.c |   21 +++++++++++++++++----
>   1 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
> index 692589e..47aa599 100644
> --- a/drivers/net/xen-netfront.c
> +++ b/drivers/net/xen-netfront.c
> @@ -1134,19 +1134,32 @@ static void xennet_release_tx_bufs(struct netfront_info *np)
>   
>   static void xennet_release_rx_bufs(struct netfront_info *np)
>   {
> -	struct sk_buff *skb;
>   	int id, ref;
>   
>   	spin_lock_bh(&np->rx_lock);
>   
>   	for (id = 0; id < NET_RX_RING_SIZE; id++) {
> +		struct sk_buff *skb;
> +		skb_frag_t *frag;
> +		const struct page *page;
> +
> +		skb = np->rx_skbs[id];
> +		if (!skb)
> +			continue;
> +
>   		ref = np->grant_rx_ref[id];
>   		if (ref == GRANT_INVALID_REF)
>   			continue;
>   
> -		skb = np->rx_skbs[id];
> -		gnttab_end_foreign_access_ref(ref, 0);
> -		gnttab_release_grant_reference(&np->gref_rx_head, ref);
> +		frag = &skb_shinfo(skb)->frags[0];
> +		page = skb_frag_page(frag);
> +
> +		/* gnttab_end_foreign_access() needs a page ref until
> +		 * foreign access is ended (which may be deferred).
> +		 */
> +		get_page(page);
> +
> +		gnttab_end_foreign_access(ref, 0, page);
>   		np->grant_rx_ref[id] = GRANT_INVALID_REF;
>   
>   		kfree_skb(skb);

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

[David Vrabel]

On 17/01/14 12:08, Wei Liu wrote:
> On Fri, Jan 17, 2014 at 02:25:40PM +0800, annie li wrote:
>>
>> On 2014/1/16 19:10, David Vrabel wrote:
>>> On 15/01/14 23:57, Annie Li wrote:
>>>> This patch implements two things:
>>>>
>>>> * release grant reference and skb for rx path, this fixex resource leaking.
>>>> * clean up grant transfer code kept from old netfront(2.6.18) which grants
>>>> pages for access/map and transfer. But grant transfer is deprecated in current
>>>> netfront, so remove corresponding release code for transfer.
>>>>
>>>> gnttab_end_foreign_access_ref may fail when the grant entry is currently used
>>>> for reading or writing. But this patch does not cover this and improvement for
>>>> this failure may be implemented in a separate patch.
>>> I don't think replacing a resource leak with a security bug is a good idea.
>>>
>>> If you would prefer not to fix the gnttab_end_foreign_access() call, I
>>> think you can fix this in netfront by taking a reference to the page
>>> before calling gnttab_end_foreign_access().  This will ensure the page
>>> isn't freed until the subsequent kfree_skb(), or the gref is released by
>>> the foreign domain (whichever is later).
>>
>> Taking a reference to the page before calling
>> gnttab_end_foreign_access() delays the free work until kfree_skb().
>> Simply adding put_page before kfree_skb() does not make things
>> different from gnttab_end_foreign_access_ref(), and the pages will
>> be freed by kfree_skb(), problem will be hit in
>> gnttab_handle_deferred() when freeing pages which already be freed.
>>
> 
> I think David's idea is:
> 
> 	get_page
> 	gnttab_end_foreign_access
> 	kfree_skb
> 
> The get_page is to offset put_page in gnttab_end_foreign_access. You
> don't need to put page before kfree_skb.

Yes.

David