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 <[email protected]>
---
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
>>> On 31.10.16 at 06:38, <[email protected]> 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
From: Dongli Zhang <[email protected]>
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 <[email protected]>
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.
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: [email protected]
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
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 <[email protected]>
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 <[email protected]>
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.
From: Dongli Zhang <[email protected]>
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.