[PATCH v2 1/2] can: fix multiple delivery of a single CAN frame for overlapping CAN filters

[PATCH v2 1/2] can: fix multiple delivery of a single CAN frame for overlapping CAN filters

[Oliver Hartkopp]

The CAN_RAW socket can set multiple CAN identifier specific filters that lead
to multiple filters in the af_can.c filter processing. These filters are
indenpendent from each other which leads to logical OR'ed filters when applied.

This patch makes sure that every CAN frame which is filtered for a specific
socket is only delivered once to the user space. This is independent from the
number of matching CAN filters of this socket.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---

v2: move simple check to be processed first

 net/can/raw.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/net/can/raw.c b/net/can/raw.c
index 00c13ef..0a6abf1 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -86,6 +86,8 @@ struct raw_sock {
 	struct can_filter dfilter; /* default/single filter */
 	struct can_filter *filter; /* pointer to filter(s) */
 	can_err_mask_t err_mask;
+	struct sk_buff *uniq_skb;
+	ktime_t uniq_tstamp;
 };
 
 /*
@@ -123,6 +125,15 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
 	if (!ro->fd_frames && oskb->len != CAN_MTU)
 		return;
 
+	/* eliminate multiple filter matches for the same skb */
+	if (ro->uniq_skb == oskb &&
+	    ktime_equal(ro->uniq_tstamp, oskb->tstamp)){
+		return;
+	} else {
+		ro->uniq_skb = oskb;
+		ro->uniq_tstamp = oskb->tstamp;
+	}
+
 	/* clone the given skb to be able to enqueue it into the rcv queue */
 	skb = skb_clone(oskb, GFP_ATOMIC);
 	if (!skb)
@@ -296,6 +307,8 @@ static int raw_init(struct sock *sk)
 	ro->loopback         = 1;
 	ro->recv_own_msgs    = 0;
 	ro->fd_frames        = 0;
+	ro->uniq_skb         = NULL;
+	ro->uniq_tstamp      = ktime_set(0, 0);
 
 	/* set notifier */
 	ro->notifier.notifier_call = raw_notifier;
-- 
2.1.4

[PATCH v2 2/2] can: introduce new raw socket option to join the given CAN filters

[Oliver Hartkopp]

The CAN_RAW socket can set multiple CAN identifier specific filters that lead
to multiple filters in the af_can.c filter processing. These filters are
indenpendent from each other which leads to logical OR'ed filters when applied.

This socket option joines the given CAN filters in the way that only CAN frames
are passed to user space that matched *all* given CAN filters. The semantic for
the applied filters is therefore changed to a logical AND.

This is useful especially when the filterset is a combination of filters where
the CAN_INV_FILTER flag is set in order to notch single CAN IDs or CAN ID
ranges from the incoming traffic.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---

v2: fix first reception of joint filter CAN frame

 Documentation/networking/can.txt | 20 ++++++++++++++++++--
 include/uapi/linux/can/raw.h     |  1 +
 net/can/raw.c                    | 31 ++++++++++++++++++++++++++++++-
 3 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/Documentation/networking/can.txt b/Documentation/networking/can.txt
index 0a2859a..5abad1e 100644
--- a/Documentation/networking/can.txt
+++ b/Documentation/networking/can.txt
@@ -22,7 +22,8 @@ This file contains
       4.1.3 RAW socket option CAN_RAW_LOOPBACK
       4.1.4 RAW socket option CAN_RAW_RECV_OWN_MSGS
       4.1.5 RAW socket option CAN_RAW_FD_FRAMES
-      4.1.6 RAW socket returned message flags
+      4.1.6 RAW socket option CAN_RAW_JOIN_FILTERS
+      4.1.7 RAW socket returned message flags
     4.2 Broadcast Manager protocol sockets (SOCK_DGRAM)
       4.2.1 Broadcast Manager operations
       4.2.2 Broadcast Manager message flags
@@ -601,7 +602,22 @@ solution for a couple of reasons:
   CAN FD frames by checking if the device maximum transfer unit is CANFD_MTU.
   The CAN device MTU can be retrieved e.g. with a SIOCGIFMTU ioctl() syscall.
 
-  4.1.6 RAW socket returned message flags
+  4.1.6 RAW socket option CAN_RAW_JOIN_FILTERS
+
+  The CAN_RAW socket can set multiple CAN identifier specific filters that
+  lead to multiple filters in the af_can.c filter processing. These filters
+  are indenpendent from each other which leads to logical OR'ed filters when
+  applied (see 4.1.1).
+
+  This socket option joines the given CAN filters in the way that only CAN
+  frames are passed to user space that matched *all* given CAN filters. The
+  semantic for the applied filters is therefore changed to a logical AND.
+
+  This is useful especially when the filterset is a combination of filters
+  where the CAN_INV_FILTER flag is set in order to notch single CAN IDs or
+  CAN ID ranges from the incoming traffic.
+
+  4.1.7 RAW socket returned message flags
 
   When using recvmsg() call, the msg->msg_flags may contain following flags:
 
diff --git a/include/uapi/linux/can/raw.h b/include/uapi/linux/can/raw.h
index 78ec76f..8735f108 100644
--- a/include/uapi/linux/can/raw.h
+++ b/include/uapi/linux/can/raw.h
@@ -57,6 +57,7 @@ enum {
 	CAN_RAW_LOOPBACK,	/* local loopback (default:on)       */
 	CAN_RAW_RECV_OWN_MSGS,	/* receive my own msgs (default:off) */
 	CAN_RAW_FD_FRAMES,	/* allow CAN FD frames (default:off) */
+	CAN_RAW_JOIN_FILTERS,	/* all filters must match to trigger */
 };
 
 #endif /* !_UAPI_CAN_RAW_H */
diff --git a/net/can/raw.c b/net/can/raw.c
index 0a6abf1..0f8908e 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -82,6 +82,8 @@ struct raw_sock {
 	int loopback;
 	int recv_own_msgs;
 	int fd_frames;
+	int join_filters;
+	int join_rx_count;
 	int count;                 /* number of active filters */
 	struct can_filter dfilter; /* default/single filter */
 	struct can_filter *filter; /* pointer to filter(s) */
@@ -128,10 +130,20 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
 	/* eliminate multiple filter matches for the same skb */
 	if (ro->uniq_skb == oskb &&
 	    ktime_equal(ro->uniq_tstamp, oskb->tstamp)){
-		return;
+		if (ro->join_filters) {
+			ro->join_rx_count++;
+			/* drop frame until all enabled filters matched */
+			if (ro->join_rx_count < ro->count)
+				return;
+		} else
+			return;
 	} else {
 		ro->uniq_skb = oskb;
 		ro->uniq_tstamp = oskb->tstamp;
+		ro->join_rx_count = 1;
+		/* drop first frame to check all enabled filters? */
+		if (ro->join_filters && ro->count > 1)
+			return;
 	}
 
 	/* clone the given skb to be able to enqueue it into the rcv queue */
@@ -307,6 +319,8 @@ static int raw_init(struct sock *sk)
 	ro->loopback         = 1;
 	ro->recv_own_msgs    = 0;
 	ro->fd_frames        = 0;
+	ro->join_filters     = 0;
+	ro->join_rx_count    = 0;
 	ro->uniq_skb         = NULL;
 	ro->uniq_tstamp      = ktime_set(0, 0);
 
@@ -596,6 +610,15 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 
 		break;
 
+	case CAN_RAW_JOIN_FILTERS:
+		if (optlen != sizeof(ro->join_filters))
+			return -EINVAL;
+
+		if (copy_from_user(&ro->join_filters, optval, optlen))
+			return -EFAULT;
+
+		break;
+
 	default:
 		return -ENOPROTOOPT;
 	}
@@ -660,6 +683,12 @@ static int raw_getsockopt(struct socket *sock, int level, int optname,
 		val = &ro->fd_frames;
 		break;
 
+	case CAN_RAW_JOIN_FILTERS:
+		if (len > sizeof(int))
+			len = sizeof(int);
+		val = &ro->join_filters;
+		break;
+
 	default:
 		return -ENOPROTOOPT;
 	}
-- 
2.1.4

Re: [PATCH v2 2/2] can: introduce new raw socket option to join the given CAN filters

[Ahmed S. Darwish]

Hi Oliver,

On Wed, Mar 18, 2015 at 07:10:35AM +0100, Oliver Hartkopp wrote:
> The CAN_RAW socket can set multiple CAN identifier specific filters that lead
> to multiple filters in the af_can.c filter processing. These filters are
> indenpendent from each other which leads to logical OR'ed filters when applied.
> 
> This socket option joines the given CAN filters in the way that only CAN frames
> are passed to user space that matched *all* given CAN filters. The semantic for
> the applied filters is therefore changed to a logical AND.
> 
> This is useful especially when the filterset is a combination of filters where
> the CAN_INV_FILTER flag is set in order to notch single CAN IDs or CAN ID
> ranges from the incoming traffic.
> 
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
> 
> v2: fix first reception of joint filter CAN frame
> 

I've done some very basic smoke tests on 4.0.0-rc4+, and
everything seems to work just as it says on the tin :-)

Before applying the two kernel patches:

$ cangen vcan0 -Di -Ii -L4 &

$ candump vcan0,0:0,0:0,0:0

  /* 3 filters, 3 times repeat for each frame */
  vcan0  000   [4]  00 00 00 00
  vcan0  000   [4]  00 00 00 00
  vcan0  000   [4]  00 00 00 00
  vcan0  001   [4]  01 00 00 00
  vcan0  001   [4]  01 00 00 00
  vcan0  001   [4]  01 00 00 00
  vcan0  002   [4]  02 00 00 00
  vcan0  002   [4]  02 00 00 00
  vcan0  002   [4]  02 00 00 00
  vcan0  003   [4]  03 00 00 00
  vcan0  003   [4]  03 00 00 00
  vcan0  003   [4]  03 00 00 00

$ candump vcan0,0:0,0:0,0:0,J

  setsockopt CAN_RAW_JOIN_FILTERS not supported by your
  Linux Kernel: Protocol not available

##########################################

After applying the two kernel patches:

$ cangen vcan0 -Di -Ii -L4 &

$ candump vcan0,0:0,0:0,0:0

  /* 3 filters, no repeat for each frame (great!) */
  vcan0  000   [4]  00 00 00 00
  vcan0  001   [4]  01 00 00 00
  vcan0  002   [4]  02 00 00 00
  vcan0  003   [4]  03 00 00 00
  vcan0  004   [4]  04 00 00 00
  vcan0  005   [4]  05 00 00 00
  vcan0  006   [4]  06 00 00 00

$ cangen vcan0 -Di -I 123 -L 4

$ candump vcan0,123~7FF

  /* Basic case: no output as all CAN IDs matches 123 */

$ candump vcan0,123~7FF,111~7FF

  /* OR operator working as expected */
  vcan0  123   [4]  00 00 00 00
  vcan0  123   [4]  01 00 00 00
  vcan0  123   [4]  02 00 00 00
  vcan0  123   [4]  03 00 00 00
  vcan0  123   [4]  04 00 00 00
  vcan0  123   [4]  05 00 00 00
  vcan0  123   [4]  06 00 00 00

$ candump vcan0,123~7FF,111~7FF,J

  /* No output; AND operator working as expected */


Regards,
Darwish

Re: [PATCH v2 1/2] can: fix multiple delivery of a single CAN frame for overlapping CAN filters

[Ahmed S. Darwish]

Hi!

On Wed, Mar 18, 2015 at 07:10:34AM +0100, Oliver Hartkopp wrote:
> The CAN_RAW socket can set multiple CAN identifier specific filters that lead
> to multiple filters in the af_can.c filter processing. These filters are
> indenpendent from each other which leads to logical OR'ed filters when applied.
> 
> This patch makes sure that every CAN frame which is filtered for a specific
> socket is only delivered once to the user space. This is independent from the
> number of matching CAN filters of this socket.
> 
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
> 
> v2: move simple check to be processed first
> 
>  net/can/raw.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/net/can/raw.c b/net/can/raw.c
> index 00c13ef..0a6abf1 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -86,6 +86,8 @@ struct raw_sock {
>  	struct can_filter dfilter; /* default/single filter */
>  	struct can_filter *filter; /* pointer to filter(s) */
>  	can_err_mask_t err_mask;
> +	struct sk_buff *uniq_skb;
> +	ktime_t uniq_tstamp;
>  };
>  

My knowledge of the networking internals are really shallow,
but the `uniq_tstamp' field above feels a little bit redundant?

As seen below:

>  /*
> @@ -123,6 +125,15 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
>  	if (!ro->fd_frames && oskb->len != CAN_MTU)
>  		return;
>  
> +	/* eliminate multiple filter matches for the same skb */
> +	if (ro->uniq_skb == oskb &&
> +	    ktime_equal(ro->uniq_tstamp, oskb->tstamp)){
> +		return;
> +	} else {
> +		ro->uniq_skb = oskb;
> +		ro->uniq_tstamp = oskb->tstamp;
> +	}
> +

The two newly-introduced fields are always set together, and they
are not read anywhere beyond the if check above.

If so, isn't the `uniq_skb' field, on its own, quite sufficient?

>  	/* clone the given skb to be able to enqueue it into the rcv queue */
>  	skb = skb_clone(oskb, GFP_ATOMIC);
>  	if (!skb)
> @@ -296,6 +307,8 @@ static int raw_init(struct sock *sk)
>  	ro->loopback         = 1;
>  	ro->recv_own_msgs    = 0;
>  	ro->fd_frames        = 0;
> +	ro->uniq_skb         = NULL;
> +	ro->uniq_tstamp      = ktime_set(0, 0);
>  
>  	/* set notifier */
>  	ro->notifier.notifier_call = raw_notifier;
>

Thanks,
Darwish

Re: [PATCH v2 1/2] can: fix multiple delivery of a single CAN frame for overlapping CAN filters

[Oliver Hartkopp]

On 18.03.2015 23:17, Ahmed S. Darwish wrote:

>> diff --git a/net/can/raw.c b/net/can/raw.c
>> index 00c13ef..0a6abf1 100644
>> --- a/net/can/raw.c
>> +++ b/net/can/raw.c
>> @@ -86,6 +86,8 @@ struct raw_sock {
>>   	struct can_filter dfilter; /* default/single filter */
>>   	struct can_filter *filter; /* pointer to filter(s) */
>>   	can_err_mask_t err_mask;
>> +	struct sk_buff *uniq_skb;
>> +	ktime_t uniq_tstamp;
>>   };
>>
>
> My knowledge of the networking internals are really shallow,
> but the `uniq_tstamp' field above feels a little bit redundant?

(..)

> If so, isn't the `uniq_skb' field, on its own, quite sufficient?

AFAIK the skbs are retrieved from a memory pool, so it can be that the skb 
pointer values can be the same by the time.

E.g. if you receive the CAN frames in a timely distance that the skb pointer 
accidentally is the same than the last time, you might unintentionally drop a 
new (changed) content.

With checking the timestamp this possibility of potentially identical skb 
pointers is removed.

Regards,
Oliver