Re: [PATCH V4 for-next 3/4] IB/core: Export ib_create/destroy_flow through uverbs

[Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]

On Sun, Sep 01, 2013 at 12:18:00PM +0300, Matan Barak wrote:
> >How will user space know what comp_mask values the kernel will
> >support?
> 
> struct ib_uverbs_create_flow_resp contains a comp_mask value. This value
> should contain the supported comp_masks fields.
> Currently, we don't support any extensions, so we zero this field by
> doing "memset(&resp, 0, sizeof(resp));"
> 
> I suggest returning an error and setting the response comp_mask field.

But this is inconsistent. The comp_mask field should work the same in
both directions, so if you want to return an error then the kernel
should also never return a comp_mask that user space can't support.

All that is hard, which is why the approach for the verbs extensions
was that everyone always sets the maximum comp_mask they support, and
receivers ignore what they don't understand.

>> The notion that was established in the verbs patches is that extra
>> structure fields are ignored by old software.

> I'm not aware of any such concrete example. Could you please point
> out ?

It was established during the discussion, I guess I now need to check
that the proposed patches followed it.

> I think it's safer to return an error. Imagine that ibv_modify_qp
> was extended for some crucial field, say IB_QP_AV2. Creating a QP
> without indicating its AV (or AV2 in that case) is an invalid
> behavior. This example is a bit artificial, though some future
> extensions could have such mandatory fields.
> The user should do ibv_query_device before requesting features that
> might be unsupported.

No, you are mixing things. The comp_mask should only be used to
indicate the memory in the structure was initialized to something
valid (which might be a no-action initialization)

It should never be used to indicate that the receiver must do
something with the data.

Requesting a fictional AV2 must be done via some other means that has
a suitable failure mode, eg check the device caps before setting an
actionable value.

At least in uverbs the notion should be that comp_mask can be reset if
the soname was bumped.

Further, we shouldn't burden userspace with setting a 'correct'
comp_mask. That is a horrible ABI, at least a 3 on the 'Rusty Scale'.

The correct comp_mask should always match the fields that are
initialized by the caller. That is simple to explain and easy to audit
for correctness.

Anything else is too hard for users.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html