Linux Kernel CAN development ()

Rostislav Lisovy | 28 Jun 2012 19:07

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

This ematch makes it possible to classify CAN frames (AF_CAN) according
to their identifiers. This functionality can not be easily achieved with
existing classifiers, such as u32, because CAN identifier is always stored
in native endianness, whereas u32 expects Network byte order.
---
Changes from the last time:
  * Integrated remarks received from the mailing list
  * Array containing rules (rules_raw) is dynamically allocated
  * When processing a rule during configuration, CAN_EFF_FLAG in mask
    determines if we care about the value of CAN_EFF_FLAG in an identifier
    -- i.e. when CAN_EFF_FLAG is set in the mask, the rule will be added
    as SFF or EFF only depending on CAN_EFF_FLAG value in the identifier.
    If CAN_EFF_FLAG is 0 in the mask, the rule will be added as both SFF
    and EFF.

---
 include/linux/can.h     |    3 +
 include/linux/pkt_cls.h |    5 +-
 net/sched/Kconfig       |   10 ++
 net/sched/Makefile      |    1 +
 net/sched/em_canid.c    |  247 +++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 264 insertions(+), 2 deletions(-)
 create mode 100644 net/sched/em_canid.c

diff --git a/include/linux/can.h b/include/linux/can.h
index 9a19bcb..08d1610 100644
--- a/include/linux/can.h
+++ b/include/linux/can.h
 <at>  <at>  -38,6 +38,9  <at>  <at> 
  */

(Continue reading)

headers

Oliver Hartkopp | 29 Jun 2012 17:44

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

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))

(Continue reading)

headers

Oliver Hartkopp | 2 Jul 2012 07:04

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:
> 
> (..)
> 
>> +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,

(Continue reading)

headers

Rostislav Lisovy | 2 Jul 2012 16:12

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

Hello Oliver;

On Fri, 2012-06-29 at 17:44 +0200, Oliver Hartkopp wrote: 
> What about a zero length check here?
> 
> 	if (!len)
> 		return -EINVAL;
> 

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

The main reason I didn't define the tcf_ematch_ops.datalen field is
because the documentation says it is "length of expected configuration
data" (not "minimal").
For the sake of possible future changes of the built-in length checking,
I will do the check by myself -- I will add the zero-length check (at
least all checks will be at the same place).

(Continue reading)