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

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:

(..)

quoted

+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;

???

quoted

+	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?

quoted

+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