Re: [patch 5/7] CAN: Add virtual CAN netdevice driver

[Oliver Hartkopp <socketcan@hartkopp.net>]

Oliver Hartkopp wrote:
> I think, it goes into the right direction to use skb->pkt_type. The flag
> should really be somewhere inside the skb as all back references into
> the sk would become sticky in the implementation.
>
> But regarding the use of skb->pkt_type i would suggest to take a closer
> look on the definitions in include/linux/if_packet.h and how the
> pkt_type is to be used inside the kernel. In my opinion we should use ...
>
> * TX-Path:
> PACKET_OTHERHOST: send the CAN frame without loopback
> PACKET_BROADCAST : send the CAN frame with loopback (aka local broadcast)
>
> See an example of this approach in drivers/net/arcnet/rfc1051.c :
> http://www.linux-m32r.org/lxr/http/source/drivers/net/arcnet/rfc1051.c?a=i386#L99
>
> * RX-Path:
> PACKET_HOST : just an incoming CAN frame for this host
>
> Any comments? ACKs?
>
> Best regards,
> Oliver
>
>
>   

The updated changes would look like this. Of course it has been tested
and is working fine :-)

Regards,
Oliver

Index: net/can/af_can.c
===================================================================
--- net/can/af_can.c    (revision 325)
+++ net/can/af_can.c    (working copy)
@@ -257,7 +257,6 @@
  */
 int can_send(struct sk_buff *skb, int loop)
 {
-    struct sock **tx_sk = (struct sock **)skb->cb;
     int err;
 
     if (skb->dev->type != ARPHRD_CAN) {
@@ -265,30 +264,43 @@
         return -EPERM;
     }
 
+    skb->protocol = htons(ETH_P_CAN);
+
     if (loop) {
         /* local loopback of sent CAN frames (default) */
 
-        /* indication for the CAN driver: do loopback */
-        *tx_sk = skb->sk;
+        /*
+         * This packet is not only sent on the CAN bus but
+         * also broadcasted to local subscribers on this host.
+         */
+        skb->pkt_type = PACKET_BROADCAST;
 
         /*
-         * The reference to the originating sock may be also required
+         * The reference to the originating sock may be required
          * by the receiving socket to indicate (and ignore) his own
-         * sent data. Example: can_raw sockopt CAN_RAW_RECV_OWN_MSGS
+         * sent data. Example: can_raw sockopt CAN_RAW_RECV_OWN_MSGS.
+         * Therefore we have to ensure that skb->sk remains the
+         * reference to the originating sock by restoring skb->sk
+         * after each skb_clone() or skb_orphan() usage.
+         * skb->sk is usually unused and unset in the rx path.
          */
 
         /* interface not capabable to do the loopback itself? */
         if (!(skb->dev->flags & IFF_LOOPBACK)) {
+            struct sock *srcsk = skb->sk;
             struct sk_buff *newskb = skb_clone(skb, GFP_ATOMIC);
 
             /* perform the local loopback here */
-            newskb->protocol  = htons(ETH_P_CAN);
             newskb->ip_summed = CHECKSUM_UNNECESSARY;
+            newskb->sk = srcsk;
             netif_rx(newskb);
         }
     } else {
-        /* indication for the CAN driver: no loopback required */
-        *tx_sk = NULL;
+        /*
+         * Indication for the CAN driver: No loopback required!
+         * This packet is only transmitted to the CAN bus.
+         */
+        skb->pkt_type = PACKET_OTHERHOST;
     }
 
     if (!(skb->dev->flags & IFF_UP))
@@ -581,10 +593,12 @@
 
 static inline void deliver(struct sk_buff *skb, struct receiver *r)
 {
+    struct sock *srcsk = skb->sk;
     struct sk_buff *clone = skb_clone(skb, GFP_ATOMIC);
 
     DBG("skbuff %p cloned to %p\n", skb, clone);
     if (clone) {
+        clone->sk = srcsk;
         r->func(clone, r->data);
         r->matches++;
     }
Index: net/can/raw.c
===================================================================
--- net/can/raw.c    (revision 325)
+++ net/can/raw.c    (working copy)
@@ -154,7 +154,7 @@
 
     if (!ro->recv_own_msgs) {
         /* check the received tx sock reference */
-        if (*(struct sock **)skb->cb == sk) {
+        if (skb->sk == sk) {
             DBG("trashed own tx msg\n");
             kfree_skb(skb);
             return;
Index: drivers/net/can/vcan.c
===================================================================
--- drivers/net/can/vcan.c    (revision 325)
+++ drivers/net/can/vcan.c    (working copy)
@@ -133,6 +133,7 @@
     skb->protocol  = htons(ETH_P_CAN);
     skb->dev       = dev;
     skb->ip_summed = CHECKSUM_UNNECESSARY;
+    skb->pkt_type  = PACKET_HOST;
 
     DBG("received skbuff on interface %d\n", dev->ifindex);
 
@@ -149,8 +150,8 @@
     stats->tx_packets++;
     stats->tx_bytes += skb->len;
 
-    /* tx socket reference pointer: Loopback required if not NULL */
-    loop = *(struct sock **)skb->cb != NULL;
+    /* indication for CAN netdevice drivers that loopback is required */
+    loop = (skb->pkt_type == PACKET_BROADCAST);
 
     if (!loopback) {
         /* no loopback handling available inside this driver */
@@ -170,6 +171,8 @@
     /* perform standard loopback handling for CAN network interfaces */
 
     if (loop) {
+        struct sock *srcsk = skb->sk;
+
         if (atomic_read(&skb->users) != 1) {
             struct sk_buff *old_skb = skb;
 
@@ -183,6 +186,8 @@
         } else
             skb_orphan(skb);
 
+        skb->sk = srcsk;
+
         /* receive with packet counting */
         vcan_rx(skb, dev);
     } else {