| Message ID | 1620382961-69701-1-git-send-email-jiapeng.chong@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | RDMA/mlx4: Remove unnessesary check in mlx4_ib_modify_wq() | expand |
On Fri, May 07, 2021 at 06:22:41PM +0800, Jiapeng Chong wrote: > cur_state and new_state are enums and when GCC considers > them as unsigned, the conditions are never met. > > Clean up the following smatch warning: > > drivers/infiniband/hw/mlx4/qp.c:4258 mlx4_ib_modify_wq() warn: unsigned > 'cur_state' is never less than zero. > > Reported-by: Abaci Robot <abaci@linux.alibaba.com> > Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com> > --- > drivers/infiniband/hw/mlx4/qp.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Thanks, Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
On Fri, May 07, 2021 at 06:22:41PM +0800, Jiapeng Chong wrote: > cur_state and new_state are enums and when GCC considers > them as unsigned, the conditions are never met. > > Clean up the following smatch warning: > > drivers/infiniband/hw/mlx4/qp.c:4258 mlx4_ib_modify_wq() warn: unsigned > 'cur_state' is never less than zero. > > Reported-by: Abaci Robot <abaci@linux.alibaba.com> > Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com> > --- > drivers/infiniband/hw/mlx4/qp.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c > index 92ddbcc..162aa59 100644 > --- a/drivers/infiniband/hw/mlx4/qp.c > +++ b/drivers/infiniband/hw/mlx4/qp.c > @@ -4255,8 +4255,7 @@ int mlx4_ib_modify_wq(struct ib_wq *ibwq, struct ib_wq_attr *wq_attr, > ibwq->state; > new_state = wq_attr_mask & IB_WQ_STATE ? wq_attr->wq_state : cur_state; > > - if (cur_state < IB_WQS_RESET || cur_state > IB_WQS_ERR || > - new_state < IB_WQS_RESET || new_state > IB_WQS_ERR) > + if (cur_state > IB_WQS_ERR || new_state > IB_WQS_ERR) > return -EINVAL; Actually the more robust change will be to move this change to the ib_uverbs_ex_modify_wq(). Thanks > > if ((new_state == IB_WQS_RDY) && (cur_state == IB_WQS_ERR)) > -- > 1.8.3.1 >
On Fri, May 07, 2021 at 06:22:41PM +0800, Jiapeng Chong wrote: > cur_state and new_state are enums and when GCC considers > them as unsigned, the conditions are never met. But doesn't gcc consider enums to be 'int' as the standard requires? This change looks really sketchy to me, cur_state and new_state are both userspace controlled data. We should not make assumptions about the underlying signedness of an enum when validating user data. Jason
On Tue, May 11, 2021 at 02:43:02PM -0300, Jason Gunthorpe wrote: > On Fri, May 07, 2021 at 06:22:41PM +0800, Jiapeng Chong wrote: > > cur_state and new_state are enums and when GCC considers > > them as unsigned, the conditions are never met. > > But doesn't gcc consider enums to be 'int' as the standard requires? Ohh, I missed that. > > This change looks really sketchy to me, cur_state and new_state are > both userspace controlled data. We should not make assumptions about > the underlying signedness of an enum when validating user data. I still think that the right change should be in ib_uverbs_ex_modify_wq(), so both mlx4 and mlx5 will be protected. Thanks > > Jason
diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c index 92ddbcc..162aa59 100644 --- a/drivers/infiniband/hw/mlx4/qp.c +++ b/drivers/infiniband/hw/mlx4/qp.c @@ -4255,8 +4255,7 @@ int mlx4_ib_modify_wq(struct ib_wq *ibwq, struct ib_wq_attr *wq_attr, ibwq->state; new_state = wq_attr_mask & IB_WQ_STATE ? wq_attr->wq_state : cur_state; - if (cur_state < IB_WQS_RESET || cur_state > IB_WQS_ERR || - new_state < IB_WQS_RESET || new_state > IB_WQS_ERR) + if (cur_state > IB_WQS_ERR || new_state > IB_WQS_ERR) return -EINVAL; if ((new_state == IB_WQS_RDY) && (cur_state == IB_WQS_ERR))
cur_state and new_state are enums and when GCC considers them as unsigned, the conditions are never met. Clean up the following smatch warning: drivers/infiniband/hw/mlx4/qp.c:4258 mlx4_ib_modify_wq() warn: unsigned 'cur_state' is never less than zero. Reported-by: Abaci Robot <abaci@linux.alibaba.com> Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com> --- drivers/infiniband/hw/mlx4/qp.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)