[PATCH net RFC] xen-netback: Fix grant ref resolution in RX path

[PATCH net RFC] xen-netback: Fix grant ref resolution in RX path

[Zoltan Kiss]

The original series for reintroducing grant mapping for netback had a patch [1]
to handle receiving of packets from an another VIF. Grant copy on the receiving
side needs the grant ref of the page to set up the op.
The original patch assumed (wrongly) that the frags array haven't changed. In
the case reported by Sander, the sending guest sent a packet where the linear
buffer and the first frag were under PKT_PROT_LEN (=128) bytes.
xenvif_tx_submit() then pulled up the linear area to 128 bytes, and ditched the
first frag. The receiving side had an off-by-one problem when gathered the grant
refs.
This patch fixes that by checking whether the actual frag's page pointer is the
same as the page in the original frag list. It can handle any kind of changes on
the original frags array, like:
- removing granted frags from the beginning or the end
- adding local pages to the frags list
To keep it optimized to the most common cases, it doesn't handle when the order
of the original frags changed. That would require ubuf to be reseted to the
beginning of the chain (skb_shinfo(skb)->destructor_arg), and reiterating
through the list every time.

OPEN QUESTIONS:
- Is it a safe assumption that nothing changes the order of the original frags?
  Removing them from the array or injecting new pages anywhere is not a problem.
- I used UINT_MAX as a kind of INVALID_GRANT_REF, however there is no such thing
  in the grant mapping API. Should we codify this or is it better if we just
  find another way to distinguish whether a frag is local or not?
- Should this fix go to David's net tree or directly to the mainline tree? Or
  both?

[1]: 3e2234: xen-netback: Handle foreign mapped pages on the guest RX path

Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
---
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index 7666540..e18ac23 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -278,7 +278,7 @@ static void xenvif_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
 		copy_gop->flags = GNTCOPY_dest_gref;
 		copy_gop->len = bytes;
 
-		if (foreign_vif) {
+		if (foreign_vif && (foreign_gref != UINT_MAX)) {
 			copy_gop->source.domid = foreign_vif->domid;
 			copy_gop->source.u.ref = foreign_gref;
 			copy_gop->flags |= GNTCOPY_source_gref;
@@ -388,15 +388,22 @@ static int xenvif_gop_skb(struct sk_buff *skb,
 
 	if ((skb_shinfo(skb)->tx_flags & SKBTX_DEV_ZEROCOPY) &&
 		 (ubuf->callback == &xenvif_zerocopy_callback)) {
-		int i = 0;
 		foreign_vif = ubuf_to_vif(ubuf);
 
-		do {
-			u16 pending_idx = ubuf->desc;
-			foreign_grefs[i++] =
-				foreign_vif->pending_tx_info[pending_idx].req.gref;
-			ubuf = (struct ubuf_info *) ubuf->ctx;
-		} while (ubuf);
+		for (i = 0; i < nr_frags; i++) {
+			foreign_grefs[i] = UINT_MAX;
+			do {
+				u16 pending_idx = ubuf->desc;
+
+				ubuf = (struct ubuf_info *) ubuf->ctx;
+				if (skb_shinfo(skb)->frags[i].page.p ==
+					foreign_vif->mmap_pages[pending_idx]) {
+					foreign_grefs[i] =
+						foreign_vif->pending_tx_info[pending_idx].req.gref;
+					break;
+				}
+			} while (ubuf);
+		}
 	}
 
 	data = skb->data;

Re: [PATCH net RFC] xen-netback: Fix grant ref resolution in RX path

[Ian Campbell]

On Tue, 2014-05-13 at 15:31 +0100, Zoltan Kiss wrote:
> The original series for reintroducing grant mapping for netback had a patch [1]
> to handle receiving of packets from an another VIF. Grant copy on the receiving
> side needs the grant ref of the page to set up the op.
> The original patch assumed (wrongly) that the frags array haven't changed. In
> the case reported by Sander, the sending guest sent a packet where the linear
> buffer and the first frag were under PKT_PROT_LEN (=128) bytes.
> xenvif_tx_submit() then pulled up the linear area to 128 bytes, and ditched the
> first frag. The receiving side had an off-by-one problem when gathered the grant
> refs.

The grant refs are gather here from the ubuf_info * link list stored in
skb->destructor_arg, is that right?

And the issue is that pulling up can consume a frag (e.g. and shift
everything up) without adjusting that linked list?

I think the commit message should make explicit reference to this linked
list and where it lives and make the implications of doing a pull up on
that list clear.

So the fix is when walking the list instead of assuming a 1:1
relationship between frags and list elements you need to traverse the
list looking for one which matches?

The ubuf's themselves are part of pending_tx_info -- is it the case that
the pending entries are retained until the skb completes even when the
corresponding frag is pulled up?

I suppose the network stack does pull ups itself -- so we can't fix this
by just fixing up the ubuf list as we do the initial PKT_PROT_LEN pull
up because there will be other pull ups which we don't control?

> This patch fixes that by checking whether the actual frag's page pointer is the
> same as the page in the original frag list. It can handle any kind of changes on
> the original frags array, like:
> - removing granted frags from the beginning or the end
> - adding local pages to the frags list
> To keep it optimized to the most common cases, it doesn't handle when the order
> of the original frags changed. That would require ubuf to be reseted to the
> beginning of the chain (skb_shinfo(skb)->destructor_arg), and reiterating
> through the list every time.

Do we need to keep the ubuf list around after this operation or could it
be destructive? I ask because by removing the entries from the list as
they are consumed the optimal case will then always have the next frag
in the head but it will handle reordering.

> OPEN QUESTIONS:
> - Is it a safe assumption that nothing changes the order of the original frags?
>   Removing them from the array or injecting new pages anywhere is not a problem.
> - I used UINT_MAX as a kind of INVALID_GRANT_REF, however there is no such thing
>   in the grant mapping API. Should we codify this or is it better if we just
>   find another way to distinguish whether a frag is local or not?

I think UINT_MAX is probably OK, but can't you avoid this by doing the
search in the existing loop over the frags which calls
xenvif_gop_frag_copy? If you do that then you can pass foreign_vif as
NULL in the cases where there is no ubuf entry.

> - Should this fix go to David's net tree or directly to the mainline tree? Or
>   both?

Via David as normal I think, but we need this for v3.15 final.

> [1]: 3e2234: xen-netback: Handle foreign mapped pages on the guest RX path
> 
> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
> ---
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index 7666540..e18ac23 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -278,7 +278,7 @@ static void xenvif_gop_frag_copy(struct xenvif *vif, struct sk_buff *skb,
>  		copy_gop->flags = GNTCOPY_dest_gref;
>  		copy_gop->len = bytes;
>  
> -		if (foreign_vif) {
> +		if (foreign_vif && (foreign_gref != UINT_MAX)) {
>  			copy_gop->source.domid = foreign_vif->domid;
>  			copy_gop->source.u.ref = foreign_gref;
>  			copy_gop->flags |= GNTCOPY_source_gref;
> @@ -388,15 +388,22 @@ static int xenvif_gop_skb(struct sk_buff *skb,
>  
>  	if ((skb_shinfo(skb)->tx_flags & SKBTX_DEV_ZEROCOPY) &&
>  		 (ubuf->callback == &xenvif_zerocopy_callback)) {
> -		int i = 0;
>  		foreign_vif = ubuf_to_vif(ubuf);
>  
> -		do {
> -			u16 pending_idx = ubuf->desc;
> -			foreign_grefs[i++] =
> -				foreign_vif->pending_tx_info[pending_idx].req.gref;
> -			ubuf = (struct ubuf_info *) ubuf->ctx;
> -		} while (ubuf);
> +		for (i = 0; i < nr_frags; i++) {
> +			foreign_grefs[i] = UINT_MAX;
> +			do {
> +				u16 pending_idx = ubuf->desc;
> +
> +				ubuf = (struct ubuf_info *) ubuf->ctx;
> +				if (skb_shinfo(skb)->frags[i].page.p ==
> +					foreign_vif->mmap_pages[pending_idx]) {
> +					foreign_grefs[i] =
> +						foreign_vif->pending_tx_info[pending_idx].req.gref;
> +					break;
> +				}
> +			} while (ubuf);
> +		}
>  	}
>  
>  	data = skb->data;

Re: [PATCH net RFC] xen-netback: Fix grant ref resolution in RX path

[Zoltan Kiss]

On 13/05/14 16:47, Ian Campbell wrote:
> On Tue, 2014-05-13 at 15:31 +0100, Zoltan Kiss wrote:
>> The original series for reintroducing grant mapping for netback had a patch [1]
>> to handle receiving of packets from an another VIF. Grant copy on the receiving
>> side needs the grant ref of the page to set up the op.
>> The original patch assumed (wrongly) that the frags array haven't changed. In
>> the case reported by Sander, the sending guest sent a packet where the linear
>> buffer and the first frag were under PKT_PROT_LEN (=128) bytes.
>> xenvif_tx_submit() then pulled up the linear area to 128 bytes, and ditched the
>> first frag. The receiving side had an off-by-one problem when gathered the grant
>> refs.
>
> The grant refs are gather here from the ubuf_info * link list stored in
> skb->destructor_arg, is that right?
Yes
>
> And the issue is that pulling up can consume a frag (e.g. and shift
> everything up) without adjusting that linked list?
Yes. Or any kind of change on the frags list (expect changing size and 
offset of an entry) breaks this part of the code at the moment.

>
> I think the commit message should make explicit reference to this linked
> list and where it lives and make the implications of doing a pull up on
> that list clear.
Ok, I'll add a reference to the description in common.h
>
> So the fix is when walking the list instead of assuming a 1:1
> relationship between frags and list elements you need to traverse the
> list looking for one which matches?
Yes.
>
> The ubuf's themselves are part of pending_tx_info -- is it the case that
> the pending entries are retained until the skb completes even when the
> corresponding frag is pulled up?
Yes. Pulling does remove the page from the frag list and put_page it, 
and when the callback called, it'll make sure it is given back to the 
sender.
>
> I suppose the network stack does pull ups itself -- so we can't fix this
> by just fixing up the ubuf list as we do the initial PKT_PROT_LEN pull
> up because there will be other pull ups which we don't control?
Yes. And I guess it's better to assume that other things can happen, 
e.g. see pskb_trim
>
>> This patch fixes that by checking whether the actual frag's page pointer is the
>> same as the page in the original frag list. It can handle any kind of changes on
>> the original frags array, like:
>> - removing granted frags from the beginning or the end
>> - adding local pages to the frags list
>> To keep it optimized to the most common cases, it doesn't handle when the order
>> of the original frags changed. That would require ubuf to be reseted to the
>> beginning of the chain (skb_shinfo(skb)->destructor_arg), and reiterating
>> through the list every time.
>
> Do we need to keep the ubuf list around after this operation or could it
> be destructive? I ask because by removing the entries from the list as
> they are consumed the optimal case will then always have the next frag
> in the head but it will handle reordering.
Yes, we need this chained list until the zerocopy callback walks through 
it and add the pending_idx's to the dealloc queue. I was tinkering with 
the idea of moving the not matching entries back to the head of the 
list, as the callback doesn't really care about the order, and I guess 
the guest neither. But that would look even scarier.
>
>> OPEN QUESTIONS:
>> - Is it a safe assumption that nothing changes the order of the original frags?
>>    Removing them from the array or injecting new pages anywhere is not a problem.
>> - I used UINT_MAX as a kind of INVALID_GRANT_REF, however there is no such thing
>>    in the grant mapping API. Should we codify this or is it better if we just
>>    find another way to distinguish whether a frag is local or not?
>
> I think UINT_MAX is probably OK, but can't you avoid this by doing the
> search in the existing loop over the frags which calls
> xenvif_gop_frag_copy? If you do that then you can pass foreign_vif as
> NULL in the cases where there is no ubuf entry.
Yes, that sounds better! That would make a lot of things easier.

I've reworked the patch to include frag reordering as well.

Zoli

Re: [PATCH net RFC] xen-netback: Fix grant ref resolution in RX path

[Eric Dumazet]

On Tue, 2014-05-13 at 15:31 +0100, Zoltan Kiss wrote:
> The original series for reintroducing grant mapping for netback had a patch [1]
> to handle receiving of packets from an another VIF. Grant copy on the receiving
> side needs the grant ref of the page to set up the op.
> The original patch assumed (wrongly) that the frags array haven't changed. In
> the case reported by Sander, the sending guest sent a packet where the linear
> buffer and the first frag were under PKT_PROT_LEN (=128) bytes.
> xenvif_tx_submit() then pulled up the linear area to 128 bytes, and ditched the
> first frag. The receiving side had an off-by-one problem when gathered the grant
> refs.
> This patch fixes that by checking whether the actual frag's page pointer is the
> same as the page in the original frag list. It can handle any kind of changes on
> the original frags array, like:
> - removing granted frags from the beginning or the end
> - adding local pages to the frags list
> To keep it optimized to the most common cases, it doesn't handle when the order
> of the original frags changed. That would require ubuf to be reseted to the
> beginning of the chain (skb_shinfo(skb)->destructor_arg), and reiterating
> through the list every time.
> 
> OPEN QUESTIONS:
> - Is it a safe assumption that nothing changes the order of the original frags?
>   Removing them from the array or injecting new pages anywhere is not a problem.
> - I used UINT_MAX as a kind of INVALID_GRANT_REF, however there is no such thing
>   in the grant mapping API. Should we codify this or is it better if we just
>   find another way to distinguish whether a frag is local or not?
> - Should this fix go to David's net tree or directly to the mainline tree? Or
>   both?
> 
> [1]: 3e2234: xen-netback: Handle foreign mapped pages on the guest RX path
> 
> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
> ---


The 'cleanup' of stale ubufs should be right after __pskb_pull_tail().

This is the function that can 'consume frags' after all.

Its not clear that you catch all cases, like skbs being purged in case
of device dismantle.

I am not saying your patch is wrong, only that it adds yet an obscure
thing with no comments. In two years, nobody will understand this.

Re: [PATCH net RFC] xen-netback: Fix grant ref resolution in RX path

[Sander Eikelenboom]

Tuesday, May 13, 2014, 6:13:55 PM, you wrote:

> On Tue, 2014-05-13 at 15:31 +0100, Zoltan Kiss wrote:
>> The original series for reintroducing grant mapping for netback had a patch [1]
>> to handle receiving of packets from an another VIF. Grant copy on the receiving
>> side needs the grant ref of the page to set up the op.
>> The original patch assumed (wrongly) that the frags array haven't changed. In
>> the case reported by Sander, the sending guest sent a packet where the linear
>> buffer and the first frag were under PKT_PROT_LEN (=128) bytes.
>> xenvif_tx_submit() then pulled up the linear area to 128 bytes, and ditched the
>> first frag. The receiving side had an off-by-one problem when gathered the grant
>> refs.
>> This patch fixes that by checking whether the actual frag's page pointer is the
>> same as the page in the original frag list. It can handle any kind of changes on
>> the original frags array, like:
>> - removing granted frags from the beginning or the end
>> - adding local pages to the frags list
>> To keep it optimized to the most common cases, it doesn't handle when the order
>> of the original frags changed. That would require ubuf to be reseted to the
>> beginning of the chain (skb_shinfo(skb)->destructor_arg), and reiterating
>> through the list every time.
>> 
>> OPEN QUESTIONS:
>> - Is it a safe assumption that nothing changes the order of the original frags?
>>   Removing them from the array or injecting new pages anywhere is not a problem.
>> - I used UINT_MAX as a kind of INVALID_GRANT_REF, however there is no such thing
>>   in the grant mapping API. Should we codify this or is it better if we just
>>   find another way to distinguish whether a frag is local or not?
>> - Should this fix go to David's net tree or directly to the mainline tree? Or
>>   both?
>> 
>> [1]: 3e2234: xen-netback: Handle foreign mapped pages on the guest RX path
>> 
>> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
>> Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
>> ---

Will test the patch tomorrow and see how this works out for me.

> The 'cleanup' of stale ubufs should be right after __pskb_pull_tail().

> This is the function that can 'consume frags' after all.

> Its not clear that you catch all cases, like skbs being purged in case
> of device dismantle.
Hmm that's quite important since a partial fix only makes the potential 
remaining cases even harder to spot and debug i'm afraid.

> I am not saying your patch is wrong, only that it adds yet an obscure
> thing with no comments. In two years, nobody will understand this.

--
Sander

Re: [PATCH net RFC] xen-netback: Fix grant ref resolution in RX path

[Ian Campbell]

On Tue, 2014-05-13 at 09:13 -0700, Eric Dumazet wrote:
> On Tue, 2014-05-13 at 15:31 +0100, Zoltan Kiss wrote:
> > The original series for reintroducing grant mapping for netback had a patch [1]
> > to handle receiving of packets from an another VIF. Grant copy on the receiving
> > side needs the grant ref of the page to set up the op.
> > The original patch assumed (wrongly) that the frags array haven't changed. In
> > the case reported by Sander, the sending guest sent a packet where the linear
> > buffer and the first frag were under PKT_PROT_LEN (=128) bytes.
> > xenvif_tx_submit() then pulled up the linear area to 128 bytes, and ditched the
> > first frag. The receiving side had an off-by-one problem when gathered the grant
> > refs.
> > This patch fixes that by checking whether the actual frag's page pointer is the
> > same as the page in the original frag list. It can handle any kind of changes on
> > the original frags array, like:
> > - removing granted frags from the beginning or the end
> > - adding local pages to the frags list
> > To keep it optimized to the most common cases, it doesn't handle when the order
> > of the original frags changed. That would require ubuf to be reseted to the
> > beginning of the chain (skb_shinfo(skb)->destructor_arg), and reiterating
> > through the list every time.
> > 
> > OPEN QUESTIONS:
> > - Is it a safe assumption that nothing changes the order of the original frags?
> >   Removing them from the array or injecting new pages anywhere is not a problem.
> > - I used UINT_MAX as a kind of INVALID_GRANT_REF, however there is no such thing
> >   in the grant mapping API. Should we codify this or is it better if we just
> >   find another way to distinguish whether a frag is local or not?
> > - Should this fix go to David's net tree or directly to the mainline tree? Or
> >   both?
> > 
> > [1]: 3e2234: xen-netback: Handle foreign mapped pages on the guest RX path
> > 
> > Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> > Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
> > ---
> 
> 
> The 'cleanup' of stale ubufs should be right after __pskb_pull_tail().
> 
> This is the function that can 'consume frags' after all.

That would be OK for the call to __pskb_pull_tail in netback itself --
but what about any other calls from other bits of the network stack
which don't know about this driver-specific data structure?

> Its not clear that you catch all cases, like skbs being purged in case
> of device dismantle.

Doesn't that go through the normal skb destroy path, as opposed to
manipulating an existing skb?

> I am not saying your patch is wrong, only that it adds yet an obscure
> thing with no comments. In two years, nobody will understand this.

Agreed.

Ian.

Re: [PATCH net RFC] xen-netback: Fix grant ref resolution in RX path

[David Vrabel]

On 14/05/14 09:32, Ian Campbell wrote:
>> I am not saying your patch is wrong, only that it adds yet an obscure
>> thing with no comments. In two years, nobody will understand this.
> 
> Agreed.

The long term solution is to make grant copy use the supplied VA to copy
to/from which would mean we no longer need to pass the grant ref around
to do the copy.

This is non-trivial and would required an updated Xen.

It would also fix other use cases (such as a domU providing an iSCSI
target which is used as the block device for a local blkback).

David

Re: [PATCH net RFC] xen-netback: Fix grant ref resolution in RX path

[Zoltan Kiss]

On 13/05/14 17:13, Eric Dumazet wrote:
> On Tue, 2014-05-13 at 15:31 +0100, Zoltan Kiss wrote:
>> The original series for reintroducing grant mapping for netback had a patch [1]
>> to handle receiving of packets from an another VIF. Grant copy on the receiving
>> side needs the grant ref of the page to set up the op.
>> The original patch assumed (wrongly) that the frags array haven't changed. In
>> the case reported by Sander, the sending guest sent a packet where the linear
>> buffer and the first frag were under PKT_PROT_LEN (=128) bytes.
>> xenvif_tx_submit() then pulled up the linear area to 128 bytes, and ditched the
>> first frag. The receiving side had an off-by-one problem when gathered the grant
>> refs.
>> This patch fixes that by checking whether the actual frag's page pointer is the
>> same as the page in the original frag list. It can handle any kind of changes on
>> the original frags array, like:
>> - removing granted frags from the beginning or the end
>> - adding local pages to the frags list
>> To keep it optimized to the most common cases, it doesn't handle when the order
>> of the original frags changed. That would require ubuf to be reseted to the
>> beginning of the chain (skb_shinfo(skb)->destructor_arg), and reiterating
>> through the list every time.
>>
>> OPEN QUESTIONS:
>> - Is it a safe assumption that nothing changes the order of the original frags?
>>    Removing them from the array or injecting new pages anywhere is not a problem.
>> - I used UINT_MAX as a kind of INVALID_GRANT_REF, however there is no such thing
>>    in the grant mapping API. Should we codify this or is it better if we just
>>    find another way to distinguish whether a frag is local or not?
>> - Should this fix go to David's net tree or directly to the mainline tree? Or
>>    both?
>>
>> [1]: 3e2234: xen-netback: Handle foreign mapped pages on the guest RX path
>>
>> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
>> Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
>> ---
>
>
> The 'cleanup' of stale ubufs should be right after __pskb_pull_tail().
We can't fix every place in the kernel where frags might be changed, 
especially with a netback specific stuff, so unfortunately that won't work
>
> This is the function that can 'consume frags' after all.
>
> Its not clear that you catch all cases, like skbs being purged in case
> of device dismantle.
We need this list for two reason:
a) give back the pages to the sending guest (kfree/skb_copy_ubufs)
b) find out the grant refs when the skb is sent to another vif
b) is handled by this patch. For a) netback doesn't mind if granted 
frags were removed and/or local ones were injected. It only needs to 
give back the pages, it doesn't matter how the skb ended up.
The only other problematic point if frags are passed around between 
skbs, I'll write a separate mail about it.

>
> I am not saying your patch is wrong, only that it adds yet an obscure
> thing with no comments. In two years, nobody will understand this.
I agree, in v3 there will be more comment lines than actual new code :)

Re: [PATCH net RFC] xen-netback: Fix grant ref resolution in RX path

[Eric Dumazet]

On Wed, 2014-05-14 at 12:12 +0100, Zoltan Kiss wrote:
> On 13/05/14 17:13, Eric Dumazet wrote:
> >
> > The 'cleanup' of stale ubufs should be right after __pskb_pull_tail().
> We can't fix every place in the kernel where frags might be changed, 
> especially with a netback specific stuff, so unfortunately that won't work
> >
> > This is the function that can 'consume frags' after all.
> >
> > Its not clear that you catch all cases, like skbs being purged in case
> > of device dismantle.
> We need this list for two reason:
> a) give back the pages to the sending guest (kfree/skb_copy_ubufs)

So If networking stack takes a reference on one particular frag, or
releases a ref on a frag, how is it done exactly ?

Are you 'giving back' page to the guest later because ubuf chain is now
obsolete ???


> b) find out the grant refs when the skb is sent to another vif
> b) is handled by this patch. For a) netback doesn't mind if granted 
> frags were removed and/or local ones were injected. It only needs to 
> give back the pages, it doesn't matter how the skb ended up.

> The only other problematic point if frags are passed around between 
> skbs, I'll write a separate mail about it.

Well, it seems this is exactly the point.
You give 'very special skb' to the stack, expecting stack will never do
any frag games, like in skb_try_coalesce()

Re: [PATCH net RFC] xen-netback: Fix grant ref resolution in RX path

[Zoltan Kiss]

On 14/05/14 12:35, Eric Dumazet wrote:
> On Wed, 2014-05-14 at 12:12 +0100, Zoltan Kiss wrote:
>> On 13/05/14 17:13, Eric Dumazet wrote:
>>>
>>> The 'cleanup' of stale ubufs should be right after __pskb_pull_tail().
>> We can't fix every place in the kernel where frags might be changed,
>> especially with a netback specific stuff, so unfortunately that won't work
>>>
>>> This is the function that can 'consume frags' after all.
>>>
>>> Its not clear that you catch all cases, like skbs being purged in case
>>> of device dismantle.
>> We need this list for two reason:
>> a) give back the pages to the sending guest (kfree/skb_copy_ubufs)
>
> So If networking stack takes a reference on one particular frag, or
> releases a ref on a frag, how is it done exactly ?
Releasing a frag (= put_page) is not a problem, it will be given back to 
the guest when the skb is freed up. But taking an another ref is bad, 
because the destructor_arg is on shinfo, an another skb won't have the 
information about how to release that frag. That's why skb_orphan_frags 
exist, it triggers a local copy of the frags to be done, and release 
them back, so later on this feature doesn't cause a trouble. skb_clone 
does that as first thing, for example.
But of course it should be carefully checked that functions which place 
frags into another frags arrays should call orphan_frags, e.g. I guess 
skb_shift does such thing. That's what I intend to start another thread 
about.
>
> Are you 'giving back' page to the guest later because ubuf chain is now
> obsolete ???
>
>
>> b) find out the grant refs when the skb is sent to another vif
>> b) is handled by this patch. For a) netback doesn't mind if granted
>> frags were removed and/or local ones were injected. It only needs to
>> give back the pages, it doesn't matter how the skb ended up.
>
>> The only other problematic point if frags are passed around between
>> skbs, I'll write a separate mail about it.
>
> Well, it seems this is exactly the point.
> You give 'very special skb' to the stack, expecting stack will never do
> any frag games, like in skb_try_coalesce()
That's another function which needs an orphan_frag. Btw. usually these 
functions doesn't touch these packets, as they don't reach the upper 
layers of the stack, unless a frontend wants a socket connection to the 
backend domain.

Re: [PATCH net RFC] xen-netback: Fix grant ref resolution in RX path

[Ian Campbell]

On Wed, 2014-05-14 at 14:25 +0100, Zoltan Kiss wrote:

> But of course it should be carefully checked that functions which place 
> frags into another frags arrays should call orphan_frags, e.g. I guess 
> skb_shift does such thing. That's what I intend to start another thread 
> about.

I see Eric has posted a fix for skb_try_coalesce(), but is that likely
to be acceptable for 3.15? How many other similar patches are we
expecting and/or how long do you think this careful checking will take?

Given that we are now at 3.15-rc5 I think we need to decide how to
proceed, either push ahead fixing all these issues or (partially) revert
this netback feature and try again for 3.16.

Ian.

Re: [PATCH net RFC] xen-netback: Fix grant ref resolution in RX path

[Sander Eikelenboom]

Wednesday, May 14, 2014, 6:40:39 PM, you wrote:


> On Wed, 2014-05-14 at 14:25 +0100, Zoltan Kiss wrote:

>> But of course it should be carefully checked that functions which place 
>> frags into another frags arrays should call orphan_frags, e.g. I guess 
>> skb_shift does such thing. That's what I intend to start another thread 
>> about.

> I see Eric has posted a fix for skb_try_coalesce(), but is that likely
> to be acceptable for 3.15? How many other similar patches are we
> expecting and/or how long do you think this careful checking will take?

> Given that we are now at 3.15-rc5 I think we need to decide how to
> proceed, either push ahead fixing all these issues or (partially) revert
> this netback feature and try again for 3.16.

> Ian.

Hi Zoltan,

Thanks for the patch, and for what it's worth:

With Zoltan's v3 patch, I have ran the same tests as i did while reporting the 
regression and when that worked out ok stressed netback and netfront some extra this afternoon.
The patch fixes the regression for me, no corruption, no problems with SSL 
connections anymore and i haven't spotted anything else.

So as a fix for the immediate regression you may put on a tested-by :-)

On the discussion about the rest of the fallout i don't think i contribute that 
much.

--
Sander

Re: [PATCH net RFC] xen-netback: Fix grant ref resolution in RX path

[Zoltan Kiss]

On 14/05/14 17:40, Ian Campbell wrote:
>
> On Wed, 2014-05-14 at 14:25 +0100, Zoltan Kiss wrote:
>
>> But of course it should be carefully checked that functions which place
>> frags into another frags arrays should call orphan_frags, e.g. I guess
>> skb_shift does such thing. That's what I intend to start another thread
>> about.
>
> I see Eric has posted a fix for skb_try_coalesce(), but is that likely
> to be acceptable for 3.15? How many other similar patches are we
> expecting and/or how long do you think this careful checking will take?
>
> Given that we are now at 3.15-rc5 I think we need to decide how to
> proceed, either push ahead fixing all these issues or (partially) revert
> this netback feature and try again for 3.16.
I guess these problems apply to KVM as well, and it haven't caused them 
problem so far, so I guess the problem shouldn't be that big. I assume 
they are using this feature to send fragmented skbs out from a guest, 
but I might be wrong.
Anyway, I think I'll check tomorrow if there are further places to worry 
about, but I don't think if there are any of them which are easily 
reproducible.

Zoli

Re: [PATCH net RFC] xen-netback: Fix grant ref resolution in RX path

[Ian Campbell]

On Wed, 2014-05-14 at 19:44 +0100, Zoltan Kiss wrote:
> On 14/05/14 17:40, Ian Campbell wrote:
> >
> > On Wed, 2014-05-14 at 14:25 +0100, Zoltan Kiss wrote:
> >
> >> But of course it should be carefully checked that functions which place
> >> frags into another frags arrays should call orphan_frags, e.g. I guess
> >> skb_shift does such thing. That's what I intend to start another thread
> >> about.
> >
> > I see Eric has posted a fix for skb_try_coalesce(), but is that likely
> > to be acceptable for 3.15? How many other similar patches are we
> > expecting and/or how long do you think this careful checking will take?
> >
> > Given that we are now at 3.15-rc5 I think we need to decide how to
> > proceed, either push ahead fixing all these issues or (partially) revert
> > this netback feature and try again for 3.16.
> I guess these problems apply to KVM as well, and it haven't caused them 
> problem so far, so I guess the problem shouldn't be that big. I assume 
> they are using this feature to send fragmented skbs out from a guest, 
> but I might be wrong.
> Anyway, I think I'll check tomorrow if there are further places to worry 
> about, but I don't think if there are any of them which are easily 
> reproducible.

OK, thanks. Assuming Eric's patch is acceptable for 3.15 it sounds like
that is the way to go.

Ian.

Re: [PATCH net RFC] xen-netback: Fix grant ref resolution in RX path

[Ian Campbell]

On Wed, 2014-05-14 at 12:12 +0100, Zoltan Kiss wrote:
> > The 'cleanup' of stale ubufs should be right after __pskb_pull_tail().
> We can't fix every place in the kernel where frags might be changed, 
> especially with a netback specific stuff, so unfortunately that won't work

Is it worth fixing up the ones in netback though so that the things
injected into the stack are consistent when we hand them over? It would
avoid some search overhead on the rx path at the other end I guess?
Perhaps not significant though.

Ian.

Re: [PATCH net RFC] xen-netback: Fix grant ref resolution in RX path

[Zoltan Kiss]

On 15/05/14 09:31, Ian Campbell wrote:
> On Wed, 2014-05-14 at 12:12 +0100, Zoltan Kiss wrote:
>>> The 'cleanup' of stale ubufs should be right after __pskb_pull_tail().
>> We can't fix every place in the kernel where frags might be changed,
>> especially with a netback specific stuff, so unfortunately that won't work
>
> Is it worth fixing up the ones in netback though so that the things
> injected into the stack are consistent when we hand them over? It would
> avoid some search overhead on the rx path at the other end I guess?
> Perhaps not significant though.

There are plans to remove that unconditional pull, as it damages 
performance. It is better to use during checksum setup maybe_pull_tail, 
and pull up whatever is needed for checksum setup (this is already done, 
partially). A sensible netfront would send the header in the first slot 
anyway, so netback won't pull, and it definitely won't pull the whole 
first frag.

Regards,

Zoli