can: add destructor for self generated skbs

PATCH In review Oliver Hartkopp 6 participants 10 messages Jan 30, 2014 – Feb 16, 2014 1 message not in the archive

Subsystems (3)

Networking Network drivers CAN network drivers

lore

Share & tools

lore

Oliver Hartkopp

author

2014-01-30 09:11 UTC #0 lore raw

PATCH 1/1 can: add destructor for self generated skbs

Self generated skbuffs in net/can/bcm.c are setting a skb->sk reference but no explicit destructor which is enforced since Linux 3.11 with commit 376c7311bdb6 (net: add a temporary sanity check in skb_orphan()).

This patch adds some helper functions to make sure that a destructor is properly defined when a sock reference is assigned to a CAN related skb. To create an unshared skb owned by the original sock a common helper function has been introduced to replace open coded functions to create CAN echo skbs.

Signed-off-by Oliver Hartkopp <[email protected]>

Tested-by Andre Naujoks <[email protected]>

---

The patch applies properly down to Linux 3.9. For older kernels the include file include/linux/can/skb.h would need to be created.

Don't know if it's worth the effort to be ported back to Linux < v3.9 as we never had any issues in real world with this skb concept for the last eight years.

drivers/net/can/dev.c +3 ~~-12~~

index 13a9098..fc59bc6 100644
@@ -323,19 +323,10 @@ void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 	}
 
 	if (!priv->echo_skb[idx]) {
-		struct sock *srcsk = skb->sk;
 
-		if (atomic_read(&skb->users) != 1) {
-			struct sk_buff *old_skb = skb;
-
-			skb = skb_clone(old_skb, GFP_ATOMIC);
-			kfree_skb(old_skb);
-			if (!skb)
-				return;
-		} else
-			skb_orphan(skb);
-
-		skb->sk = srcsk;
+		skb = can_create_echo_skb(skb);
+		if (!skb)
+			return;
 
 		/* make settings for echo to reduce code in irq context */
 		skb->protocol = htons(ETH_P_CAN);

drivers/net/can/janz-ican3.c +4 ~~-14~~

index e24e669..2124c679 100644
@@ -18,6 +18,7 @@
 #include <linux/netdevice.h>
 #include <linux/can.h>
 #include <linux/can/dev.h>
+#include <linux/can/skb.h>
 #include <linux/can/error.h>
 
 #include <linux/mfd/janz.h>

@@ -1133,20 +1134,9 @@ static void ican3_handle_message(struct ican3_dev *mod, struct ican3_msg *msg)
  */
 static void ican3_put_echo_skb(struct ican3_dev *mod, struct sk_buff *skb)
 {
-	struct sock *srcsk = skb->sk;
-
-	if (atomic_read(&skb->users) != 1) {
-		struct sk_buff *old_skb = skb;
-
-		skb = skb_clone(old_skb, GFP_ATOMIC);
-		kfree_skb(old_skb);
-		if (!skb)
-			return;
-	} else {
-		skb_orphan(skb);
-	}
-
-	skb->sk = srcsk;
+	skb = can_create_echo_skb(skb);
+	if (!skb)
+		return;
 
 	/* save this skb for tx interrupt echo handling */
 	skb_queue_tail(&mod->echoq, skb);

drivers/net/can/vcan.c +4 -5

index 0a2a5ee..4e94057 100644
@@ -46,6 +46,7 @@
 #include <linux/if_ether.h>
 #include <linux/can.h>
 #include <linux/can/dev.h>
+#include <linux/can/skb.h>
 #include <linux/slab.h>
 #include <net/rtnetlink.h>

@@ -109,25 +110,23 @@ static netdev_tx_t vcan_tx(struct sk_buff *skb, struct net_device *dev)
 			stats->rx_packets++;
 			stats->rx_bytes += cfd->len;
 		}
-		kfree_skb(skb);
+		consume_skb(skb);
 		return NETDEV_TX_OK;
 	}
 
 	/* perform standard echo handling for CAN network interfaces */
 
 	if (loop) {
-		struct sock *srcsk = skb->sk;
 
-		skb = skb_share_check(skb, GFP_ATOMIC);
+		skb = can_create_echo_skb(skb);
 		if (!skb)
 			return NETDEV_TX_OK;
 
 		/* receive with packet counting */
-		skb->sk = srcsk;
 		vcan_rx(skb, dev);
 	} else {
 		/* no looped packets => no counting */
-		kfree_skb(skb);
+		consume_skb(skb);
 	}
 	return NETDEV_TX_OK;
 }

include/linux/can/skb.h +38 -0

index 2f0543f..f9bbbb4 100644
@@ -11,7 +11,9 @@
 #define CAN_SKB_H
 
 #include <linux/types.h>
+#include <linux/skbuff.h>
 #include <linux/can.h>
+#include <net/sock.h>
 
 /*
  * The struct can_skb_priv is used to transport additional information along

@@ -42,4 +44,40 @@ static inline void can_skb_reserve(struct sk_buff *skb)
 	skb_reserve(skb, sizeof(struct can_skb_priv));
 }
 
+static inline void can_skb_destructor(struct sk_buff *skb)
+{
+	sock_put(skb->sk);
+}
+
+static inline void can_skb_set_owner(struct sk_buff *skb, struct sock *sk)
+{
+	if (sk) {
+		sock_hold(sk);
+		skb->destructor = can_skb_destructor;
+		skb->sk = sk;
+	}
+}
+
+/*
+ * returns an unshared skb owned by the original sock to be echo'ed back
+ */
+static inline struct sk_buff *can_create_echo_skb(struct sk_buff *skb)
+{
+	if (skb_shared(skb)) {
+		struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
+
+		if (likely(nskb)) {
+			can_skb_set_owner(nskb, skb->sk);
+			consume_skb(skb);
+			return nskb;
+		} else {
+			kfree_skb(skb);
+			return NULL;
+		}
+	}
+
+	/* we can assume to have an unshared skb with proper owner */
+	return skb;
+}
+
 #endif /* CAN_SKB_H */

net/can/af_can.c +2 -1

index d249874..a27f8aa 100644
@@ -57,6 +57,7 @@
 #include <linux/skbuff.h>
 #include <linux/can.h>
 #include <linux/can/core.h>
+#include <linux/can/skb.h>
 #include <linux/ratelimit.h>
 #include <net/net_namespace.h>
 #include <net/sock.h>

@@ -290,7 +291,7 @@ int can_send(struct sk_buff *skb, int loop)
 				return -ENOMEM;
 			}
 
-			newskb->sk = skb->sk;
+			can_skb_set_owner(newskb, skb->sk);
 			newskb->ip_summed = CHECKSUM_UNNECESSARY;
 			newskb->pkt_type = PACKET_BROADCAST;
 		}

net/can/bcm.c +2 -2

index 3fc737b..dcb75c0 100644
@@ -268,7 +268,7 @@ static void bcm_can_tx(struct bcm_op *op)
 
 	/* send with loopback */
 	skb->dev = dev;
-	skb->sk = op->sk;
+	can_skb_set_owner(skb, op->sk);
 	can_send(skb, 1);
 
 	/* update statistics */

@@ -1223,7 +1223,7 @@ static int bcm_tx_send(struct msghdr *msg, int ifindex, struct sock *sk)
 
 	can_skb_prv(skb)->ifindex = dev->ifindex;
 	skb->dev = dev;
-	skb->sk  = sk;
+	can_skb_set_owner(skb, sk);
 	err = can_send(skb, 1); /* send with loopback */
 	dev_put(dev);

Marc Kleine-Budde

maintainer

2014-01-30 09:24 UTC #1 lore raw

Quoted 9 lines · Oliver Hartkopp

Self generated skbuffs in net/can/bcm.c are setting a skb->sk reference but no explicit destructor which is enforced since Linux 3.11 with commit 376c7311bdb6 (net: add a temporary sanity check in skb_orphan()).
This patch adds some helper functions to make sure that a destructor is properly defined when a sock reference is assigned to a CAN related skb. To create an unshared skb owned by the original sock a common helper function has been introduced to replace open coded functions to create CAN echo skbs.

Quoted 7 lines

Signed-off-by Oliver Hartkopp <[email protected]>
Tested-by Andre Naujoks <[email protected]>
---
The patch applies properly down to Linux 3.9. For older kernels the include file include/linux/can/skb.h would need to be created.

Should I add stable on Cc when applying this patch?

Marc

Signature · 4 lines

Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

Eric Dumazet

Reviewed-by on v1 1/1

2014-01-30 23:10 UTC #2 lore raw

In reply to Oliver Hartkopp

Quoted 9 lines · Oliver Hartkopp

Self generated skbuffs in net/can/bcm.c are setting a skb->sk reference but no explicit destructor which is enforced since Linux 3.11 with commit 376c7311bdb6 (net: add a temporary sanity check in skb_orphan()).
This patch adds some helper functions to make sure that a destructor is properly defined when a sock reference is assigned to a CAN related skb. To create an unshared skb owned by the original sock a common helper function has been introduced to replace open coded functions to create CAN echo skbs.

Quoted 4 lines

Signed-off-by Oliver Hartkopp <[email protected]>
Tested-by Andre Naujoks <[email protected]>
---

Reviewed-by Eric Dumazet <[email protected]>

David Miller

maintainer

2014-01-31 00:27 UTC #3 lore raw

In reply to Oliver Hartkopp

From: Oliver Hartkopp <[email protected]>
Date: Thu, 30 Jan 2014 10:11:28 +0100

Quoted 11 lines

Self generated skbuffs in net/can/bcm.c are setting a skb->sk reference but no explicit destructor which is enforced since Linux 3.11 with commit 376c7311bdb6 (net: add a temporary sanity check in skb_orphan()).
This patch adds some helper functions to make sure that a destructor is properly defined when a sock reference is assigned to a CAN related skb. To create an unshared skb owned by the original sock a common helper function has been introduced to replace open coded functions to create CAN echo skbs.
Signed-off-by Oliver Hartkopp <[email protected]>
Tested-by Andre Naujoks <[email protected]>

Applied and queued up for -stable, thanks.

A message that is not in the archive (sent off-list or not yet received): 20140130.162723.1124545320708055175.davem-fT/PcQaiUtIeIZ0/[email protected]

Oliver Hartkopp

2014-02-01 18:14 UTC #5 lore raw

[INFO] BCM and ISOTP crashes Linux 3.11 - 3.13 when running on REAL HW CAN interfaces

Hello all,

at 2013-08-01 Eric Dumazet created this patch to make sure some networking rules are enforced in the Linux Kernel in Linux 3.11:
http://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=376c7311bdb6efea3322310333576a04d73fbe4c

This affects CAN BCM and CAN ISOTP and leads to a KERNEL CRASH when you are *sending* with the BCM or ISOTP on REAL(!) CAN interfaces.

AFAIK there are not so many use cases. That's why it lasted 6 months to detect it :-( Virtual CAN and SLCAN interfaces do not have this problem (no echo skbs).

Do I have to care about this issue?

Usually the embedded systems do not have such a recent kernel. Regarding desktop distributions Redhat 7 runs a 3.10 kernel which is safe.

But e.g. OpenSuse 13.1 and Ubuntu 13.10 / Linux Mint Petra are based on Linux 3.11 and Debian Jessie (current Debian testing) is on Linux 3.12.

The latter have the described problem with BCM / ISOTP until this patch

http://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=0ae89beb283a0db5980d1d4781c7d7be2f2810d6

is applied by the stable kernel maintainers and distribution maintainers. The patch is already upstream for Linux 3.14 but it will take some time until it gets into the stable kernels 3.11 - 3.13 and the distributions.

As Linux 3.11 is already end-of-life [EOL] I'll address the Ubuntu and OpenSuse maintainers directly so that this patch for 3.11 does not get lost. David Miller already queued this patch up for -stable
Original post: http://marc.info/?l=linux-netdev&m=139107310226665&w=2

Take care,
Oliver

-------- Original Message --------
Subject: Re: [PATCH stable 3.9+] can: add destructor for self generated skbs
Date: Thu, 30 Jan 2014 16:27:23 -0800 (PST)
From: David Miller <davem-fT/PcQaiUtIeIZ0/[email protected]>
To: [email protected]
CC: [email protected], [email protected], [email protected], [email protected]

From: Oliver Hartkopp <[email protected]>
Date: Thu, 30 Jan 2014 10:11:28 +0100

Quoted 11 lines

Self generated skbuffs in net/can/bcm.c are setting a skb->sk reference but no explicit destructor which is enforced since Linux 3.11 with commit 376c7311bdb6 (net: add a temporary sanity check in skb_orphan()).
This patch adds some helper functions to make sure that a destructor is properly defined when a sock reference is assigned to a CAN related skb. To create an unshared skb owned by the original sock a common helper function has been introduced to replace open coded functions to create CAN echo skbs.
Signed-off-by Oliver Hartkopp <[email protected]>
Tested-by Andre Naujoks <[email protected]>

Applied and queued up for -stable, thanks.

Marc Kleine-Budde

maintainer

2014-02-01 18:44 UTC #6 lore raw

Re: [INFO] BCM and ISOTP crashes Linux 3.11 - 3.13 when running on REAL HW CAN interfaces

Quoted 24 lines · Oliver Hartkopp

Hello all,
at 2013-08-01 Eric Dumazet created this patch to make sure some networking rules are enforced in the Linux Kernel in Linux 3.11:
http://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=376c7311bdb6efea3322310333576a04d73fbe4c
This affects CAN BCM and CAN ISOTP and leads to a KERNEL CRASH when you are *sending* with the BCM or ISOTP on REAL(!) CAN interfaces.
AFAIK there are not so many use cases. That's why it lasted 6 months to detect it :-( Virtual CAN and SLCAN interfaces do not have this problem (no echo skbs).
Do I have to care about this issue?
Usually the embedded systems do not have such a recent kernel. Regarding desktop distributions Redhat 7 runs a 3.10 kernel which is safe.
But e.g. OpenSuse 13.1 and Ubuntu 13.10 / Linux Mint Petra are based on Linux 3.11 and Debian Jessie (current Debian testing) is on Linux 3.12.
The latter have the described problem with BCM / ISOTP until this patch
http://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=0ae89beb283a0db5980d1d4781c7d7be2f2810d6

Quoted 8 lines

is applied by the stable kernel maintainers and distribution maintainers. The patch is already upstream for Linux 3.14 but it will take some time until it gets into the stable kernels 3.11 - 3.13 and the distributions.
As Linux 3.11 is already end-of-life [EOL] I'll address the Ubuntu and OpenSuse maintainers directly so that this patch for 3.11 does not get lost. David Miller already queued this patch up for -stable
Original post: http://marc.info/?l=linux-netdev&m=139107310226665&w=2

At least the Ubuntu maintainers usually pick up the stable patches, so that they appear "automatically" in Ubuntu's stable. Good Work!

Marc

Signature · 4 lines

Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

Luis Henriques

2014-02-02 18:00 UTC #7 lore raw

Re: [INFO] BCM and ISOTP crashes Linux 3.11 - 3.13 when running on REAL HW CAN interfaces

In reply to Oliver Hartkopp

Quoted 28 lines · Oliver Hartkopp

Hello all,
at 2013-08-01 Eric Dumazet created this patch to make sure some networking rules are enforced in the Linux Kernel in Linux 3.11:
http://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=376c7311bdb6efea3322310333576a04d73fbe4c
This affects CAN BCM and CAN ISOTP and leads to a KERNEL CRASH when you are *sending* with the BCM or ISOTP on REAL(!) CAN interfaces.
AFAIK there are not so many use cases. That's why it lasted 6 months to detect it :-( Virtual CAN and SLCAN interfaces do not have this problem (no echo skbs).
Do I have to care about this issue?
Usually the embedded systems do not have such a recent kernel. Regarding desktop distributions Redhat 7 runs a 3.10 kernel which is safe.
But e.g. OpenSuse 13.1 and Ubuntu 13.10 / Linux Mint Petra are based on Linux 3.11 and Debian Jessie (current Debian testing) is on Linux 3.12.
The latter have the described problem with BCM / ISOTP until this patch
http://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=0ae89beb283a0db5980d1d4781c7d7be2f2810d6
is applied by the stable kernel maintainers and distribution maintainers. The patch is already upstream for Linux 3.14 but it will take some time until it gets into the stable kernels 3.11 - 3.13 and the distributions.

Quoted 7 lines

As Linux 3.11 is already end-of-life [EOL] I'll address the Ubuntu and OpenSuse maintainers directly so that this patch for 3.11 does not get lost. David Miller already queued this patch up for -stable
Original post: http://marc.info/?l=linux-netdev&m=139107310226665&w=2
Take care,
Oliver

Hi Oliver,

Thanks a lot for the heads-up on this issue.

I've just checked David Miller's net -stable queue[1] and it looks like he has in fact already queued it. We usually pick his stable patches when he sends them to the stable mailing list, so I guess we'll just wait for this to happen.

[1] http://patchwork.ozlabs.org/bundle/davem/stable/?state=*

Cheers,
--
Luis

Quoted 27 lines

-------- Original Message --------
Subject: Re: [PATCH stable 3.9+] can: add destructor for self generated skbs
Date: Thu, 30 Jan 2014 16:27:23 -0800 (PST)
From: David Miller <[email protected]>
To: [email protected]
CC: [email protected], [email protected], [email protected], [email protected]
From: Oliver Hartkopp <[email protected]>
Date: Thu, 30 Jan 2014 10:11:28 +0100
Quoted 11 lines
Self generated skbuffs in net/can/bcm.c are setting a skb->sk reference but no explicit destructor which is enforced since Linux 3.11 with commit 376c7311bdb6 (net: add a temporary sanity check in skb_orphan()).
This patch adds some helper functions to make sure that a destructor is properly defined when a sock reference is assigned to a CAN related skb. To create an unshared skb owned by the original sock a common helper function has been introduced to replace open coded functions to create CAN echo skbs.
Signed-off-by Oliver Hartkopp <[email protected]>
Tested-by Andre Naujoks <[email protected]>
Applied and queued up for -stable, thanks.

Oliver Hartkopp

2014-02-02 19:29 UTC #8 lore raw

Re: [INFO] BCM and ISOTP crashes Linux 3.11 - 3.13 when running on REAL HW CAN interfaces

Quoted 6 lines · Luis Henriques

I've just checked David Miller's net -stable queue[1] and it looks like he has in fact already queued it. We usually pick his stable patches when he sends them to the stable mailing list, so I guess we'll just wait for this to happen.
[1] http://patchwork.ozlabs.org/bundle/davem/stable/?state=*

Hi Luis,

I was not aware about Daves stable queue URL and especially not about the process for the end-of-life kernels. Good to know that you check the lists yourself for potential relevant stuff :-)

Many thanks for your work!

Best regards,
Oliver

Oliver Hartkopp

author

2014-02-15 17:42 UTC #9 lore raw

Netdev stable patches status

Hello Dave,

there are many patches queued up for stable but not hitting the stable kernels for a while now (even when they're already upstream).

http://patchwork.ozlabs.org/bundle/davem/stable/?state=*

Regards,
Oliver

Quoted 20 lines · Oliver Hartkopp

Quoted 6 lines · Luis Henriques
I've just checked David Miller's net -stable queue[1] and it looks like he has in fact already queued it. We usually pick his stable patches when he sends them to the stable mailing list, so I guess we'll just wait for this to happen.
[1] http://patchwork.ozlabs.org/bundle/davem/stable/?state=*
Hi Luis,
I was not aware about Daves stable queue URL and especially not about the process for the end-of-life kernels. Good to know that you check the lists yourself for potential relevant stuff :-)
Many thanks for your work!
Best regards,
Oliver

David Miller

maintainer

2014-02-16 00:07 UTC #10 lore raw

Re: Netdev stable patches status

From: Oliver Hartkopp <[email protected]>
Date: Sat, 15 Feb 2014 18:42:29 +0100

Quoted 6 lines

Hello Dave,
there are many patches queued up for stable but not hitting the stable kernels for a while now (even when they're already upstream).
http://patchwork.ozlabs.org/bundle/davem/stable/?state=*

The delay can be 2 or 3 weeks, and is of a time of my own choosing.

I allow patches to "cook" in that queue for a minimum of several days. They do not just automatically just get sent to -stable.