[PATCH] can: Fix CAN_(EFF|RTR)_FLAG handling in can_filter

[PATCH] can: Fix CAN_(EFF|RTR)_FLAG handling in can_filter

[Oliver Hartkopp]

[-- Attachment #1: Type: text/plain, Size: 1077 bytes --]

can: Fix CAN_(EFF|RTR)_FLAG handling in can_filter

Due to a wrong safety check in af_can.c it was not possible to filter
for SFF frames with a specific CAN identifier without getting the
same selected CAN identifier from a received EFF frame also.

This fix has a minimum (but user visible) impact on the CAN filter
API and therefore the CAN version is set to a new date.

Indeed the 'old' API is still working as-is. But when now setting
CAN_(EFF|RTR)_FLAG in can_filter.can_mask you might get less traffic
than before - but still the stuff that you expected to get for your
defined filter ...

Thanks to Kurt Van Dijck for pointing at this issue and for the review.

Signed-Off-by: Oliver Hartkopp <oliver@hartkopp.net>
Acked-by: Kurt Van Dijck <kurt.van.dijck@eia.be>
---

Hello Dave & Greg,

this patch fixes an issue when running with mixed CAN frame formats.

It should go into 2.6.28 before the bug fix window closes.

Additionally the patch applies excellent down to 2.6.26. Greg please
consider this patch for stable .26 and .27 also.

Thanks & best regards,
Oliver


[-- Attachment #2: can_filter.patch --]
[-- Type: text/x-patch, Size: 4464 bytes --]

diff --git a/include/linux/can/core.h b/include/linux/can/core.h
index e9ca210..f50785a 100644
--- a/include/linux/can/core.h
+++ b/include/linux/can/core.h
@@ -19,7 +19,7 @@
 #include <linux/skbuff.h>
 #include <linux/netdevice.h>
 
-#define CAN_VERSION "20071116"
+#define CAN_VERSION "20081130"
 
 /* increment this number each time you change some user-space interface */
 #define CAN_ABI_VERSION "8"
diff --git a/net/can/af_can.c b/net/can/af_can.c
index 7d4d2b3..d8173e5 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -319,23 +319,52 @@ static struct dev_rcv_lists *find_dev_rcv_lists(struct net_device *dev)
 	return n ? d : NULL;
 }
 
+/**
+ * find_rcv_list - determine optimal filterlist inside device filter struct
+ * @can_id: pointer to CAN identifier of a given can_filter
+ * @mask: pointer to CAN mask of a given can_filter
+ * @d: pointer to the device filter struct
+ *
+ * Description:
+ *  Returns the optimal filterlist to reduce the filter handling in the
+ *  receive path. This function is called by service functions that need
+ *  to register or unregister a can_filter in the filter lists.
+ *
+ *  A filter matches in general, when
+ *
+ *          <received_can_id> & mask == can_id & mask
+ *
+ *  so every bit set in the mask (even CAN_EFF_FLAG, CAN_RTR_FLAG) describe
+ *  relevant bits for the filter.
+ *
+ *  The filter can be inverted (CAN_INV_FILTER bit set in can_id) or it can
+ *  filter for error frames (CAN_ERR_FLAG bit set in mask). For error frames
+ *  there is a special filterlist and a special rx path filter handling.
+ *
+ * Return:
+ *  Pointer to optimal filterlist for the given can_id/mask pair.
+ *  Constistency checked mask.
+ *  Reduced can_id to have a preprocessed filter compare value.
+ */
 static struct hlist_head *find_rcv_list(canid_t *can_id, canid_t *mask,
 					struct dev_rcv_lists *d)
 {
 	canid_t inv = *can_id & CAN_INV_FILTER; /* save flag before masking */
 
-	/* filter error frames */
+	/* filter for error frames in extra filterlist */
 	if (*mask & CAN_ERR_FLAG) {
-		/* clear CAN_ERR_FLAG in list entry */
+		/* clear CAN_ERR_FLAG in filter entry */
 		*mask &= CAN_ERR_MASK;
 		return &d->rx[RX_ERR];
 	}
 
-	/* ensure valid values in can_mask */
-	if (*mask & CAN_EFF_FLAG)
-		*mask &= (CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG);
-	else
-		*mask &= (CAN_SFF_MASK | CAN_RTR_FLAG);
+	/* with cleared CAN_ERR_FLAG we have a simple mask/value filterpair */
+
+#define CAN_EFF_RTR_FLAGS (CAN_EFF_FLAG | CAN_RTR_FLAG)
+
+	/* ensure valid values in can_mask for 'SFF only' frame filtering */
+	if ((*mask & CAN_EFF_FLAG) && !(*can_id & CAN_EFF_FLAG))
+		*mask &= (CAN_SFF_MASK | CAN_EFF_RTR_FLAGS);
 
 	/* reduce condition testing at receive time */
 	*can_id &= *mask;
@@ -348,15 +377,19 @@ static struct hlist_head *find_rcv_list(canid_t *can_id, canid_t *mask,
 	if (!(*mask))
 		return &d->rx[RX_ALL];
 
-	/* use extra filterset for the subscription of exactly *ONE* can_id */
-	if (*can_id & CAN_EFF_FLAG) {
-		if (*mask == (CAN_EFF_MASK | CAN_EFF_FLAG)) {
-			/* RFC: a use-case for hash-tables in the future? */
-			return &d->rx[RX_EFF];
+	/* extra filterlists for the subscription of a single non-RTR can_id */
+	if (((*mask & CAN_EFF_RTR_FLAGS) == CAN_EFF_RTR_FLAGS)
+	    && !(*can_id & CAN_RTR_FLAG)) {
+
+		if (*can_id & CAN_EFF_FLAG) {
+			if (*mask == (CAN_EFF_MASK | CAN_EFF_RTR_FLAGS)) {
+				/* RFC: a future use-case for hash-tables? */
+				return &d->rx[RX_EFF];
+			}
+		} else {
+			if (*mask == (CAN_SFF_MASK | CAN_EFF_RTR_FLAGS))
+				return &d->rx_sff[*can_id];
 		}
-	} else {
-		if (*mask == CAN_SFF_MASK)
-			return &d->rx_sff[*can_id];
 	}
 
 	/* default: filter via can_id/can_mask */
diff --git a/net/can/bcm.c b/net/can/bcm.c
index d0dd382..da0d426 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -64,10 +64,11 @@
 #define BCM_CAN_DLC_MASK 0x0F /* clean private flags in can_dlc by masking */
 
 /* get best masking value for can_rx_register() for a given single can_id */
-#define REGMASK(id) ((id & CAN_RTR_FLAG) | ((id & CAN_EFF_FLAG) ? \
-			(CAN_EFF_MASK | CAN_EFF_FLAG) : CAN_SFF_MASK))
+#define REGMASK(id) ((id & CAN_EFF_FLAG) ? \
+		     (CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG) : \
+		     (CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG))
 
-#define CAN_BCM_VERSION "20080415"
+#define CAN_BCM_VERSION CAN_VERSION
 static __initdata const char banner[] = KERN_INFO
 	"can: broadcast manager protocol (rev " CAN_BCM_VERSION ")\n";
 

Re: [PATCH] can: Fix CAN_(EFF|RTR)_FLAG handling in can_filter

[Sam Ravnborg]

On Mon, Dec 01, 2008 at 05:15:42PM +0100, Oliver Hartkopp wrote:
> can: Fix CAN_(EFF|RTR)_FLAG handling in can_filter
> 
> Due to a wrong safety check in af_can.c it was not possible to filter
> for SFF frames with a specific CAN identifier without getting the
> same selected CAN identifier from a received EFF frame also.
> 
> This fix has a minimum (but user visible) impact on the CAN filter
> API and therefore the CAN version is set to a new date.
> 
> Indeed the 'old' API is still working as-is. But when now setting
> CAN_(EFF|RTR)_FLAG in can_filter.can_mask you might get less traffic
> than before - but still the stuff that you expected to get for your
> defined filter ...
> 
> Thanks to Kurt Van Dijck for pointing at this issue and for the review.
> 
> Signed-Off-by: Oliver Hartkopp <oliver@hartkopp.net>
> Acked-by: Kurt Van Dijck <kurt.van.dijck@eia.be>

If Kurt tested this patch then he should
be credited with a "Tested-by:" tag.

I recall he did but I deleted the thread from my can mailbox.

	Sam

Re: [PATCH] can: Fix CAN_(EFF|RTR)_FLAG handling in can_filter

[Oliver Hartkopp]

Sam Ravnborg wrote:
> On Mon, Dec 01, 2008 at 05:15:42PM +0100, Oliver Hartkopp wrote:
>   
>>
>> Thanks to Kurt Van Dijck for pointing at this issue and for the review.
>>
>> Signed-Off-by: Oliver Hartkopp <oliver@hartkopp.net>
>> Acked-by: Kurt Van Dijck <kurt.van.dijck@eia.be>
>>     
>
> If Kurt tested this patch then he should
> be credited with a "Tested-by:" tag.
>
> I recall he did but I deleted the thread from my can mailbox.
>
> 	Sam
>   
Hi Sam,

i did some testing and posted the results of a first patch that went 
into the final direction.

In

https://lists.berlios.de/pipermail/socketcan-core/2008-December/002117.html

Kurt gave a "Reviewed-by:" and in a later mail in

https://lists.berlios.de/pipermail/socketcan-core/2008-December/002119.html

he wrote "my Acked-by: still stands" for the final patch.

So i choosed "Acked-by:".

As Kurt went that deep into the code that he pointed me to the right 
line and presented a first idea for a patch, an 'Acked-by:' for 
confirming the final patch looked like an appropriate credit to me.

Regards,
Oliver

Re: [PATCH] can: Fix CAN_(EFF|RTR)_FLAG handling in can_filter

[Sam Ravnborg]

On Mon, Dec 01, 2008 at 07:42:31PM +0100, Oliver Hartkopp wrote:
> Sam Ravnborg wrote:
> >On Mon, Dec 01, 2008 at 05:15:42PM +0100, Oliver Hartkopp wrote:
> >  
> >>
> >>Thanks to Kurt Van Dijck for pointing at this issue and for the review.
> >>
> >>Signed-Off-by: Oliver Hartkopp <oliver@hartkopp.net>
> >>Acked-by: Kurt Van Dijck <kurt.van.dijck@eia.be>
> >>    
> >
> >If Kurt tested this patch then he should
> >be credited with a "Tested-by:" tag.
> >
> >I recall he did but I deleted the thread from my can mailbox.
> >
> >	Sam
> >  
> Hi Sam,
> 
> i did some testing and posted the results of a first patch that went 
> into the final direction.
> 
> In
> 
> https://lists.berlios.de/pipermail/socketcan-core/2008-December/002117.html
> 
> Kurt gave a "Reviewed-by:" and in a later mail in
> 
> https://lists.berlios.de/pipermail/socketcan-core/2008-December/002119.html
> 
> he wrote "my Acked-by: still stands" for the final patch.
> 
> So i choosed "Acked-by:".
> 
> As Kurt went that deep into the code that he pointed me to the right 
> line and presented a first idea for a patch, an 'Acked-by:' for 
> confirming the final patch looked like an appropriate credit to me.

OK and thanks for the clarification Oliver.

	Sam

Re: [PATCH] can: Fix CAN_(EFF|RTR)_FLAG handling in can_filter

[Kurt Van Dijck]

On Mon, Dec 01, 2008 at 07:42:31PM +0100, Oliver Hartkopp wrote:
> Sam Ravnborg wrote:
>> On Mon, Dec 01, 2008 at 05:15:42PM +0100, Oliver Hartkopp wrote:
>>   
>>>
>>> Thanks to Kurt Van Dijck for pointing at this issue and for the review.
>>>
>>> Signed-Off-by: Oliver Hartkopp <oliver@hartkopp.net>
>>> Acked-by: Kurt Van Dijck <kurt.van.dijck@eia.be>
>>>     
>>
>> If Kurt tested this patch then he should
>> be credited with a "Tested-by:" tag.
>>
>> I recall he did but I deleted the thread from my can mailbox.
>>
>> 	Sam
>>   
> Hi Sam,
>
> i did some testing and posted the results of a first patch that went  
> into the final direction.
>
> In
>
> https://lists.berlios.de/pipermail/socketcan-core/2008-December/002117.html
>
> Kurt gave a "Reviewed-by:" and in a later mail in
>
> https://lists.berlios.de/pipermail/socketcan-core/2008-December/002119.html
>
> he wrote "my Acked-by: still stands" for the final patch.
>
> So i choosed "Acked-by:".
>
> As Kurt went that deep into the code that he pointed me to the right  
> line and presented a first idea for a patch, an 'Acked-by:' for  
> confirming the final patch looked like an appropriate credit to me.
Well, I did not physically test Oliver's final patch, which looked totally
different.
By lack of time, did not take the effort of completely testing it again. The test
scenario included the problematic scenario, which made me think Oliver
understood the problem.
For the final patch, a 'Acked-by:' looks appropriate to me too.
Sam,
I'm pleased to notice the credits are watched this close too.
>
> Regards,
> Oliver
>
Kurt

Re: [PATCH] can: Fix CAN_(EFF|RTR)_FLAG handling in can_filter

[David Miller]

From: Oliver Hartkopp <oliver@hartkopp.net>
Date: Mon, 01 Dec 2008 17:15:42 +0100

> can: Fix CAN_(EFF|RTR)_FLAG handling in can_filter
> 
> Due to a wrong safety check in af_can.c it was not possible to filter
> for SFF frames with a specific CAN identifier without getting the
> same selected CAN identifier from a received EFF frame also.
> 
> This fix has a minimum (but user visible) impact on the CAN filter
> API and therefore the CAN version is set to a new date.
> 
> Indeed the 'old' API is still working as-is. But when now setting
> CAN_(EFF|RTR)_FLAG in can_filter.can_mask you might get less traffic
> than before - but still the stuff that you expected to get for your
> defined filter ...
> 
> Thanks to Kurt Van Dijck for pointing at this issue and for the review.
> 
> Signed-Off-by: Oliver Hartkopp <oliver@hartkopp.net>
> Acked-by: Kurt Van Dijck <kurt.van.dijck@eia.be>

Applied to net-2.6

But you are totally kidding yourself if you think that bumping
that completely stupid date string has any meaning at all for
userspace.

You're either keeping to the API you're exporting to userspace
or you're not.  And using a date string which userland has to
test is just a cop-out for not breaking userspace interfaces.

Re: [PATCH] can: Fix CAN_(EFF|RTR)_FLAG handling in can_filter

[Oliver Hartkopp]

David Miller wrote:
> From: Oliver Hartkopp <oliver@hartkopp.net>
> Date: Mon, 01 Dec 2008 17:15:42 +0100
>
>   
>> can: Fix CAN_(EFF|RTR)_FLAG handling in can_filter
>>
>>     
>
> Applied to net-2.6
>   

Thanks!

> But you are totally kidding yourself if you think that bumping
> that completely stupid date string has any meaning at all for
> userspace.
>
> You're either keeping to the API you're exporting to userspace
> or you're not.  And using a date string which userland has to
> test is just a cop-out for not breaking userspace interfaces.
>   

This fix does fortunately not break the userspace interface.

The version string is just for information and should not be used to 
determine changing userspace interfaces. Thinking about your concerns it 
may be the best approach to remove this version stuff entirely, before 
people begin to use it in any weird manner :-/

I'll make a RFC on the CAN ML if it can be removed.

Thanks for the feedback,

Oliver

[PATCH net-2.6] can: omit received RTR frames for single ID filter lists

[Oliver Hartkopp]

[-- Attachment #1: Type: text/plain, Size: 823 bytes --]

can: omit received RTR frames for single ID filter lists

Since commit d253eee20195b25e298bf162a6e72f14bf4803e5 the single CAN
identifier filter lists handle only non-RTR CAN frames.

So we need to omit the check of these filter lists when receiving RTR CAN frames.

Signed-Off-by: Oliver Hartkopp <oliver@hartkopp.net>
---

Hello Dave & Greg,

while writing a CAN filter test suite on a business trip i discovered a 
missing check that became necessary together with the referenced commit

'can: Fix CAN_(EFF|RTR)_FLAG handling in can_filter'

Sorry for the effort of this extra patch.

It should go into 2.6.28 before the bug fix window closes.

Additionally the patch applies down to 2.6.26. Greg please consider this patch 
together with the referenced one for stable .26 and .27 also.

Thanks & best regards,
Oliver



[-- Attachment #2: can_rx_filter.patch --]
[-- Type: text/x-patch, Size: 490 bytes --]

diff --git a/net/can/af_can.c b/net/can/af_can.c
index d8173e5..3dadb33 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -622,7 +622,10 @@ static int can_rcv_filter(struct dev_rcv_lists *d, struct sk_buff *skb)
 		}
 	}
 
-	/* check CAN_ID specific entries */
+	/* check filterlists for single non-RTR can_ids */
+	if (can_id & CAN_RTR_FLAG)
+		return matches;
+
 	if (can_id & CAN_EFF_FLAG) {
 		hlist_for_each_entry_rcu(r, n, &d->rx[RX_EFF], list) {
 			if (r->can_id == can_id) {

Re: [PATCH net-2.6] can: omit received RTR frames for single ID filter lists

[David Miller]

From: Oliver Hartkopp <oliver@hartkopp.net>
Date: Thu, 04 Dec 2008 18:40:40 +0100

> can: omit received RTR frames for single ID filter lists
> 
> Since commit d253eee20195b25e298bf162a6e72f14bf4803e5 the single CAN
> identifier filter lists handle only non-RTR CAN frames.
> 
> So we need to omit the check of these filter lists when receiving RTR CAN frames.
> 
> Signed-Off-by: Oliver Hartkopp <oliver@hartkopp.net>

Applied to net-2.6, thanks.

Please stop capitalizing the "Off"  in Signed-off-by, I keep
correcting that for you :-)

Thanks again.