bogus wrap check in xen-netback

bogus wrap check in xen-netback

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 1222 bytes --]

With commit 48856286b64e ("xen/netback: shutdown the ring if it contains garbage.") a new check was added to xen-netback, which triggers for me.

Since there are bugs in ring buffer handling I tried to trigger them earlier by changing RING_IDX from u32 to u16. Now I found another one, and I wonder if the error below could potentially also hit with u32:

...
[  624.186492] br0: port 3(vif2.0) entered forwarding state
[  624.186522] br0: port 3(vif2.0) entered forwarding state
[  680.865398] vif vif-1-0 vif1.0: Impossible number of requests. req_prod 0, req_cons 65400, size 256
[  680.865402] vif vif-1-0 vif1.0: fatal error; disabling device
[  680.865495] br0: port 2(vif1.0) entered disabled state
[  689.433849] vif vif-2-0 vif2.0: Impossible number of requests. req_prod 0, req_cons 65527, size 256
[  689.433857] vif vif-2-0 vif2.0: fatal error; disabling device
[  689.433945] br0: port 3(vif2.0) entered disabled state
[  690.930512] pktgen: Packet Generator for packet performance testing. Version: 2.75
...

What exactly is that check in xenvif_tx_build_gops trying to achieve? Subtracting a non-zero value from zero will always create something larger than XEN_NETIF_TX_RING_SIZE.

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: bogus wrap check in xen-netback

[Juergen Gross]

On 25/04/18 09:19, Olaf Hering wrote:
> With commit 48856286b64e ("xen/netback: shutdown the ring if it contains garbage.") a new check was added to xen-netback, which triggers for me.
> 
> Since there are bugs in ring buffer handling I tried to trigger them earlier by changing RING_IDX from u32 to u16. Now I found another one, and I wonder if the error below could potentially also hit with u32:
> 
> ...
> [  624.186492] br0: port 3(vif2.0) entered forwarding state
> [  624.186522] br0: port 3(vif2.0) entered forwarding state
> [  680.865398] vif vif-1-0 vif1.0: Impossible number of requests. req_prod 0, req_cons 65400, size 256
> [  680.865402] vif vif-1-0 vif1.0: fatal error; disabling device
> [  680.865495] br0: port 2(vif1.0) entered disabled state
> [  689.433849] vif vif-2-0 vif2.0: Impossible number of requests. req_prod 0, req_cons 65527, size 256
> [  689.433857] vif vif-2-0 vif2.0: fatal error; disabling device
> [  689.433945] br0: port 3(vif2.0) entered disabled state
> [  690.930512] pktgen: Packet Generator for packet performance testing. Version: 2.75
> ...
> 
> What exactly is that check in xenvif_tx_build_gops trying to achieve? Subtracting a non-zero value from zero will always create something larger than XEN_NETIF_TX_RING_SIZE.

Why? (u16)0 - (u16)65400 == 136


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: bogus wrap check in xen-netback

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 457 bytes --]

Am Wed, 25 Apr 2018 09:28:38 +0200
schrieb Juergen Gross <jgross@suse.com>:

> Why? (u16)0 - (u16)65400 == 136

My helloworld.c shows that ushort gets promoted to uint, unless it is done like that:

-               if (queue->tx.sring->req_prod - queue->tx.req_cons >
-                   XEN_NETIF_TX_RING_SIZE) {
+               idx = queue->tx.sring->req_prod - queue->tx.req_cons; 
+               if ( idx > XEN_NETIF_TX_RING_SIZE) {

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: bogus wrap check in xen-netback

[Wei Liu]

On Wed, Apr 25, 2018 at 09:39:24AM +0200, Olaf Hering wrote:
> Am Wed, 25 Apr 2018 09:28:38 +0200
> schrieb Juergen Gross <jgross@suse.com>:
> 
> > Why? (u16)0 - (u16)65400 == 136
> 
> My helloworld.c shows that ushort gets promoted to uint, unless it is done like that:
> 
> -               if (queue->tx.sring->req_prod - queue->tx.req_cons >
> -                   XEN_NETIF_TX_RING_SIZE) {
> +               idx = queue->tx.sring->req_prod - queue->tx.req_cons; 
> +               if ( idx > XEN_NETIF_TX_RING_SIZE) {

Do you have the full diff of your changes? I don't follow why integer
conversion works differently if you write the code like this. As far as
I can tell the type of LHS didn't change.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: bogus wrap check in xen-netback

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 1308 bytes --]

Am Wed, 25 Apr 2018 09:59:23 +0100
schrieb Wei Liu <wei.liu2@citrix.com>:

> Do you have the full diff of your changes? 

Not right now. But without 'val', or val being uint, the same error happens in f():

#include <stdio.h>
void f(void)
{
        unsigned short req_prod = 0, req_cons = 65400;
        unsigned short val;
        val = req_prod - req_cons;
        printf("req_prod - req_cons %u\n", val);
        printf("req_prod - req_cons %x\n", val);
}

int main(void)
{
#if 1
        unsigned nr_ents = 0x100U, req_prod_pvt = 0x14U, rsp_cons = 0xffffffeeU, req_prod = 0xfffffffeU;
        unsigned rx_target = 0x40U, qlen = 0x1aU;
#else
        unsigned nr_ents = 0x100U, req_prod_pvt = 0x00U, rsp_cons = 0xffffffeeU, req_prod = 0xfffffffeU;
        unsigned rx_target = 0x40U, qlen = 0x1aU;
#endif
        printf("batch_target %u, skb_queue_len %u, rx_target %u\n", rx_target - (req_prod_pvt - rsp_cons), qlen, rx_target);
        printf("nr_ents %u\n", nr_ents);
        printf("req_prod_pvt - rsp_cons %u\n", req_prod_pvt - rsp_cons);
        printf("req_prod_pvt - req_prod %u\n", req_prod_pvt - req_prod);
        printf("%u\n", nr_ents - (req_prod_pvt - rsp_cons));
        printf("%u\n", nr_ents - (req_prod_pvt - rsp_cons));
        f();
        return 0;
}

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: bogus wrap check in xen-netback

[Jan Beulich]

>>> On 25.04.18 at 11:04, <olaf@aepfle.de> wrote:
> Am Wed, 25 Apr 2018 09:59:23 +0100
> schrieb Wei Liu <wei.liu2@citrix.com>:
> 
>> Do you have the full diff of your changes? 
> 
> Not right now. But without 'val', or val being uint, the same error happens 
> in f():
> 
> #include <stdio.h>
> void f(void)
> {
>         unsigned short req_prod = 0, req_cons = 65400;
>         unsigned short val;
>         val = req_prod - req_cons;
>         printf("req_prod - req_cons %u\n", val);
>         printf("req_prod - req_cons %x\n", val);
> }

Well, of course - anything more narrow than int will be promoted to int first.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: bogus wrap check in xen-netback

[Wei Liu]

On Wed, Apr 25, 2018 at 11:04:26AM +0200, Olaf Hering wrote:
> Am Wed, 25 Apr 2018 09:59:23 +0100
> schrieb Wei Liu <wei.liu2@citrix.com>:
> 
> > Do you have the full diff of your changes? 
> 
> Not right now. But without 'val', or val being uint, the same error happens in f():
> 
> #include <stdio.h>
> void f(void)
> {
>         unsigned short req_prod = 0, req_cons = 65400;
>         unsigned short val;
>         val = req_prod - req_cons;
>         printf("req_prod - req_cons %u\n", val);
>         printf("req_prod - req_cons %x\n", val);
> }

What Jan said.

Integer promotion makes unsigned short into unsigned int first then do
the calculation. Assigning the result to val truncates it back to
unsigned short.

For the original code, idx is of type unsigned int. No promotion or
truncation is needed so the end result is correct.

Wei.

> 
> int main(void)
> {
> #if 1
>         unsigned nr_ents = 0x100U, req_prod_pvt = 0x14U, rsp_cons = 0xffffffeeU, req_prod = 0xfffffffeU;
>         unsigned rx_target = 0x40U, qlen = 0x1aU;
> #else
>         unsigned nr_ents = 0x100U, req_prod_pvt = 0x00U, rsp_cons = 0xffffffeeU, req_prod = 0xfffffffeU;
>         unsigned rx_target = 0x40U, qlen = 0x1aU;
> #endif
>         printf("batch_target %u, skb_queue_len %u, rx_target %u\n", rx_target - (req_prod_pvt - rsp_cons), qlen, rx_target);
>         printf("nr_ents %u\n", nr_ents);
>         printf("req_prod_pvt - rsp_cons %u\n", req_prod_pvt - rsp_cons);
>         printf("req_prod_pvt - req_prod %u\n", req_prod_pvt - req_prod);
>         printf("%u\n", nr_ents - (req_prod_pvt - rsp_cons));
>         printf("%u\n", nr_ents - (req_prod_pvt - rsp_cons));
>         f();
>         return 0;
> }



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: bogus wrap check in xen-netback

[Jan Beulich]

>>> On 25.04.18 at 12:06, <wei.liu2@citrix.com> wrote:
> On Wed, Apr 25, 2018 at 11:04:26AM +0200, Olaf Hering wrote:
>> Am Wed, 25 Apr 2018 09:59:23 +0100
>> schrieb Wei Liu <wei.liu2@citrix.com>:
>> 
>> > Do you have the full diff of your changes? 
>> 
>> Not right now. But without 'val', or val being uint, the same error happens 
> in f():
>> 
>> #include <stdio.h>
>> void f(void)
>> {
>>         unsigned short req_prod = 0, req_cons = 65400;
>>         unsigned short val;
>>         val = req_prod - req_cons;
>>         printf("req_prod - req_cons %u\n", val);
>>         printf("req_prod - req_cons %x\n", val);
>> }
> 
> What Jan said.
> 
> Integer promotion makes unsigned short into unsigned int first then do
> the calculation.

Not exactly - it promotes to plain (i.e. signed) int.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: bogus wrap check in xen-netback

[Wei Liu]

On Wed, Apr 25, 2018 at 04:26:06AM -0600, Jan Beulich wrote:
> >>> On 25.04.18 at 12:06, <wei.liu2@citrix.com> wrote:
> > On Wed, Apr 25, 2018 at 11:04:26AM +0200, Olaf Hering wrote:
> >> Am Wed, 25 Apr 2018 09:59:23 +0100
> >> schrieb Wei Liu <wei.liu2@citrix.com>:
> >> 
> >> > Do you have the full diff of your changes? 
> >> 
> >> Not right now. But without 'val', or val being uint, the same error happens 
> > in f():
> >> 
> >> #include <stdio.h>
> >> void f(void)
> >> {
> >>         unsigned short req_prod = 0, req_cons = 65400;
> >>         unsigned short val;
> >>         val = req_prod - req_cons;
> >>         printf("req_prod - req_cons %u\n", val);
> >>         printf("req_prod - req_cons %x\n", val);
> >> }
> > 
> > What Jan said.
> > 
> > Integer promotion makes unsigned short into unsigned int first then do
> > the calculation.
> 
> Not exactly - it promotes to plain (i.e. signed) int.

My bad. Yes, they are converted to int, not unsigned int.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: bogus wrap check in xen-netback

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 328 bytes --]

Am Wed, 25 Apr 2018 11:31:25 +0100
schrieb Wei Liu <wei.liu2@citrix.com>:

> My bad. Yes, they are converted to int, not unsigned int.

Hopefully that happens only if the target is int, not if all involved variables are short.

Unless there are objections I will prepare a patch to deal with RING_IDX being u16.

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: bogus wrap check in xen-netback

[Wei Liu]

On Wed, Apr 25, 2018 at 12:35:11PM +0200, Olaf Hering wrote:
> Am Wed, 25 Apr 2018 11:31:25 +0100
> schrieb Wei Liu <wei.liu2@citrix.com>:
> 
> > My bad. Yes, they are converted to int, not unsigned int.
> 
> Hopefully that happens only if the target is int, not if all involved variables are short.
> 
> Unless there are objections I will prepare a patch to deal with RING_IDX being u16.
> 

RING_IDX is a type defined in the public header -- I don't think you can
change that at all. You don't know what third party drivers rely on
that.

If you want to change the type locally in netback, then why? Aren't you
just debugging?

In that light, I don't see a point having a patch to deal with u16.

Wei.


> Olaf



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel