[PATCH] can: add new socket option CAN_RAW_FILTER_UNIQUE to eliminate duplicate matches

[PATCH] can: add new socket option CAN_RAW_FILTER_UNIQUE to eliminate duplicate matches

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

Especially when the filter is intended to notch single CAN IDs or CAN ID ranges
a problem emerges when using more than just one filter: The notched CAN IDs
will be delivered once while the other CAN IDs will be delivered twice.

This patch implements a new option to make 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>
---
 include/uapi/linux/can/raw.h |  1 +
 net/can/raw.c                | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/include/uapi/linux/can/raw.h b/include/uapi/linux/can/raw.h
index 78ec76f..723247a 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_FILTER_UNIQUE,	/* eliminate multiple filter matches */
 };
 
 #endif /* !_UAPI_CAN_RAW_H */
diff --git a/net/can/raw.c b/net/can/raw.c
index 00c13ef..e593dc7 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -74,6 +74,12 @@ MODULE_ALIAS("can-proto-1");
  * storing the single filter in dfilter, to avoid using dynamic memory.
  */
 
+/* structure to detect multiple filter matches for the same skb */
+struct unique_can_frame {
+	struct sk_buff *skb;
+	ktime_t tstamp;
+};
+
 struct raw_sock {
 	struct sock sk;
 	int bound;
@@ -82,10 +88,12 @@ struct raw_sock {
 	int loopback;
 	int recv_own_msgs;
 	int fd_frames;
+	int filter_unique;
 	int count;                 /* number of active filters */
 	struct can_filter dfilter; /* default/single filter */
 	struct can_filter *filter; /* pointer to filter(s) */
 	can_err_mask_t err_mask;
+	struct unique_can_frame uniq_cf;
 };
 
 /*
@@ -119,6 +127,16 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
 	if (!ro->recv_own_msgs && oskb->sk == sk)
 		return;
 
+	/* eliminate multiple filter matches for the same skb */
+	if (ro->filter_unique) {
+		if (ro->uniq_cf.skb == oskb &&
+		    ktime_equal(ro->uniq_cf.tstamp, oskb->tstamp))
+			return;
+
+		ro->uniq_cf.skb = oskb;
+		ro->uniq_cf.tstamp = oskb->tstamp;
+	}
+
 	/* do not pass non-CAN2.0 frames to a legacy socket */
 	if (!ro->fd_frames && oskb->len != CAN_MTU)
 		return;
@@ -296,6 +314,9 @@ static int raw_init(struct sock *sk)
 	ro->loopback         = 1;
 	ro->recv_own_msgs    = 0;
 	ro->fd_frames        = 0;
+	ro->filter_unique    = 0;
+	ro->uniq_cf.skb      = NULL;
+	ro->uniq_cf.tstamp   = ktime_set(0, 0);
 
 	/* set notifier */
 	ro->notifier.notifier_call = raw_notifier;
@@ -583,6 +604,15 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 
 		break;
 
+	case CAN_RAW_FILTER_UNIQUE:
+		if (optlen != sizeof(ro->filter_unique))
+			return -EINVAL;
+
+		if (copy_from_user(&ro->filter_unique, optval, optlen))
+			return -EFAULT;
+
+		break;
+
 	default:
 		return -ENOPROTOOPT;
 	}
@@ -647,6 +677,12 @@ static int raw_getsockopt(struct socket *sock, int level, int optname,
 		val = &ro->fd_frames;
 		break;
 
+	case CAN_RAW_FILTER_UNIQUE:
+		if (len > sizeof(int))
+			len = sizeof(int);
+		val = &ro->filter_unique;
+		break;
+
 	default:
 		return -ENOPROTOOPT;
 	}
-- 
2.1.4

Re: [PATCH] can: add new socket option CAN_RAW_FILTER_UNIQUE to eliminate duplicate matches

[Oliver Hartkopp]

Hi Marc,

On 17.03.2015 13:58, 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.
>
> Especially when the filter is intended to notch single CAN IDs or CAN ID ranges
> a problem emerges when using more than just one filter: The notched CAN IDs
> will be delivered once while the other CAN IDs will be delivered twice.
>
> This patch implements a new option to make 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.

Just to make sure, this patch really helps:

With the example above all CAN frames are delivered just once. The OR'ed 
filters still match. So you won't have the solution that you can have 
different notched areas of CAN IDs ...

I hope I expressed myself clearly.

Regards,
Oliver

Re: [PATCH] can: add new socket option CAN_RAW_FILTER_UNIQUE to eliminate duplicate matches

[Oliver Hartkopp]

Hi Marc,

I know why I thought it will get messy ...

E.g. when you notch three CAN IDs like this:

candump can0,100~7FF,200~7FF,400~7FF

You will get *without* the unify option:

cansend can0 100#	-> two times CAN frame with 0x100
cansend can0 200#	-> two times CAN frame with 0x200
cansend can0 400#	-> two times CAN frame with 0x400
cansend can0 333#	-> three times CAN frame with 0x333

You will get *with* the unify option from the posted patch:

cansend can0 100#	-> one time CAN frame with 0x100
cansend can0 200#	-> one time CAN frame with 0x200
cansend can0 400#	-> one time CAN frame with 0x400
cansend can0 333#	-> one time CAN frame with 0x333

... the same as 'candump can0' %-(

So what you probably *want* to have is:

cansend can0 100#	-> no CAN frame
cansend can0 200#	-> no CAN frame
cansend can0 400#	-> no CAN frame
cansend can0 333#	-> one CAN frame with 0x333

Right?

An approach to solve exactly this use case could be to check whether *all* 
configured filters have passed the CAN frame.

This could be done by counting the receptions of the identical frames:

diff --git a/net/can/raw.c b/net/can/raw.c
index e593dc7..3022c25 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -78,6 +78,7 @@ MODULE_ALIAS("can-proto-1");
  struct unique_can_frame {
  	struct sk_buff *skb;
  	ktime_t tstamp;
+	int rx_count;
  };

  struct raw_sock {
@@ -130,11 +131,15 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
  	/* eliminate multiple filter matches for the same skb */
  	if (ro->filter_unique) {
  		if (ro->uniq_cf.skb == oskb &&
-		    ktime_equal(ro->uniq_cf.tstamp, oskb->tstamp))
-			return;
-
-		ro->uniq_cf.skb = oskb;
-		ro->uniq_cf.tstamp = oskb->tstamp;
+		    ktime_equal(ro->uniq_cf.tstamp, oskb->tstamp)){
+			ro->uniq_cf.rx_count++;
+			if (ro->uniq_cf.rx_count < ro->count)
+				return;
+		} else {
+			ro->uniq_cf.skb = oskb;
+			ro->uniq_cf.tstamp = oskb->tstamp;
+			ro->uniq_cf.rx_count = 1;
+		}
  	}

  	/* do not pass non-CAN2.0 frames to a legacy socket */

(based on the patch before)

But does it really make sense to put such a tricky thing into the raw socket?

It flips the OR sematic to an AND semantic over the provided filters.
And then the name of the socket option should be adapted too.
Something like "all filters must match"

Regards,
Oliver



On 17.03.2015 14:23, Oliver Hartkopp wrote:
> Hi Marc,
>
> On 17.03.2015 13:58, 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.
>>
>> Especially when the filter is intended to notch single CAN IDs or CAN ID ranges
>> a problem emerges when using more than just one filter: The notched CAN IDs
>> will be delivered once while the other CAN IDs will be delivered twice.
>>
>> This patch implements a new option to make 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.
>
> Just to make sure, this patch really helps:
>
> With the example above all CAN frames are delivered just once. The OR'ed
> filters still match. So you won't have the solution that you can have
> different notched areas of CAN IDs ...
>
> I hope I expressed myself clearly.
>
> 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

Re: [PATCH] can: add new socket option CAN_RAW_FILTER_UNIQUE to eliminate duplicate matches

[Andre Naujoks]

On 17.03.2015 13:58, 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.
> 
> Especially when the filter is intended to notch single CAN IDs or CAN ID ranges
> a problem emerges when using more than just one filter: The notched CAN IDs
> will be delivered once while the other CAN IDs will be delivered twice.
> 
> This patch implements a new option to make 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.

Hi Oliver.

I don't think this should be an option. A CAN-frame should never be
delivered more than once even if multiple filters match.

In my eyes this is a long standing bug ;-), which would get an
off-switch this way.

Regards
  Andre

Re: [PATCH] can: add new socket option CAN_RAW_FILTER_UNIQUE to eliminate duplicate matches

[Andre Naujoks]

On 17.03.2015 16:25, Oliver Hartkopp wrote:
> Hi Marc,
> 
> I know why I thought it will get messy ...
> 
> E.g. when you notch three CAN IDs like this:
> 
> candump can0,100~7FF,200~7FF,400~7FF
> 
> You will get *without* the unify option:
> 
> cansend can0 100#    -> two times CAN frame with 0x100
> cansend can0 200#    -> two times CAN frame with 0x200
> cansend can0 400#    -> two times CAN frame with 0x400
> cansend can0 333#    -> three times CAN frame with 0x333
> 
> You will get *with* the unify option from the posted patch:
> 
> cansend can0 100#    -> one time CAN frame with 0x100
> cansend can0 200#    -> one time CAN frame with 0x200
> cansend can0 400#    -> one time CAN frame with 0x400
> cansend can0 333#    -> one time CAN frame with 0x333
> 
> ... the same as 'candump can0' %-(
> 
> So what you probably *want* to have is:
> 
> cansend can0 100#    -> no CAN frame
> cansend can0 200#    -> no CAN frame
> cansend can0 400#    -> no CAN frame
> cansend can0 333#    -> one CAN frame with 0x333
> 
> Right?
> 
> An approach to solve exactly this use case could be to check whether
> *all* configured filters have passed the CAN frame.


I would like to see this as a socket option, too. That way you can
choose which way you want it. Either one filter match is required or all
filters have to match in order for a CAN-frame to pass.

Regards
  Andre


> 
> This could be done by counting the receptions of the identical frames:
> 
> diff --git a/net/can/raw.c b/net/can/raw.c
> index e593dc7..3022c25 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -78,6 +78,7 @@ MODULE_ALIAS("can-proto-1");
>  struct unique_can_frame {
>      struct sk_buff *skb;
>      ktime_t tstamp;
> +    int rx_count;
>  };
> 
>  struct raw_sock {
> @@ -130,11 +131,15 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
>      /* eliminate multiple filter matches for the same skb */
>      if (ro->filter_unique) {
>          if (ro->uniq_cf.skb == oskb &&
> -            ktime_equal(ro->uniq_cf.tstamp, oskb->tstamp))
> -            return;
> -
> -        ro->uniq_cf.skb = oskb;
> -        ro->uniq_cf.tstamp = oskb->tstamp;
> +            ktime_equal(ro->uniq_cf.tstamp, oskb->tstamp)){
> +            ro->uniq_cf.rx_count++;
> +            if (ro->uniq_cf.rx_count < ro->count)
> +                return;
> +        } else {
> +            ro->uniq_cf.skb = oskb;
> +            ro->uniq_cf.tstamp = oskb->tstamp;
> +            ro->uniq_cf.rx_count = 1;
> +        }
>      }
> 
>      /* do not pass non-CAN2.0 frames to a legacy socket */
> 
> (based on the patch before)
> 
> But does it really make sense to put such a tricky thing into the raw
> socket?
> 
> It flips the OR sematic to an AND semantic over the provided filters.
> And then the name of the socket option should be adapted too.
> Something like "all filters must match"
> 
> Regards,
> Oliver
> 
> 
> 
> On 17.03.2015 14:23, Oliver Hartkopp wrote:
>> Hi Marc,
>>
>> On 17.03.2015 13:58, 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.
>>>
>>> Especially when the filter is intended to notch single CAN IDs or CAN
>>> ID ranges
>>> a problem emerges when using more than just one filter: The notched
>>> CAN IDs
>>> will be delivered once while the other CAN IDs will be delivered twice.
>>>
>>> This patch implements a new option to make 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.
>>
>> Just to make sure, this patch really helps:
>>
>> With the example above all CAN frames are delivered just once. The OR'ed
>> filters still match. So you won't have the solution that you can have
>> different notched areas of CAN IDs ...
>>
>> I hope I expressed myself clearly.
>>
>> 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
> -- 
> 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

Re: [PATCH] can: add new socket option CAN_RAW_FILTER_UNIQUE to eliminate duplicate matches

[Oliver Hartkopp]

On 17.03.2015 16:36, Andre Naujoks wrote:
> On 17.03.2015 13:58, 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.
>>
>> Especially when the filter is intended to notch single CAN IDs or CAN ID ranges
>> a problem emerges when using more than just one filter: The notched CAN IDs
>> will be delivered once while the other CAN IDs will be delivered twice.
>>
>> This patch implements a new option to make 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.
>
> Hi Oliver.
>
> I don't think this should be an option. A CAN-frame should never be
> delivered more than once even if multiple filters match.
>
> In my eyes this is a long standing bug ;-), which would get an
> off-switch this way.

https://www.youtube.com/watch?v=4JgxDxwf7RU

:-))

So you think the 'unifying option' should be enabled by default!?!
Would you assume this to break any user space apps?

Btw. what's your opinion about

http://marc.info/?l=linux-can&m=142660595731528&w=2

??

Regards,
Oliver

Re: [PATCH] can: add new socket option CAN_RAW_FILTER_UNIQUE to eliminate duplicate matches

[Andre Naujoks]

On 17.03.2015 16:49, Oliver Hartkopp wrote:
> On 17.03.2015 16:36, Andre Naujoks wrote:
>> On 17.03.2015 13:58, 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.
>>>
>>> Especially when the filter is intended to notch single CAN IDs or CAN
>>> ID ranges
>>> a problem emerges when using more than just one filter: The notched
>>> CAN IDs
>>> will be delivered once while the other CAN IDs will be delivered twice.
>>>
>>> This patch implements a new option to make 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.
>>
>> Hi Oliver.
>>
>> I don't think this should be an option. A CAN-frame should never be
>> delivered more than once even if multiple filters match.
>>
>> In my eyes this is a long standing bug ;-), which would get an
>> off-switch this way.
> 
> https://www.youtube.com/watch?v=4JgxDxwf7RU
> 
> :-))
> 
> So you think the 'unifying option' should be enabled by default!?!

Yes!

> Would you assume this to break any user space apps?

I don't see how. In fact I had to work around the "duplpicate frame
issue" on occasion. If nobody sees a reason against this, I am strongly
in favour of making this the default behaviour.

> 
> Btw. what's your opinion about
> 
> http://marc.info/?l=linux-can&m=142660595731528&w=2

http://marc.info/?l=linux-can&m=142660682531909&w=2 ;-)

I'd like that as a socket option. Effectively choosing if the set
filters should be OR'd or AND'd.

Regards
  Andre

> 
> ??
> 
> Regards,
> Oliver