Re: [PATCH net-next] em_canid: Ematch rule to match CAN frames according to their identifiers

[Oliver Hartkopp <socketcan@hartkopp.net>]

One additional remark:

When applying your patch on Dave Millers net-next tree there are some offsets
in the patch ...

Please send the next patch based on the net-next tree, as this would be the
tree, it will be applied to. See URLs at:

http://git.kernel.org/?p=linux/kernel/git/davem/net-next.git

Tnx!

Oliver

On 29.06.2012 17:44, Oliver Hartkopp wrote:

> Hello Rostislav,
> 
> looks really good now.
> 
> 1. Your Signed-off-by: is missing.
> 
> 2. One remark to a removed length check:
> 
> (..)
> 
>> +static int em_canid_change(struct tcf_proto *tp, void *data, int len,
>> +			  struct tcf_ematch *m)
>> +{
>> +	struct can_filter *conf = data; /* Array with rules,
>> +					 * fixed size EM_CAN_RULES_SIZE
>> +					 */
>> +	struct canid_match *cm;
>> +	struct canid_match *cm_old = (struct canid_match *) m->data;
>> +	int i;
>> +	int rulescnt;
>> +
> 
> 
> What about a zero length check here?
> 
> 	if (!len)
> 		return -EINVAL;
> 
> ???
> 
>> +	if (len % sizeof(struct can_filter))
>> +		return -EINVAL;
>> +
>> +	if (len > sizeof(struct can_filter) * EM_CAN_RULES_MAX)
>> +		return -EINVAL;
>> +
>> +	rulescnt = len / sizeof(struct can_filter);
>> +
>> +	cm = kzalloc(sizeof(struct canid_match) + sizeof(struct can_filter) *
>> +		rulescnt, GFP_KERNEL);
>> +	if (!cm)
>> +		return -ENOMEM;
> 
> 
> The length could alternatively be checked here too
> 
> http://lxr.linux.no/#linux+v3.4.4/net/sched/ematch.c#L235
> 
> if em->ops->datalen is set.
> 
> But here's no
> 
> 	.datalen = sizeof(struct can_filter),
> 
> defined, right?
> 
>> +static struct tcf_ematch_ops em_canid_ops = {
>> +	.kind	  = TCF_EM_CANID,
>> +	.change	  = em_canid_change,
>> +	.match	  = em_canid_match,
>> +	.destroy  = em_canid_destroy,
>> +	.dump	  = em_canid_dump,
>> +	.owner	  = THIS_MODULE,
>> +	.link	  = LIST_HEAD_INIT(em_canid_ops.link)
>> +};
> 
> 
> Regards,
> Oliver
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html