Oliver Hartkopp [off-list ref] writes:
2. The loopback indication is done by using the unused skb->protocol in
the tx path.
I don't think we should (mis-)use skb->protocol as a loopback flag.
As the name says, it's the protocol number and not a flag whether
loopback is to be done by the driver. Even if it's not used otherwise
in tx path (haven't checked this), in my experience it is asking for
future trouble to use variables other than for it's intended purpose.
IMO it would be better to skb->pkt_type. This is used to indicate
packet type to rcv functions registered by dev_add_pack(). It is set
by netdevice drivers to PACKET_{MULTICAST,BROADCAST,HOST,OTHER} for
received packets. In the send path it is set to PACKET_OUTGOING on
the copy of the skbuff that is delivered to the sockets registered on
ptype_all (typically packet sockets from tcpdump or other sniffers).
AFAICS, pkt_type is not used otherwise in the send path.
We could set skb->pkt_type = PACKET_LOOPBACK to flag to the CAN
netdevice driver whether to loop back the packet.
Any objections?
urs
Urs Thuermann wrote:
Oliver Hartkopp [off-list ref] writes:
quoted
2. The loopback indication is done by using the unused skb->protocol in
the tx path.
I don't think we should (mis-)use skb->protocol as a loopback flag.
Yep! After reading the comments from Patrick and Urs i definitely agree
to their concerns.
IMO it would be better to skb->pkt_type. This is used to indicate
packet type to rcv functions registered by dev_add_pack(). It is set
by netdevice drivers to PACKET_{MULTICAST,BROADCAST,HOST,OTHER} for
received packets. In the send path it is set to PACKET_OUTGOING on
the copy of the skbuff that is delivered to the sockets registered on
ptype_all (typically packet sockets from tcpdump or other sniffers).
AFAICS, pkt_type is not used otherwise in the send path.
We could set skb->pkt_type = PACKET_LOOPBACK to flag to the CAN
netdevice driver whether to loop back the packet.
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
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 {