Fix sloppy CAN_(EFF|RTR)_FLAG handling in can_filter.can_mask .

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 impact on the CAN filter API as the 'sloppy'
handling is still a correct (and possibly wanted?) use-case.

Please update the can-utils (especially candump) whose filter definition
on the commandline made assumptions to correct the user input that are
probably unwanted now.

Thanks to Kurt van Dijck for pointing at this issue!

Signed-Off-by: Oliver Hartkopp <oliver@hartkopp.net>
git-svn-id: svn://svn.berlios.de//socketcan/trunk@872 030b6a49-0b11-0410-94ab-b0dab22257f2

diff --git a/can-utils/candump.c b/can-utils/candump.c
index 940f81de5ec1193577231398ecf71e405674d223..f2762f9e0bc4cae3cc8f392b815e54d665ca1176 100644 (file)
--- a/can-utils/candump.c
+++ b/can-utils/candump.c
@@ -130,8 +130,9 @@ void print_usage(char *prg)
        fprintf(stderr, "%s -c -c -ta can0,123:7FF,400:700,#000000FF can2,400~7F0 can3 can8\n", prg);
        fprintf(stderr, "%s -l any,0~0,#FFFFFFFF    (log only error frames but no(!) data frames)\n", prg);
        fprintf(stderr, "%s -l any,0:0,#FFFFFFFF    (log error frames and also all data frames)\n", prg);
-       fprintf(stderr, "%s vcan2,92345678:9FFFFFFF (match only for extended CAN ID 12345678)\n", prg);
-       fprintf(stderr, "%s vcan2,12345678:1FFFFFFF (analog to above due to 8 digit value length)\n", prg);
+       fprintf(stderr, "%s vcan2,92345678:DFFFFFFF (match only for extended CAN ID 12345678)\n", prg);
+       fprintf(stderr, "%s vcan2,123:7FF (matches CAN ID 123 - including EFF and RTR frames)\n", prg);
+       fprintf(stderr, "%s vcan2,123:C00007FF (matches CAN ID 123 - only SFF and non-RTR frames)\n", prg);
        fprintf(stderr, "\n");
 }
 
@@ -189,21 +190,6 @@ int idx2dindex(int ifidx, int socket) {
        return i;
 }
 
-canid_t checkeff(char *ptr, char *nptr)
-{
-       int len;
-
-       if (nptr)
-               len = nptr - ptr;
-       else
-               len = strlen(ptr);
-
-       if (len == 17 && (ptr[8] == ':' || ptr[8] == '~'))
-               return CAN_EFF_FLAG;
-       else
-               return 0;
-}
-
 int main(int argc, char **argv)
 {
        fd_set rdfs;
@@ -219,7 +205,6 @@ int main(int argc, char **argv)
        int opt, ret;
        int currmax, numfilter;
        char *ptr, *nptr;
-       canid_t eff;
        struct sockaddr_can addr;
        struct can_filter rfilter[MAXFILTER];
        can_err_mask_t err_mask;
@@ -411,9 +396,6 @@ int main(int argc, char **argv)
                                           &rfilter[numfilter].can_id, 
                                           (long unsigned int *)
                                           &rfilter[numfilter].can_mask) == 2) {
-                                       eff = checkeff(ptr, nptr);
-                                       rfilter[numfilter].can_id |= eff;
-                                       rfilter[numfilter].can_mask |= eff;
                                        rfilter[numfilter].can_mask &= ~CAN_ERR_FLAG;
                                        numfilter++;
                                } else if (sscanf(ptr, "%lx~%lx",
@@ -422,9 +404,6 @@ int main(int argc, char **argv)
                                                  (long unsigned int *)
                                                  &rfilter[numfilter].can_mask) == 2) {
                                        rfilter[numfilter].can_id |= CAN_INV_FILTER;
-                                       eff = checkeff(ptr, nptr);
-                                       rfilter[numfilter].can_id |= eff;
-                                       rfilter[numfilter].can_mask |= eff;
                                        rfilter[numfilter].can_mask &= ~CAN_ERR_FLAG;
                                        numfilter++;
                                } else if (sscanf(ptr, "#%lx",

diff --git a/can-utils/isotpdump.c b/can-utils/isotpdump.c
index 22b81414e302f25c92b52e86cfcf254908f60393..c5c4de5afb703df96fe90f3e1c8489d171c203f7 100644 (file)
--- a/can-utils/isotpdump.c
+++ b/can-utils/isotpdump.c
@@ -162,17 +162,22 @@ int main(int argc, char **argv)
                return 1;
        }
 
-       rfilter[0].can_id   = src;
-       if (src & CAN_EFF_FLAG)
-               rfilter[0].can_mask = CAN_EFF_MASK | CAN_EFF_FLAG;
-       else
-               rfilter[0].can_mask = CAN_SFF_MASK;
-
-       rfilter[1].can_id   = dst;
-       if (dst & CAN_EFF_FLAG)
-               rfilter[1].can_mask = CAN_EFF_MASK | CAN_EFF_FLAG;
-       else
-               rfilter[1].can_mask = CAN_SFF_MASK;
+
+       if (src & CAN_EFF_FLAG) {
+               rfilter[0].can_id   = src & (CAN_EFF_MASK | CAN_EFF_FLAG);
+               rfilter[0].can_mask = (CAN_EFF_MASK|CAN_EFF_FLAG|CAN_RTR_FLAG);
+       } else {
+               rfilter[0].can_id   = src & CAN_SFF_MASK;
+               rfilter[0].can_mask = (CAN_SFF_MASK|CAN_EFF_FLAG|CAN_RTR_FLAG);
+       }
+
+       if (dst & CAN_EFF_FLAG) {
+               rfilter[1].can_id   = dst & (CAN_EFF_MASK | CAN_EFF_FLAG);
+               rfilter[1].can_mask = (CAN_EFF_MASK|CAN_EFF_FLAG|CAN_RTR_FLAG);
+       } else {
+               rfilter[1].can_id   = dst & CAN_SFF_MASK;
+               rfilter[1].can_mask = (CAN_SFF_MASK|CAN_EFF_FLAG|CAN_RTR_FLAG);
+       }
 
        setsockopt(s, SOL_CAN_RAW, CAN_RAW_FILTER, &rfilter, sizeof(rfilter));
 

diff --git a/kernel/2.6/include/linux/can/core.h b/kernel/2.6/include/linux/can/core.h
index f917c25923ee37656b942c3a9d3a415967946bc4..89f7c444b42c94cb14fc85d27c64cf664540b77c 100644 (file)
--- a/kernel/2.6/include/linux/can/core.h
+++ b/kernel/2.6/include/linux/can/core.h
@@ -21,7 +21,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/kernel/2.6/net/can/af_can.c b/kernel/2.6/net/can/af_can.c
index 71fd406f87ccf97b06fad5cf1cff94b316a842b6..d2b4bd0ba9e71911b9b0b12d89ddd4775011543d 100644 (file)
--- a/kernel/2.6/net/can/af_can.c
+++ b/kernel/2.6/net/can/af_can.c
@@ -390,23 +390,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;
@@ -419,15 +448,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/kernel/2.6/net/can/bcm.c b/kernel/2.6/net/can/bcm.c
index 9062a5fff3f288d4d1be41ed4548028034880618..1a389c6a0524f5ced0a7b82759f365f79aedbe57 100644 (file)
--- a/kernel/2.6/net/can/bcm.c
+++ b/kernel/2.6/net/can/bcm.c
@@ -75,12 +75,12 @@ RCSID("$Id$");
 #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"
 static __initdata const char banner[] = KERN_INFO
-       "can: broadcast manager protocol (rev " CAN_BCM_VERSION ")\n";
+       "can: broadcast manager protocol (" CAN_VERSION_STRING ")\n";
 
 MODULE_DESCRIPTION("PF_CAN broadcast manager protocol");
 MODULE_LICENSE("Dual BSD/GPL");

diff --git a/kernel/2.6/net/can/isotp.c b/kernel/2.6/net/can/isotp.c
index d5b58d1938ad265a2438c0fc4492ad2161b66e30..a84734b97dc31ec87734e8118e5474a97178dfb3 100644 (file)
--- a/kernel/2.6/net/can/isotp.c
+++ b/kernel/2.6/net/can/isotp.c
@@ -75,7 +75,7 @@
 #include <linux/can/version.h> /* for RCSID. Removed by mkpatch script */
 RCSID("$Id$");
 
-#define CAN_ISOTP_VERSION "20080902-alpha"
+#define CAN_ISOTP_VERSION "20081130-alpha"
 static __initdata const char banner[] =
        KERN_INFO "can: isotp protocol (rev " CAN_ISOTP_VERSION ")\n";
 
@@ -92,8 +92,9 @@ MODULE_AUTHOR("Oliver Hartkopp <oliver.hartkopp@volkswagen.de>");
 #undef DBG
 #define DBG(fmt, args...) 
 
-#define SINGLE_MASK(id) (id & CAN_EFF_FLAG) ? \
-       (CAN_EFF_MASK|CAN_EFF_FLAG) : CAN_SFF_MASK
+#define SINGLE_MASK(id) ((id & CAN_EFF_FLAG) ? \
+                        (CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG) : \
+                        (CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG))
 
 /* N_PCI type values in bits 7-4 of N_PCI bytes */
 #define N_PCI_SF 0x00  /* single frame */
@@ -816,6 +817,10 @@ static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len)
        if (addr->can_addr.tp.rx_id == addr->can_addr.tp.tx_id)
                return -EADDRNOTAVAIL;
 
+       if ((addr->can_addr.tp.rx_id | addr->can_addr.tp.tx_id) &
+           (CAN_ERR_FLAG | CAN_RTR_FLAG))
+               return -EADDRNOTAVAIL;
+
        if (!addr->can_ifindex)
                return -ENODEV;
 

diff --git a/test/tst-raw-filter.c b/test/tst-raw-filter.c
index 38e9ecdb0947ece576061881f0ca64a39f415a16..14335729d3468e6104abaf9aa8f9cd0cedba8df6 100644 (file)
--- a/test/tst-raw-filter.c
+++ b/test/tst-raw-filter.c
@@ -91,8 +91,8 @@ int main(int argc, char **argv)
                                fputs("too many filters\n", stderr);
                                break;
                        }
-                       rfilter[nfilters].can_id = strtol(strtok(optarg, ":"), NULL, 16);
-                       rfilter[nfilters].can_mask = strtol(strtok(NULL, ":"), NULL, 16);
+                       rfilter[nfilters].can_id = strtoul(strtok(optarg, ":"), NULL, 16);
+                       rfilter[nfilters].can_mask = strtoul(strtok(NULL, ":"), NULL, 16);
                        nfilters++;
                        break;
                default: