Xen devel - Nov 2011 - [PATCH] linux-2.6.18/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);
 		}
 





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

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

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
>

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

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