On 11/04/2018 23:14, Ruhl, Michael J wrote:
-----Original Message-----
From: linux-rdma-owner@xxxxxxxxxxxxxxx [mailto:linux-rdma-
owner@xxxxxxxxxxxxxxx] On Behalf Of Matan Barak
Sent: Tuesday, April 10, 2018 11:29 AM
To: Jason Gunthorpe <jgg@xxxxxxxx>
Cc: Dan Carpenter <dan.carpenter@xxxxxxxxxx>; aviadye@xxxxxxxxxxxx; linux-
rdma@xxxxxxxxxxxxxxx
Subject: Re: [bug report] IB/mlx5: Add implementation for create and destroy
action_xfrm
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.
The distinction between the mandatory and the optional seems to get lost
in the method area. Would it be reasonable to have specific functions for
coping one vs. the other?
Then the code could be optimized for that difference.
It would also eliminate code like this:
/* this is a mandatory option, so I don't need to use the general function */
copy_an_attribute().
I think that should be our next logical step for the infrastructure.
When considering type-safe API, we might want to have bunch of
uverbs_copy/get/find mini-functions and check that they are used
correctly according to the spec declarations.
In this case, uverbs_copy_from_zopt (copy if available and zero if not
available) seems like a nice and easier to understand replacement function.
Mike
Matan
Jason
Matan
--
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
--
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
