Re: [PATCH 1/1] xen-netfront: do not cast grant table reference to signed short

Re: [PATCH 1/1] xen-netfront: do not cast grant table reference to signed short

[Dongli Zhang]

Hi David and Jan,

I did more testing on the code. Casting to either (long) or (unsigned long)
would be fine.

However, there is still an issue that ref is of type uint32_t and
IS_ERR_VALUE((unsigned long)ref) would not return true when ref=-ENOSPC (or
other error code).

IS_ERR_VALUE((long)ref) would return false as well.

The solution is to cast ref to (int) first as follows:

-   BUG_ON((signed short)ref < 0);
+   WARN_ON_ONCE(IS_ERR_VALUE((unsigned long)(int)ref));


David, I am very sorry for this error and I will be careful the next time.
Would you please let me know if I should resend a new patch or an incremental
based on previous one at 
https://git.kernel.org/cgit/linux/kernel/git/davem/net.git?

Thank you very much!

Dongli Zhang


----- Original Message -----
From: davem@davemloft.net
To: dongli.zhang@oracle.com
Cc: linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org, netdev@vger.kernel.org, boris.ostrovsky@oracle.com, david.vrabel@citrix.com, jgross@suse.com
Sent: Tuesday, November 1, 2016 4:06:27 AM GMT +08:00 Beijing / Chongqing / Hong Kong / Urumqi
Subject: Re: [PATCH 1/1] xen-netfront: do not cast grant table reference to signed short

From: Dongli Zhang <dongli.zhang@oracle.com>
Date: Mon, 31 Oct 2016 13:38:29 +0800

> While grant reference is of type uint32_t, xen-netfront erroneously casts
> it to signed short in BUG_ON().
> 
> This would lead to the xen domU panic during boot-up or migration when it
> is attached with lots of paravirtual devices.
> 
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>

Since this is consistent with how the macros in linux/err.h handle "is
this an error" checks, this change looks good to me.

Applied, thanks.

Re: [PATCH 1/1] xen-netfront: do not cast grant table reference to signed short

[Dongli Zhang]

Hi David and Jan,

I did more testing on the code. Casting to either (long) or (unsigned long)
would be fine.

However, there is still an issue that ref is of type uint32_t and
IS_ERR_VALUE((unsigned long)ref) would not return true when ref=-ENOSPC (or
other error code).

IS_ERR_VALUE((long)ref) would return false as well.

The solution is to cast ref to (int) first as follows:

-   BUG_ON((signed short)ref < 0);
+   WARN_ON_ONCE(IS_ERR_VALUE((unsigned long)(int)ref));


David, I am very sorry for this error and I will be careful the next time.
Would you please let me know if I should resend a new patch or an incremental
based on previous one at 
https://git.kernel.org/cgit/linux/kernel/git/davem/net.git?

Thank you very much!

Dongli Zhang


----- Original Message -----
From: davem@davemloft.net
To: dongli.zhang@oracle.com
Cc: linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org, netdev@vger.kernel.org, boris.ostrovsky@oracle.com, david.vrabel@citrix.com, jgross@suse.com
Sent: Tuesday, November 1, 2016 4:06:27 AM GMT +08:00 Beijing / Chongqing / Hong Kong / Urumqi
Subject: Re: [PATCH 1/1] xen-netfront: do not cast grant table reference to signed short

From: Dongli Zhang <dongli.zhang@oracle.com>
Date: Mon, 31 Oct 2016 13:38:29 +0800

> While grant reference is of type uint32_t, xen-netfront erroneously casts
> it to signed short in BUG_ON().
> 
> This would lead to the xen domU panic during boot-up or migration when it
> is attached with lots of paravirtual devices.
> 
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>

Since this is consistent with how the macros in linux/err.h handle "is
this an error" checks, this change looks good to me.

Applied, thanks.

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

Re: [PATCH 1/1] xen-netfront: do not cast grant table reference to signed short

[Dongli Zhang]

Hi David and Jan,

I did more testing on the code. Casting to either (long) or (unsigned long)
would be fine.

However, there is still an issue that ref is of type uint32_t and
IS_ERR_VALUE((unsigned long)ref) would not return true when ref=-ENOSPC (or
other error code).

IS_ERR_VALUE((long)ref) would return false as well.

The solution is to cast ref to (int) first as follows:

-   BUG_ON((signed short)ref < 0);
+   WARN_ON_ONCE(IS_ERR_VALUE((unsigned long)(int)ref));


David, I am very sorry for this error and I will be careful the next time.
Would you please let me know if I should resend a new patch or an incremental
based on previous one at 
https://git.kernel.org/cgit/linux/kernel/git/davem/net.git?

Thank you very much!

Dongli Zhang


----- Original Message -----
From: davem@davemloft.net
To: dongli.zhang@oracle.com
Cc: linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org, netdev@vger.kernel.org, boris.ostrovsky@oracle.com, david.vrabel@citrix.com, jgross@suse.com
Sent: Tuesday, November 1, 2016 4:06:27 AM GMT +08:00 Beijing / Chongqing / Hong Kong / Urumqi
Subject: Re: [PATCH 1/1] xen-netfront: do not cast grant table reference to signed short

From: Dongli Zhang <dongli.zhang@oracle.com>
Date: Mon, 31 Oct 2016 13:38:29 +0800

> While grant reference is of type uint32_t, xen-netfront erroneously casts
> it to signed short in BUG_ON().
> 
> This would lead to the xen domU panic during boot-up or migration when it
> is attached with lots of paravirtual devices.
> 
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>

Since this is consistent with how the macros in linux/err.h handle "is
this an error" checks, this change looks good to me.

Applied, thanks.

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

Re: [PATCH 1/1] xen-netfront: do not cast grant table reference to signed short

[David Miller]

From: Dongli Zhang <dongli.zhang@oracle.com>
Date: Mon, 31 Oct 2016 21:46:09 -0700 (PDT)

> David, I am very sorry for this error and I will be careful the next time.
> Would you please let me know if I should resend a new patch or an incremental
> based on previous one at 
> https://git.kernel.org/cgit/linux/kernel/git/davem/net.git?

Incremental, please.

Re: [PATCH 1/1] xen-netfront: do not cast grant table reference to signed short

[David Miller]

From: Dongli Zhang <dongli.zhang@oracle.com>
Date: Mon, 31 Oct 2016 21:46:09 -0700 (PDT)

> David, I am very sorry for this error and I will be careful the next time.
> Would you please let me know if I should resend a new patch or an incremental
> based on previous one at 
> https://git.kernel.org/cgit/linux/kernel/git/davem/net.git?

Incremental, please.

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

Re: [PATCH 1/1] xen-netfront: do not cast grant table reference to signed short

[David Miller]

From: Dongli Zhang <dongli.zhang@oracle.com>
Date: Mon, 31 Oct 2016 13:38:29 +0800

> While grant reference is of type uint32_t, xen-netfront erroneously casts
> it to signed short in BUG_ON().
> 
> This would lead to the xen domU panic during boot-up or migration when it
> is attached with lots of paravirtual devices.
> 
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>

Since this is consistent with how the macros in linux/err.h handle "is
this an error" checks, this change looks good to me.

Applied, thanks.

Re: [PATCH 1/1] xen-netfront: do not cast grant table reference to signed short

[David Miller]

From: Dongli Zhang <dongli.zhang@oracle.com>
Date: Mon, 31 Oct 2016 13:38:29 +0800

> While grant reference is of type uint32_t, xen-netfront erroneously casts
> it to signed short in BUG_ON().
> 
> This would lead to the xen domU panic during boot-up or migration when it
> is attached with lots of paravirtual devices.
> 
> Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>

Since this is consistent with how the macros in linux/err.h handle "is
this an error" checks, this change looks good to me.

Applied, thanks.

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

Re: [PATCH 1/1] xen-netfront: do not cast grant table reference to signed short

[Jan Beulich]

>>> On 31.10.16 at 09:28, <dongli.zhang@oracle.com> wrote:
>> >                  ref = gnttab_claim_grant_reference(&queue->gref_rx_head);
>> > -                BUG_ON((signed short)ref < 0);
>> > +                WARN_ON_ONCE(IS_ERR_VALUE((unsigned long)ref));
> 
>> You really need to cast to plain (or signed) long here - casting to
>> unsigned long will work only in 32-bit configurations, as otherwise
>> you lose the sign of the value.
> 
> Seems the sign of value would not be lost since casting to unsigned long will
> use signed extension. Is my understanding wrong or did I miss anything?

ref being of type grant_ref_t, which is a typedef of uint32_t, I can't
see how the conversion to unsigned long would be using sign
extension. Did you look at the generated code? (If ref was of a signed
type, I don't think the original author would have found a need to cast
it to signed short.)

> I have verified this with both sample userspace helloworld program
> and a kernel module.

Are you saying you did see the warning trigger with a 64-bit kernel?
I'd be very surprised, but of course I may be overlooking something.

Jan

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

Re: [PATCH 1/1] xen-netfront: do not cast grant table reference to signed short

[Dongli Zhang]

> >                  ref = gnttab_claim_grant_reference(&queue->gref_rx_head);
> > -                BUG_ON((signed short)ref < 0);
> > +                WARN_ON_ONCE(IS_ERR_VALUE((unsigned long)ref));

> You really need to cast to plain (or signed) long here - casting to
> unsigned long will work only in 32-bit configurations, as otherwise
> you lose the sign of the value.

Seems the sign of value would not be lost since casting to unsigned long will
use signed extension. Is my understanding wrong or did I miss anything? I have
verified this with both sample userspace helloworld program and a kernel
module.

> And then just issuing a warning here is insufficient, I think: Either
> you follow David's line of thought assuming that no failure here is

Keeping this can help debug xen-netfront.

> possible at all (in which case the BUG_ON() can be ditched without
> replacement), or you follow your original one (which matches mine)
> that we can't exclude an error here because of a bug elsewhere,
> in which case this either needs to stay BUG_ON() or should be
> followed by some form of bailing out (so that the bad ref won't get
> stored, preventing its later use from causing further damage).

The reason I use warning instead BUG_ON is that Linus suggested use
WARN_ON_ONCE() in a previous email:
http://lkml.iu.edu/hypermail/linux/kernel/1610.0/00878.html

I would change to BUG_ON() if it is OK to you.

Thank you very much!

Dongli Zhang 


----- Original Message -----
From: JBeulich@suse.com
To: dongli.zhang@oracle.com
Cc: david.vrabel@citrix.com, xen-devel@lists.xenproject.org, boris.ostrovsky@oracle.com, JGross@suse.com, linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Sent: Monday, October 31, 2016 3:48:03 PM GMT +08:00 Beijing / Chongqing / Hong Kong / Urumqi
Subject: Re: [Xen-devel] [PATCH 1/1] xen-netfront: do not cast grant table reference to signed short

>>> On 31.10.16 at 06:38, <dongli.zhang@oracle.com> wrote:
> --- a/drivers/net/xen-netfront.c
> +++ b/drivers/net/xen-netfront.c
> @@ -304,7 +304,7 @@ static void xennet_alloc_rx_buffers(struct netfront_queue *queue)
>  		queue->rx_skbs[id] = skb;
>  
>  		ref = gnttab_claim_grant_reference(&queue->gref_rx_head);
> -		BUG_ON((signed short)ref < 0);
> +		WARN_ON_ONCE(IS_ERR_VALUE((unsigned long)ref));

You really need to cast to plain (or signed) long here - casting to
unsigned long will work only in 32-bit configurations, as otherwise
you lose the sign of the value.

And then just issuing a warning here is insufficient, I think: Either
you follow David's line of thought assuming that no failure here is
possible at all (in which case the BUG_ON() can be ditched without
replacement), or you follow your original one (which matches mine)
that we can't exclude an error here because of a bug elsewhere,
in which case this either needs to stay BUG_ON() or should be
followed by some form of bailing out (so that the bad ref won't get
stored, preventing its later use from causing further damage).

Jan

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

Re: [PATCH 1/1] xen-netfront: do not cast grant table reference to signed short

[Jan Beulich]

>>> On 31.10.16 at 06:38, <dongli.zhang@oracle.com> wrote:
> --- a/drivers/net/xen-netfront.c
> +++ b/drivers/net/xen-netfront.c
> @@ -304,7 +304,7 @@ static void xennet_alloc_rx_buffers(struct netfront_queue *queue)
>  		queue->rx_skbs[id] = skb;
>  
>  		ref = gnttab_claim_grant_reference(&queue->gref_rx_head);
> -		BUG_ON((signed short)ref < 0);
> +		WARN_ON_ONCE(IS_ERR_VALUE((unsigned long)ref));

You really need to cast to plain (or signed) long here - casting to
unsigned long will work only in 32-bit configurations, as otherwise
you lose the sign of the value.

And then just issuing a warning here is insufficient, I think: Either
you follow David's line of thought assuming that no failure here is
possible at all (in which case the BUG_ON() can be ditched without
replacement), or you follow your original one (which matches mine)
that we can't exclude an error here because of a bug elsewhere,
in which case this either needs to stay BUG_ON() or should be
followed by some form of bailing out (so that the bad ref won't get
stored, preventing its later use from causing further damage).

Jan

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

[PATCH 1/1] xen-netfront: do not cast grant table reference to signed short

[Dongli Zhang]

While grant reference is of type uint32_t, xen-netfront erroneously casts
it to signed short in BUG_ON().

This would lead to the xen domU panic during boot-up or migration when it
is attached with lots of paravirtual devices.

Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
 drivers/net/xen-netfront.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index e17879d..189a28d 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -304,7 +304,7 @@ static void xennet_alloc_rx_buffers(struct netfront_queue *queue)
 		queue->rx_skbs[id] = skb;
 
 		ref = gnttab_claim_grant_reference(&queue->gref_rx_head);
-		BUG_ON((signed short)ref < 0);
+		WARN_ON_ONCE(IS_ERR_VALUE((unsigned long)ref));
 		queue->grant_rx_ref[id] = ref;
 
 		page = skb_frag_page(&skb_shinfo(skb)->frags[0]);
@@ -428,7 +428,7 @@ static void xennet_tx_setup_grant(unsigned long gfn, unsigned int offset,
 	id = get_id_from_freelist(&queue->tx_skb_freelist, queue->tx_skbs);
 	tx = RING_GET_REQUEST(&queue->tx, queue->tx.req_prod_pvt++);
 	ref = gnttab_claim_grant_reference(&queue->gref_tx_head);
-	BUG_ON((signed short)ref < 0);
+	WARN_ON_ONCE(IS_ERR_VALUE((unsigned long)ref));
 
 	gnttab_grant_foreign_access_ref(ref, queue->info->xbdev->otherend_id,
 					gfn, GNTMAP_readonly);
-- 
2.7.4

[PATCH 1/1] xen-netfront: do not cast grant table reference to signed short

[Dongli Zhang]

While grant reference is of type uint32_t, xen-netfront erroneously casts
it to signed short in BUG_ON().

This would lead to the xen domU panic during boot-up or migration when it
is attached with lots of paravirtual devices.

Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
 drivers/net/xen-netfront.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index e17879d..189a28d 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -304,7 +304,7 @@ static void xennet_alloc_rx_buffers(struct netfront_queue *queue)
 		queue->rx_skbs[id] = skb;
 
 		ref = gnttab_claim_grant_reference(&queue->gref_rx_head);
-		BUG_ON((signed short)ref < 0);
+		WARN_ON_ONCE(IS_ERR_VALUE((unsigned long)ref));
 		queue->grant_rx_ref[id] = ref;
 
 		page = skb_frag_page(&skb_shinfo(skb)->frags[0]);
@@ -428,7 +428,7 @@ static void xennet_tx_setup_grant(unsigned long gfn, unsigned int offset,
 	id = get_id_from_freelist(&queue->tx_skb_freelist, queue->tx_skbs);
 	tx = RING_GET_REQUEST(&queue->tx, queue->tx.req_prod_pvt++);
 	ref = gnttab_claim_grant_reference(&queue->gref_tx_head);
-	BUG_ON((signed short)ref < 0);
+	WARN_ON_ONCE(IS_ERR_VALUE((unsigned long)ref));
 
 	gnttab_grant_foreign_access_ref(ref, queue->info->xbdev->otherend_id,
 					gfn, GNTMAP_readonly);
-- 
2.7.4


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