[dpdk-dev] [PATCH] ixgbe: fix bad shift operation in ixgbe_set_pool_rx

[dpdk-dev] [PATCH] ixgbe: fix bad shift operation in ixgbe_set_pool_rx

[Tomasz Kulasek]

CID 13193 (#1 of 1): Bad bit shift operation (BAD_SHIFT)
large_shift: In expression 1 << pool, left shifting by more than 31 bits
has undefined behavior. The shift amount, pool, is at least 32.

This patch limits mask shift to be in range of 32 bit PFVFRE[1] register,
for pool > 31.

Fixes: fe3a45fd4104 ("ixgbe: add VMDq support")

Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 3f1ebc1..f676a64 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -4401,7 +4401,7 @@ ixgbe_set_pool_rx(struct rte_eth_dev *dev, uint16_t pool, uint8_t on)
 
 	addr = IXGBE_VFRE(pool >= ETH_64_POOLS/2);
 	reg = IXGBE_READ_REG(hw, addr);
-	val = bit1 << pool;
+	val = bit1 << (pool & 0x01F);
 
 	if (on)
 		reg |= val;
-- 
1.7.9.5

Re: [dpdk-dev] [PATCH] ixgbe: fix bad shift operation in ixgbe_set_pool_rx

[Lu, Wenzhuo]

Hi Tomasz,


> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tomasz Kulasek
> Sent: Friday, April 15, 2016 9:39 PM
> To: dev@dpdk.org
> Cc: Zhang, Helin; Ananyev, Konstantin
> Subject: [dpdk-dev] [PATCH] ixgbe: fix bad shift operation in ixgbe_set_pool_rx
> 
> CID 13193 (#1 of 1): Bad bit shift operation (BAD_SHIFT)
> large_shift: In expression 1 << pool, left shifting by more than 31 bits has
> undefined behavior. The shift amount, pool, is at least 32.
> 
> This patch limits mask shift to be in range of 32 bit PFVFRE[1] register, for pool >
> 31.
> 
> Fixes: fe3a45fd4104 ("ixgbe: add VMDq support")
> 
> Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
Acked-by: Wenzhuo Lu <wenzhuo.lu@intel.com>

Re: [dpdk-dev] [PATCH] ixgbe: fix bad shift operation in ixgbe_set_pool_rx

[Bruce Richardson]

On Fri, Apr 15, 2016 at 03:39:09PM +0200, Tomasz Kulasek wrote:
> CID 13193 (#1 of 1): Bad bit shift operation (BAD_SHIFT)
> large_shift: In expression 1 << pool, left shifting by more than 31 bits
> has undefined behavior. The shift amount, pool, is at least 32.
> 
> This patch limits mask shift to be in range of 32 bit PFVFRE[1] register,
> for pool > 31.
> 
> Fixes: fe3a45fd4104 ("ixgbe: add VMDq support")
> 
> Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
> ---
>  drivers/net/ixgbe/ixgbe_ethdev.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index 3f1ebc1..f676a64 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -4401,7 +4401,7 @@ ixgbe_set_pool_rx(struct rte_eth_dev *dev, uint16_t pool, uint8_t on)
>  
>  	addr = IXGBE_VFRE(pool >= ETH_64_POOLS/2);
>  	reg = IXGBE_READ_REG(hw, addr);
> -	val = bit1 << pool;
> +	val = bit1 << (pool & 0x01F);
>  
Are we sure this is the correct way to fix this. Rather than silently truncating
the pool value, are we not better to check our input parameters and return
EINVAL to the caller if pool overflows?

/Bruce

Re: [dpdk-dev] [PATCH] ixgbe: fix bad shift operation in ixgbe_set_pool_rx

[Kulasek, TomaszX]

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Thursday, April 21, 2016 15:52
> To: Kulasek, TomaszX <tomaszx.kulasek@intel.com>
> Cc: dev@dpdk.org; Zhang, Helin <helin.zhang@intel.com>; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] ixgbe: fix bad shift operation in
> ixgbe_set_pool_rx
> 
> On Fri, Apr 15, 2016 at 03:39:09PM +0200, Tomasz Kulasek wrote:
> > CID 13193 (#1 of 1): Bad bit shift operation (BAD_SHIFT)
> > large_shift: In expression 1 << pool, left shifting by more than 31
> > bits has undefined behavior. The shift amount, pool, is at least 32.
> >
> > This patch limits mask shift to be in range of 32 bit PFVFRE[1]
> > register, for pool > 31.
> >
> > Fixes: fe3a45fd4104 ("ixgbe: add VMDq support")
> >
> > Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
> > ---
> >  drivers/net/ixgbe/ixgbe_ethdev.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > index 3f1ebc1..f676a64 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > @@ -4401,7 +4401,7 @@ ixgbe_set_pool_rx(struct rte_eth_dev *dev,
> > uint16_t pool, uint8_t on)
> >
> >  	addr = IXGBE_VFRE(pool >= ETH_64_POOLS/2);

For pool in 0..31 PFVFRE[0] is used, for pool in 32..63, PFVFRE[1], but for second case, we set/unset (pool-32) bit in the register. Invalid value if pool > 63, but catching it doesn't solve a problem of possible overflow for pool > 31.

> >  	reg = IXGBE_READ_REG(hw, addr);
> > -	val = bit1 << pool;

Previous implementation expects that for shift operation will be used rol on 32 bit value, and the bits that slide off the end of the register are fed back into the spaces, eg. (bit1 << 33) == (bit1 << 1).
Pool value can be bigger than 31, and this is not an error while pool is smaller than 64.

Truncating pool value is clearer for me, than relay on obscure shift operation.

> > +	val = bit1 << (pool & 0x01F);
> >
> Are we sure this is the correct way to fix this. Rather than silently
> truncating the pool value, are we not better to check our input parameters
> and return EINVAL to the caller if pool overflows?
> 
> /Bruce

Tomasz

Re: [dpdk-dev] [PATCH] ixgbe: fix bad shift operation in ixgbe_set_pool_rx

[Bruce Richardson]

On Thu, Apr 21, 2016 at 03:44:03PM +0100, Kulasek, TomaszX wrote:
> 
> 
> > -----Original Message-----
> > From: Richardson, Bruce
> > Sent: Thursday, April 21, 2016 15:52
> > To: Kulasek, TomaszX <tomaszx.kulasek@intel.com>
> > Cc: dev@dpdk.org; Zhang, Helin <helin.zhang@intel.com>; Ananyev,
> > Konstantin <konstantin.ananyev@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH] ixgbe: fix bad shift operation in
> > ixgbe_set_pool_rx
> > 
> > On Fri, Apr 15, 2016 at 03:39:09PM +0200, Tomasz Kulasek wrote:
> > > CID 13193 (#1 of 1): Bad bit shift operation (BAD_SHIFT)
> > > large_shift: In expression 1 << pool, left shifting by more than 31
> > > bits has undefined behavior. The shift amount, pool, is at least 32.
> > >
> > > This patch limits mask shift to be in range of 32 bit PFVFRE[1]
> > > register, for pool > 31.
> > >
> > > Fixes: fe3a45fd4104 ("ixgbe: add VMDq support")
> > >
> > > Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
> > > ---
> > >  drivers/net/ixgbe/ixgbe_ethdev.c |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > index 3f1ebc1..f676a64 100644
> > > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > @@ -4401,7 +4401,7 @@ ixgbe_set_pool_rx(struct rte_eth_dev *dev,
> > > uint16_t pool, uint8_t on)
> > >
> > >  	addr = IXGBE_VFRE(pool >= ETH_64_POOLS/2);
> 
> For pool in 0..31 PFVFRE[0] is used, for pool in 32..63, PFVFRE[1], but for second case, we set/unset (pool-32) bit in the register. Invalid value if pool > 63, but catching it doesn't solve a problem of possible overflow for pool > 31.
> 
> > >  	reg = IXGBE_READ_REG(hw, addr);
> > > -	val = bit1 << pool;
> 
> Previous implementation expects that for shift operation will be used rol on 32 bit value, and the bits that slide off the end of the register are fed back into the spaces, eg. (bit1 << 33) == (bit1 << 1).
> Pool value can be bigger than 31, and this is not an error while pool is smaller than 64.
> 
> Truncating pool value is clearer for me, than relay on obscure shift operation.
>
Thanks for the explanation, that indeed does make it clearer.

However, all that detail is completely unclear to the reader of the function, so
perhaps we can clean up the code to make it more explicit what is happening.
For example:

        /* for pool >= 32, set bit in PFVFRE[1], otherwise PFVFRE[0] */
        if (pool >= ETH_64_POOLS)
                 return -EINVAL;
        else if (pool >= ETH_64_POOLS/2) {
                 addr = IXGBE_VFRE(1);
                 val = bit1 << (pool - 32);
        } else {
                addr = IXGBE_VFRE(0);
                val = bit1 << pool;
        }

        reg = IXGBE_READ_REG(hw, addr);

This should fix the issue and make the resulting code clearer, I think.

/Bruce

Re: [dpdk-dev] [PATCH] ixgbe: fix bad shift operation in ixgbe_set_pool_rx

[Kulasek, TomaszX]

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Thursday, April 21, 2016 17:28
> To: Kulasek, TomaszX <tomaszx.kulasek@intel.com>
> Cc: dev@dpdk.org; Zhang, Helin <helin.zhang@intel.com>; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] ixgbe: fix bad shift operation in
> ixgbe_set_pool_rx
> 
> On Thu, Apr 21, 2016 at 03:44:03PM +0100, Kulasek, TomaszX wrote:
> >
> >
> > > -----Original Message-----
> > > From: Richardson, Bruce
> > > Sent: Thursday, April 21, 2016 15:52
> > > To: Kulasek, TomaszX <tomaszx.kulasek@intel.com>
> > > Cc: dev@dpdk.org; Zhang, Helin <helin.zhang@intel.com>; Ananyev,
> > > Konstantin <konstantin.ananyev@intel.com>
> > > Subject: Re: [dpdk-dev] [PATCH] ixgbe: fix bad shift operation in
> > > ixgbe_set_pool_rx
> > >
> > > On Fri, Apr 15, 2016 at 03:39:09PM +0200, Tomasz Kulasek wrote:
> > > > CID 13193 (#1 of 1): Bad bit shift operation (BAD_SHIFT)
> > > > large_shift: In expression 1 << pool, left shifting by more than
> > > > 31 bits has undefined behavior. The shift amount, pool, is at least
> 32.
> > > >
> > > > This patch limits mask shift to be in range of 32 bit PFVFRE[1]
> > > > register, for pool > 31.
> > > >
> > > > Fixes: fe3a45fd4104 ("ixgbe: add VMDq support")
> > > >
> > > > Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
> > > > ---
> > > >  drivers/net/ixgbe/ixgbe_ethdev.c |    2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > > index 3f1ebc1..f676a64 100644
> > > > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > > > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > > > @@ -4401,7 +4401,7 @@ ixgbe_set_pool_rx(struct rte_eth_dev *dev,
> > > > uint16_t pool, uint8_t on)
> > > >
> > > >  	addr = IXGBE_VFRE(pool >= ETH_64_POOLS/2);
> >
> > For pool in 0..31 PFVFRE[0] is used, for pool in 32..63, PFVFRE[1], but
> for second case, we set/unset (pool-32) bit in the register. Invalid value
> if pool > 63, but catching it doesn't solve a problem of possible overflow
> for pool > 31.
> >
> > > >  	reg = IXGBE_READ_REG(hw, addr);
> > > > -	val = bit1 << pool;
> >
> > Previous implementation expects that for shift operation will be used
> rol on 32 bit value, and the bits that slide off the end of the register
> are fed back into the spaces, eg. (bit1 << 33) == (bit1 << 1).
> > Pool value can be bigger than 31, and this is not an error while pool is
> smaller than 64.
> >
> > Truncating pool value is clearer for me, than relay on obscure shift
> operation.
> >
> Thanks for the explanation, that indeed does make it clearer.
> 
> However, all that detail is completely unclear to the reader of the
> function, so perhaps we can clean up the code to make it more explicit
> what is happening.
> For example:
> 
>         /* for pool >= 32, set bit in PFVFRE[1], otherwise PFVFRE[0] */
>         if (pool >= ETH_64_POOLS)
>                  return -EINVAL;
>         else if (pool >= ETH_64_POOLS/2) {
>                  addr = IXGBE_VFRE(1);
>                  val = bit1 << (pool - 32);
>         } else {
>                 addr = IXGBE_VFRE(0);
>                 val = bit1 << pool;
>         }
> 
>         reg = IXGBE_READ_REG(hw, addr);
> 
> This should fix the issue and make the resulting code clearer, I think.
> 
> /Bruce

Yes, I got it. I will send v2.

Tomasz

[dpdk-dev] [PATCH v2] ixgbe: fix bad shift operation in ixgbe_set_pool_rx/tx

[Tomasz Kulasek]

Fix issue reported by Coverity.

Coverity ID 13193: Bad bit shift operation (BAD_SHIFT)
large_shift: In expression 1 << pool, left shifting by more than 31 bits
has undefined behavior. The shift amount, pool, is at least 32.

This patch is a rework of register addr selection logic and mask
computation to made it more readable and avoid bit overflow when 32 bit
value is shifted over its size for pool > 31.

Fixes: fe3a45fd4104 ("ixgbe: add VMDq support")

Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
---
v2:
 - joined two patches for same issue in tx/rx
 - added pool parameter checking for invalid value
 - reworked register selection logic and mask shift in more explicit way

 drivers/net/ixgbe/ixgbe_ethdev.c |   28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 3f1ebc1..eed2662 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -4399,9 +4399,19 @@ ixgbe_set_pool_rx(struct rte_eth_dev *dev, uint16_t pool, uint8_t on)
 	if (ixgbe_vmdq_mode_check(hw) < 0)
 		return -ENOTSUP;
 
-	addr = IXGBE_VFRE(pool >= ETH_64_POOLS/2);
+	if (pool >= ETH_64_POOLS)
+		return -EINVAL;
+
+	/* for pool >= 32, set bit in PFVFRE[1], otherwise PFVFRE[0] */
+	if (pool >= 32) {
+		addr = IXGBE_VFRE(1);
+		val = bit1 << (pool - 32);
+	} else {
+		addr = IXGBE_VFRE(0);
+		val = bit1 << pool;
+	}
+
 	reg = IXGBE_READ_REG(hw, addr);
-	val = bit1 << pool;
 
 	if (on)
 		reg |= val;
@@ -4426,9 +4436,19 @@ ixgbe_set_pool_tx(struct rte_eth_dev *dev, uint16_t pool, uint8_t on)
 	if (ixgbe_vmdq_mode_check(hw) < 0)
 		return -ENOTSUP;
 
-	addr = IXGBE_VFTE(pool >= ETH_64_POOLS/2);
+	if (pool >= ETH_64_POOLS)
+		return -EINVAL;
+
+	/* for pool >= 32, set bit in PFVFTE[1], otherwise PFVFTE[0] */
+	if (pool >= 32) {
+		addr = IXGBE_VFTE(1);
+		val = bit1 << (pool - 32);
+	} else {
+		addr = IXGBE_VFTE(0);
+		val = bit1 << pool;
+	}
+
 	reg = IXGBE_READ_REG(hw, addr);
-	val = bit1 << pool;
 
 	if (on)
 		reg |= val;
-- 
1.7.9.5

Re: [dpdk-dev] [PATCH v2] ixgbe: fix bad shift operation in ixgbe_set_pool_rx/tx

[Bruce Richardson]

On Fri, Apr 22, 2016 at 05:35:57PM +0200, Tomasz Kulasek wrote:
> Fix issue reported by Coverity.
> 
> Coverity ID 13193: Bad bit shift operation (BAD_SHIFT)
> large_shift: In expression 1 << pool, left shifting by more than 31 bits
> has undefined behavior. The shift amount, pool, is at least 32.
> 
> This patch is a rework of register addr selection logic and mask
> computation to made it more readable and avoid bit overflow when 32 bit
> value is shifted over its size for pool > 31.
> 
> Fixes: fe3a45fd4104 ("ixgbe: add VMDq support")
> 
> Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [dpdk-dev] [PATCH v2] ixgbe: fix bad shift operation in ixgbe_set_pool_rx/tx

[Bruce Richardson]

On Mon, Apr 25, 2016 at 11:35:44AM +0100, Bruce Richardson wrote:
> On Fri, Apr 22, 2016 at 05:35:57PM +0200, Tomasz Kulasek wrote:
> > Fix issue reported by Coverity.
> > 
> > Coverity ID 13193: Bad bit shift operation (BAD_SHIFT)
> > large_shift: In expression 1 << pool, left shifting by more than 31 bits
> > has undefined behavior. The shift amount, pool, is at least 32.
> > 
> > This patch is a rework of register addr selection logic and mask
> > computation to made it more readable and avoid bit overflow when 32 bit
> > value is shifted over its size for pool > 31.
> > 
> > Fixes: fe3a45fd4104 ("ixgbe: add VMDq support")
> > 
> > Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
> 
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> 
Applied to dpdk-next-net/rel_16_07

/Bruce