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

Commit Message

Or Gerlitz Sept. 11, 2013, 3:10 p.m. UTC

On 11/09/2013 17:04, Yann Droneaud wrote:

[...]
>> +	int i;
>> +	int kern_attr_size;
>> +
>> +	if (out_len < sizeof(resp))
>> +		return -ENOSPC;
>> +
>> +	if (copy_from_user(&cmd, buf, sizeof(cmd)))
>> +		return -EFAULT;
>> +
>> +	if (cmd.comp_mask)
>> +		return -EINVAL;
>> +
>> +	if ((cmd.flow_attr.type == IB_FLOW_ATTR_SNIFFER &&
>> +	     !capable(CAP_NET_ADMIN)) || !capable(CAP_NET_RAW))
>> +		return -EPERM;
>> +
>> +	if (cmd.flow_attr.num_of_specs < 0 ||
> num_of_specs is unsigned ...

sure, will send fix that eliminates this redundant check
>
>> +	    cmd.flow_attr.num_of_specs > IB_FLOW_SPEC_SUPPORT_LAYERS)
>> +		return -EINVAL;
>> +
>> +	kern_attr_size = cmd.flow_attr.size - sizeof(cmd) -
>> +			 sizeof(struct ib_uverbs_cmd_hdr_ex);
>> +
> Please, no under/overflow. Check that cmd.flow_attr.size is bigger than
> sizeof(cmd) + sizeof(struct ib_uverbs_cmd_hdr_ex) before computing kern_attr_size.

We've got feedback on this code from Sean and Roland and Matan Barak 
from our team addressed it to the bit.
Matan is OOO for couple of days, plus we're having few holidays here.  
This way or another (accepting the comments
and sending fixes or arguing with you over the list), all your feedback 
will be addressed before 3.12-rc2 is out, thanks for
looking over this!

>
>> +	if (cmd.flow_attr.size < 0 || cmd.flow_attr.size > in_len ||
> just like num_of_specs, it's unsigned !

ditto here, will fix.

[...]

>> +		struct ib_kern_spec_ipv4    ipv4;
>> +		struct ib_kern_spec_tcp_udp tcp_udp;
>> +	};
>> +};
>> +
> [Aside note: no IPv6 spec ?]

indeed, but note that the way the specs are defines allow for adding 
future spec types w.o breaking anything,
its done in TLV (Type-Length-Value) manner, see the change log of the IB 
core main patch  for flow steering
319a441 IB/core: Add receive flow steering support

Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Yann Droneaud Sept. 11, 2013, 3:25 p.m. UTC | #1

Le 11.09.2013 17:10, Or Gerlitz a écrit :
> On 11/09/2013 17:04, Yann Droneaud wrote:
> 
> [...]
>>> +	int i;
>>> +	int kern_attr_size;
>>> +
>>> +	if (out_len < sizeof(resp))
>>> +		return -ENOSPC;
>>> +
>>> +	if (copy_from_user(&cmd, buf, sizeof(cmd)))
>>> +		return -EFAULT;
>>> +
>>> +	if (cmd.comp_mask)
>>> +		return -EINVAL;
>>> +
>>> +	if ((cmd.flow_attr.type == IB_FLOW_ATTR_SNIFFER &&
>>> +	     !capable(CAP_NET_ADMIN)) || !capable(CAP_NET_RAW))
>>> +		return -EPERM;
>>> +
>>> +	if (cmd.flow_attr.num_of_specs < 0 ||
>> num_of_specs is unsigned ...
> 
> sure, will send fix that eliminates this redundant check
>> 
>>> +	    cmd.flow_attr.num_of_specs > IB_FLOW_SPEC_SUPPORT_LAYERS)
>>> +		return -EINVAL;
>>> +
>>> +	kern_attr_size = cmd.flow_attr.size - sizeof(cmd) -
>>> +			 sizeof(struct ib_uverbs_cmd_hdr_ex);
>>> +
>> Please, no under/overflow. Check that cmd.flow_attr.size is bigger 
>> than
>> sizeof(cmd) + sizeof(struct ib_uverbs_cmd_hdr_ex) before computing 
>> kern_attr_size.
> 
> We've got feedback on this code from Sean and Roland and Matan Barak
> from our team addressed it to the bit.
> Matan is OOO for couple of days, plus we're having few holidays here.
> This way or another (accepting the comments
> and sending fixes or arguing with you over the list), all your
> feedback will be addressed before 3.12-rc2 is out, thanks for
> looking over this!
> 

Thanks a lot.

>> 
>>> +	if (cmd.flow_attr.size < 0 || cmd.flow_attr.size > in_len ||
>> just like num_of_specs, it's unsigned !
> 
> ditto here, will fix.
> 
> [...]
> 
> diff --git a/include/uapi/rdma/ib_user_verbs.h
> b/include/uapi/rdma/ib_user_verbs.h
> index 61535aa..0b233c5 100644
> --- a/include/uapi/rdma/ib_user_verbs.h
> +++ b/include/uapi/rdma/ib_user_verbs.h
> @@ -86,7 +86,9 @@ enum {
> 
>>> +struct ib_kern_spec {
>>> +	union {
>>> +		struct {
>>> +			__u32 type;
>>> +			__u16 size;
>>> +			__u16 reserved;
>>> +		};
>>> +		struct ib_kern_spec_eth	    eth;
>>> +		struct ib_kern_spec_ipv4    ipv4;
>>> +		struct ib_kern_spec_tcp_udp tcp_udp;
>>> +	};
>>> +};
>>> +
>> [Aside note: no IPv6 spec ?]
> 
> indeed, but note that the way the specs are defines allow for adding
> future spec types w.o breaking anything,
> its done in TLV (Type-Length-Value) manner, see the change log of the
> IB core main patch  for flow steering
> 319a441 IB/core: Add receive flow steering support
> 

I've noticed the TLV nature of ib_kern_spec, but a bit too late, so my 
comments about + 1 and first element being handled differently from the 
others one are rather pointless.

I will try to provide a patch to propose another way of describing the 
data (same layout) before v3.12 release.
Hopefully I have more time since Linus hard drive broke ;)

Regards.

Patch

diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h
index 61535aa..0b233c5 100644
--- a/include/uapi/rdma/ib_user_verbs.h
+++ b/include/uapi/rdma/ib_user_verbs.h
@@ -86,7 +86,9 @@  enum {

>> +struct ib_kern_spec {
>> +	union {
>> +		struct {
>> +			__u32 type;
>> +			__u16 size;
>> +			__u16 reserved;
>> +		};
>> +		struct ib_kern_spec_eth	    eth;