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

Kulasek, TomaszX tomaszx.kulasek at intel.com
Thu Apr 21 16:44:03 CEST 2016



> -----Original Message-----
> From: Richardson, Bruce
> Sent: Thursday, April 21, 2016 15:52
> To: Kulasek, TomaszX <tomaszx.kulasek at intel.com>
> Cc: dev at dpdk.org; Zhang, Helin <helin.zhang at intel.com>; Ananyev,
> Konstantin <konstantin.ananyev at 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 at 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


More information about the dev mailing list