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

[Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>]

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.

[...]

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

Or.
--
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