Skip to contentSkip to conversation

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

PATCH Applied Oliver Hartkopp 5 participants 9 messages Dec 1, 2008 – Dec 4, 2008 1 message not in the archive

Subsystems (3)
Networking Network drivers CAN network layer
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Oliver Hartkopp

author
#1 lore raw

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

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 <[email protected]>
Acked-by Kurt Van Dijck <[email protected]>

---

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

include/linux/can/core.h +1 -1
index e9ca210..f50785a 100644
@@ -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"
net/can/af_can.c +48 -15

86 lines not on this page · raw message

net/can/bcm.c +4 -3
index d0dd382..da0d426 100644
@@ -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";
#2 lore raw
Quoted 14 lines · Oliver Hartkopp

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

Quoted 4 lines

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

Signed-off-by Oliver Hartkopp <[email protected]>
Acked-by Kurt Van Dijck <[email protected]>

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

maintainer
#3 lore raw
Quoted 9 lines · Sam Ravnborg
Quoted 6 lines · Oliver Hartkopp

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

Signed-off-by Oliver Hartkopp <[email protected]>
Acked-by Kurt Van Dijck <[email protected]>
Quoted 7 lines

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

#4 lore raw
Quoted 30 lines · Oliver Hartkopp
Quoted 9 lines · Sam Ravnborg
Quoted 6 lines · Oliver Hartkopp

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

Signed-off-by Oliver Hartkopp <[email protected]>
Acked-by Kurt Van Dijck <[email protected]>
Quoted 7 lines

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

Quoted 7 lines

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

#5 lore raw
In reply to Oliver Hartkopp
Quoted 30 lines · Oliver Hartkopp
Quoted 9 lines · Sam Ravnborg
Quoted 6 lines · Oliver Hartkopp

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

Signed-off-by Oliver Hartkopp <[email protected]>
Acked-by Kurt Van Dijck <[email protected]>
Quoted 7 lines

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

Quoted 7 lines

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.

Quoted 4 lines

Regards,
Oliver

Kurt

maintainer Applied
#6 lore raw
In reply to Oliver Hartkopp

From: Oliver Hartkopp <[email protected]>
Date: Mon, 01 Dec 2008 17:15:42 +0100

Quoted 14 lines

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

Quoted 4 lines

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

Signed-off-by Oliver Hartkopp <[email protected]>
Acked-by Kurt Van Dijck <[email protected]>

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.

author
#7 lore raw
Quoted 10 lines · David Miller

From: Oliver Hartkopp <[email protected]>
Date: Mon, 01 Dec 2008 17:15:42 +0100

Quoted 3 lines

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

Applied to net-2.6

Thanks!

Quoted 8 lines

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

author
#8 lore raw

PATCH 1/1 can: omit received RTR frames for single ID filter lists

In reply to a message not in the archive

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 <[email protected]>

---

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

net/can/af_can.c +4 -1
index d8173e5..3dadb33 100644
@@ -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) {
maintainer Applied
#9 lore raw

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

From: Oliver Hartkopp <[email protected]>
Date: Thu, 04 Dec 2008 18:40:40 +0100

Quoted 8 lines

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 <[email protected]>

Applied to net-2.6, thanks.

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

Thanks again.