[PATCH] linux-2.6.18/netback: use correct index for invalidation in netbk_tx_check_mop()

[PATCH] linux-2.6.18/netback: use correct index for invalidation in netbk_tx_check_mop()

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 463 bytes --]

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/drivers/xen/netback/netback.c
+++ b/drivers/xen/netback/netback.c
@@ -1155,7 +1155,7 @@ static int netbk_tx_check_mop(struct sk_
 		pending_idx = *((u16 *)skb->data);
 		netif_idx_release(pending_idx);
 		for (j = start; j < i; j++) {
-			pending_idx = (unsigned long)shinfo->frags[i].page;
+			pending_idx = (unsigned long)shinfo->frags[j].page;
 			netif_idx_release(pending_idx);
 		}
 




[-- Attachment #2: xen-netback-check_mop-error.patch --]
[-- Type: text/plain, Size: 528 bytes --]

netback: use correct index for invalidation in netbk_tx_check_mop()

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/drivers/xen/netback/netback.c
+++ b/drivers/xen/netback/netback.c
@@ -1155,7 +1155,7 @@ static int netbk_tx_check_mop(struct sk_
 		pending_idx = *((u16 *)skb->data);
 		netif_idx_release(pending_idx);
 		for (j = start; j < i; j++) {
-			pending_idx = (unsigned long)shinfo->frags[i].page;
+			pending_idx = (unsigned long)shinfo->frags[j].page;
 			netif_idx_release(pending_idx);
 		}
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] linux-2.6.18/netback: use correct index for invalidation in netbk_tx_check_mop()

[Jan Beulich]

>>> On 18.11.11 at 14:15, "Jan Beulich" <JBeulich@suse.com> wrote:
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/drivers/xen/netback/netback.c
> +++ b/drivers/xen/netback/netback.c
> @@ -1155,7 +1155,7 @@ static int netbk_tx_check_mop(struct sk_
>  		pending_idx = *((u16 *)skb->data);
>  		netif_idx_release(pending_idx);
>  		for (j = start; j < i; j++) {
> -			pending_idx = (unsigned long)shinfo->frags[i].page;
> +			pending_idx = (unsigned long)shinfo->frags[j].page;
>  			netif_idx_release(pending_idx);
>  		}
>  

I'm sure you want to apply the equivalent to xen-netback to (there
it is in function xen_netbk_tx_check_gop()).

Jan

Re: [PATCH] linux-2.6.18/netback: use correct index for invalidation in netbk_tx_check_mop()

[Ian Campbell]

On Fri, 2011-11-18 at 13:25 +0000, Jan Beulich wrote:
> >>> On 18.11.11 at 14:15, "Jan Beulich" <JBeulich@suse.com> wrote:
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > --- a/drivers/xen/netback/netback.c
> > +++ b/drivers/xen/netback/netback.c
> > @@ -1155,7 +1155,7 @@ static int netbk_tx_check_mop(struct sk_
> >  		pending_idx = *((u16 *)skb->data);
> >  		netif_idx_release(pending_idx);
> >  		for (j = start; j < i; j++) {
> > -			pending_idx = (unsigned long)shinfo->frags[i].page;
> > +			pending_idx = (unsigned long)shinfo->frags[j].page;
> >  			netif_idx_release(pending_idx);
> >  		}
> >  
> 
> I'm sure you want to apply the equivalent to xen-netback to (there
> it is in function xen_netbk_tx_check_gop()).

Yes. I'll pick this up. Thanks,
Ian.

> 
> Jan
> 

Re: [PATCH] linux-2.6.18/netback: use correct index for invalidation in netbk_tx_check_mop()

[Laszlo Ersek]

Hi,

On 11/18/11 14:15, Jan Beulich wrote:

> --- a/drivers/xen/netback/netback.c
> +++ b/drivers/xen/netback/netback.c
> @@ -1155,7 +1155,7 @@ static int netbk_tx_check_mop(struct sk_
>   		pending_idx = *((u16 *)skb->data);
>   		netif_idx_release(pending_idx);
>   		for (j = start; j<  i; j++) {
> -			pending_idx = (unsigned long)shinfo->frags[i].page;
> +			pending_idx = (unsigned long)shinfo->frags[j].page;
>   			netif_idx_release(pending_idx);
>   		}

Please excuse the uneducated question: what could be the consequences of using the wrong index here? I notice that with the fix, frags[i].page is never touched, while without the fix, it is the only one that's released.

I'm asking because we have an elusive bug: netloop sometimes crashes in skb_remove_foreign_references(). As of 1125:985b8f62df25, the crash happens on line 118:

   116                  vaddr = kmap_skb_frag(&skb_shinfo(skb)->frags[i]);
   117                  off = skb_shinfo(skb)->frags[i].page_offset;
   118                  memcpy(page_address(page) + off,
   119                         vaddr + off,
   120                         skb_shinfo(skb)->frags[i].size);

because the PTE for the vaddr returned by kmap_skb_frag() for the first frag is 0.

It appears somehow related to NFS and closing/reopening TCP connections for NFS.

Thanks!
Laszlo

Re: [PATCH] linux-2.6.18/netback: use correct index for invalidation in netbk_tx_check_mop()

[Jan Beulich]

>>> On 18.11.11 at 17:25, Laszlo Ersek <lersek@redhat.com> wrote:
> On 11/18/11 14:15, Jan Beulich wrote:
> 
>> --- a/drivers/xen/netback/netback.c
>> +++ b/drivers/xen/netback/netback.c
>> @@ -1155,7 +1155,7 @@ static int netbk_tx_check_mop(struct sk_
>>   		pending_idx = *((u16 *)skb->data);
>>   		netif_idx_release(pending_idx);
>>   		for (j = start; j<  i; j++) {
>> -			pending_idx = (unsigned long)shinfo->frags[i].page;
>> +			pending_idx = (unsigned long)shinfo->frags[j].page;
>>   			netif_idx_release(pending_idx);
>>   		}
> 
> Please excuse the uneducated question: what could be the consequences of 
> using the wrong index here? I notice that with the fix, frags[i].page is 
> never touched, while without the fix, it is the only one that's released.

The index correlating to i gets released right before the loop. The
consequences of using the wrong index in the loop are - afaict -
that guests may cause netback (and perhaps Dom0 as a whole) to
run out of certain resources (grant table entries come to mind).

> I'm asking because we have an elusive bug: netloop sometimes crashes in 
> skb_remove_foreign_references(). As of 1125:985b8f62df25, the crash happens 
> on line 118:
> 
>    116                  vaddr = kmap_skb_frag(&skb_shinfo(skb)->frags[i]);
>    117                  off = skb_shinfo(skb)->frags[i].page_offset;
>    118                  memcpy(page_address(page) + off,
>    119                         vaddr + off,
>    120                         skb_shinfo(skb)->frags[i].size);
> 
> because the PTE for the vaddr returned by kmap_skb_frag() for the first frag 
> is 0.
> 
> It appears somehow related to NFS and closing/reopening TCP connections for 
> NFS.

Sorry, I don't see any connection to the above, but then again I didn't
find the bug above due to actual misbehavior, but just due to having
worked on that code while moving our patch set forward to 3.2-rc.

Jan