>-----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(). Mike > >> 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 ��.n��������+%������w��{.n�����{���fk��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
