On Tue, Apr 10, 2018 at 06:29:13PM +0300, Matan Barak wrote:
> On 10/04/2018 18:18, Jason Gunthorpe wrote:
> >On Tue, Apr 10, 2018 at 06:03:29PM +0300, Matan Barak wrote:
> >>On 09/04/2018 20:24, Jason Gunthorpe wrote:
> >>>On Mon, Apr 09, 2018 at 02:18:53PM +0300, Dan Carpenter wrote:
> >>>>Hello Aviad Yehezkel,
> >>>>
> >>>>The patch c6475a0bca30: "IB/mlx5: Add implementation for create and
> >>>>destroy action_xfrm" from Mar 28, 2018, leads to the following static
> >>>>checker warning:
> >>>>
> >>>> drivers/infiniband/hw/mlx5/main.c:3256 mlx5_ib_create_flow_action_esp()
> >>>> error: uninitialized symbol 'action_flags'.
> >>>>
> >>>>drivers/infiniband/hw/mlx5/main.c
> >>>> 3359 static struct ib_flow_action *
> >>>> 3360 mlx5_ib_create_flow_action_esp(struct ib_device *device,
> >>>> 3361 const struct ib_flow_action_attrs_esp *attr,
> >>>> 3362 struct uverbs_attr_bundle *attrs)
> >>>> 3363 {
> >>>> 3364 struct mlx5_ib_dev *mdev = to_mdev(device);
> >>>> 3365 struct ib_uverbs_flow_action_esp_keymat_aes_gcm *aes_gcm;
> >>>> 3366 struct mlx5_accel_esp_xfrm_attrs accel_attrs = {};
> >>>> 3367 struct mlx5_ib_flow_action *action;
> >>>> 3368 u64 action_flags;
> >>>> ^^^^^^^^^^^^
> >>>> 3369 u64 flags;
> >>>> 3370 int err = 0;
> >>>> 3371
> >>>> 3372 if (IS_UVERBS_COPY_ERR(uverbs_copy_from(&action_flags, attrs,
> >>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >>>> 3373 MLX5_IB_ATTR_CREATE_FLOW_ACTION_FLAGS)))
> >>>> 3374 return ERR_PTR(-EFAULT);
> >>>> 3375
> >>>> 3376 if (action_flags >= (MLX5_FLOW_ACTION_ESP_CREATE_LAST_SUPPORTED << 1))
> >>>> ^^^^^^^^^^^^
> >>>>
> >>>>The problem is that IS_UVERBS_COPY_ERR() treats -ENOENT as A-OK but if
> >>>>we hit that error then uverbs_copy_from() is a no-op...
> >>>
> >>>It needs to be uverbs_copy_from_or_zero and wrapping in a macro is so
> >>>obviously hard to use the macro needs to go too.
> >>>
> >>>uverbs_zcopy_opt() or something
> >>>
> >>>I assume the intent was action_flags is zero'd if not present.. Matan?
> >>>
> >>
> >>That attribute is currently marked as mandatory, so you actually can't get
> >>-ENOENT here (as you'll fail earlier in the parsing stage).
> >
> >If ENONET is not possible here then what is the point of
> >IB_UVERBS_COPY_ERR?
> >
>
> Generally, uverbs_copy_from can fail if it can't copy the data
> (copy_from_user returns -EFAULT in that case). If we deep dive further,
> there's an optimization here and that attribute will be passed inlined, so
> even -EFAULT can't happen. However, since how this attribute is passed
> shouldn't be a concern for the verbs developer, I prefer to explicitly write
> a code that checks if the copy was done successfully without considering the
> attribute's size.
But since the attribute is mandatory and ENOENT isn't possible it
should just be
err = uverbs_copy_from(&action_flags, attrs,
MLX5_IB_ATTR_CREATE_FLOW_ACTION_FLAGS);
if (err)
return err;
IS_UVERBS_COPY_ERR only makes sense in places where ENOENT can happen
and only in those places where the calling code is handling the ENOENT
directly. It is a pretty ugly macro really, we should get rid of it.
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
