Re: [PATCH stable 3.11+] can: use private sk reference to detect originating socket

Re: [PATCH stable 3.11+] can: use private sk reference to detect originating socket

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 10518 bytes --]

Please put linux-can on Cc for CAN related patches.

Marc

On 01/29/2014 02:44 PM, Oliver Hartkopp wrote:
> Commit 376c7311bdb6 (net: add a temporary sanity check in skb_orphan())
> leads to a BUG in can_put_echo_skb() when skb_orphan() is executed.
> 
> The use of skb->sk to detect the originating socket can lead to side effects
> in net/core and net/sched. Therefore the reference needed in net/can/raw.c is
> now stored in a CAN private skbuff space in struct can_skb_priv.
> 
> The reference which comes with the CAN frame is only needed in raw_rcv() and
> checked against his own sock pointer and does only special things if it
> matches. So removing the socket does not hurt as the in-flight skbs are
> removed and the receive filters are purged anyway when the socket disappears.
> 
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> 
> ---
> 
> Here's my idea to fix up the original skb->sk handling concept for detecting
> the originating socket instance. The patch applies properly down to 3.11.
> 
> Eric please take a look if I used sock_alloc_send_skb() correctly in bcm.c.
> Andre please check if you see any functional differences on your machine.
> 
> Tnx & regards,
> Oliver
> 
> ps. I would suggest to move the BUG() in WARN_ON_ONCE() to fulfill the
> requirement to detect a possible misuse but not kill the entire machine.
> BUG() is intended to be used when there's no way to continue any reasonable
> operation. Detecting and fixing this issue half a year after the "temporary
> sanity check" has been integrated is a bad example for using BUG() IMO.
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index 13a9098..6d51fb7 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -323,7 +323,6 @@ 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;
> @@ -335,8 +334,6 @@ void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
>  		} else
>  			skb_orphan(skb);
>  
> -		skb->sk = srcsk;
> -
>  		/* make settings for echo to reduce code in irq context */
>  		skb->protocol = htons(ETH_P_CAN);
>  		skb->pkt_type = PACKET_BROADCAST;
> @@ -513,6 +510,7 @@ struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf)
>  
>  	can_skb_reserve(skb);
>  	can_skb_prv(skb)->ifindex = dev->ifindex;
> +	can_skb_prv(skb)->src_sk = NULL;
>  
>  	*cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame));
>  	memset(*cf, 0, sizeof(struct can_frame));
> diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
> index e24e669..d27a1c9 100644
> --- a/drivers/net/can/janz-ican3.c
> +++ b/drivers/net/can/janz-ican3.c
> @@ -1133,8 +1133,6 @@ 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;
>  
> @@ -1142,11 +1140,8 @@ static void ican3_put_echo_skb(struct ican3_dev *mod, struct sk_buff *skb)
>  		kfree_skb(old_skb);
>  		if (!skb)
>  			return;
> -	} else {
> +	} else
>  		skb_orphan(skb);
> -	}
> -
> -	skb->sk = srcsk;
>  
>  	/* save this skb for tx interrupt echo handling */
>  	skb_queue_tail(&mod->echoq, skb);
> diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
> index 3fcdae2..44766c18 100644
> --- a/drivers/net/can/slcan.c
> +++ b/drivers/net/can/slcan.c
> @@ -215,6 +215,7 @@ static void slc_bump(struct slcan *sl)
>  
>  	can_skb_reserve(skb);
>  	can_skb_prv(skb)->ifindex = sl->dev->ifindex;
> +	can_skb_prv(skb)->src_sk = NULL;
>  
>  	memcpy(skb_put(skb, sizeof(struct can_frame)),
>  	       &cf, sizeof(struct can_frame));
> diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c
> index 0a2a5ee..1a89116 100644
> --- a/drivers/net/can/vcan.c
> +++ b/drivers/net/can/vcan.c
> @@ -116,14 +116,11 @@ static netdev_tx_t vcan_tx(struct sk_buff *skb, struct net_device *dev)
>  	/* perform standard echo handling for CAN network interfaces */
>  
>  	if (loop) {
> -		struct sock *srcsk = skb->sk;
> -
>  		skb = skb_share_check(skb, GFP_ATOMIC);
>  		if (!skb)
>  			return NETDEV_TX_OK;
>  
>  		/* receive with packet counting */
> -		skb->sk = srcsk;
>  		vcan_rx(skb, dev);
>  	} else {
>  		/* no looped packets => no counting */
> diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
> index 2f0543f..a8dc078 100644
> --- a/include/linux/can/skb.h
> +++ b/include/linux/can/skb.h
> @@ -25,10 +25,12 @@
>  /**
>   * struct can_skb_priv - private additional data inside CAN sk_buffs
>   * @ifindex:	ifindex of the first interface the CAN frame appeared on
> + * @src_sk:	pointer to the originating sock to detect self generated skbs
>   * @cf:		align to the following CAN frame at skb->data
>   */
>  struct can_skb_priv {
>  	int ifindex;
> +	struct sock *src_sk;
>  	struct can_frame cf[0];
>  };
>  
> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index d249874..6ed6a23 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -270,15 +270,6 @@ int can_send(struct sk_buff *skb, int loop)
>  		/* indication for the CAN driver: do loopback */
>  		skb->pkt_type = PACKET_LOOPBACK;
>  
> -		/*
> -		 * The reference to the originating sock may be required
> -		 * by the receiving socket to check whether the frame is
> -		 * its own. 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.
> -		 */
> -
>  		if (!(skb->dev->flags & IFF_ECHO)) {
>  			/*
>  			 * If the interface is not capable to do loopback
> @@ -290,7 +281,6 @@ int can_send(struct sk_buff *skb, int loop)
>  				return -ENOMEM;
>  			}
>  
> -			newskb->sk = skb->sk;
>  			newskb->ip_summed = CHECKSUM_UNNECESSARY;
>  			newskb->pkt_type = PACKET_BROADCAST;
>  		}
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index 3fc737b..f10f521 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -245,7 +245,9 @@ static void bcm_can_tx(struct bcm_op *op)
>  {
>  	struct sk_buff *skb;
>  	struct net_device *dev;
> +	struct sock *sk = op->sk;
>  	struct can_frame *cf = &op->frames[op->currframe];
> +	int err;
>  
>  	/* no target device? => exit */
>  	if (!op->ifindex)
> @@ -257,18 +259,23 @@ static void bcm_can_tx(struct bcm_op *op)
>  		return;
>  	}
>  
> -	skb = alloc_skb(CFSIZ + sizeof(struct can_skb_priv), gfp_any());
> +	/*
> +	 * we are probably in softirq context (timer, net-rx):
> +	 * get an unblocked skb
> +	 */
> +	sk->sk_allocation = GFP_ATOMIC;
> +	skb = sock_alloc_send_skb(sk, CFSIZ + sizeof(struct can_skb_priv),
> +				  1, &err);
>  	if (!skb)
>  		goto out;
>  
>  	can_skb_reserve(skb);
> -	can_skb_prv(skb)->ifindex = dev->ifindex;
> -
>  	memcpy(skb_put(skb, CFSIZ), cf, CFSIZ);
>  
>  	/* send with loopback */
> +	can_skb_prv(skb)->ifindex = dev->ifindex;
> +	can_skb_prv(skb)->src_sk = sk;
>  	skb->dev = dev;
> -	skb->sk = op->sk;
>  	can_send(skb, 1);
>  
>  	/* update statistics */
> @@ -1203,12 +1210,13 @@ static int bcm_tx_send(struct msghdr *msg, int ifindex, struct sock *sk)
>  	if (!ifindex)
>  		return -ENODEV;
>  
> -	skb = alloc_skb(CFSIZ + sizeof(struct can_skb_priv), GFP_KERNEL);
> +	/* get an unblocked skb */
> +	skb = sock_alloc_send_skb(sk, CFSIZ + sizeof(struct can_skb_priv),
> +				  1, &err);
>  	if (!skb)
>  		return -ENOMEM;
>  
>  	can_skb_reserve(skb);
> -
>  	err = memcpy_fromiovec(skb_put(skb, CFSIZ), msg->msg_iov, CFSIZ);
>  	if (err < 0) {
>  		kfree_skb(skb);
> @@ -1221,10 +1229,11 @@ static int bcm_tx_send(struct msghdr *msg, int ifindex, struct sock *sk)
>  		return -ENODEV;
>  	}
>  
> -	can_skb_prv(skb)->ifindex = dev->ifindex;
> +	/* send with loopback */
> +	can_skb_prv(skb)->ifindex = ifindex;
> +	can_skb_prv(skb)->src_sk = sk;
>  	skb->dev = dev;
> -	skb->sk  = sk;
> -	err = can_send(skb, 1); /* send with loopback */
> +	err = can_send(skb, 1);
>  	dev_put(dev);
>  
>  	if (err)
> diff --git a/net/can/raw.c b/net/can/raw.c
> index 07d72d8..7e67560 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -113,12 +113,13 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
>  {
>  	struct sock *sk = (struct sock *)data;
>  	struct raw_sock *ro = raw_sk(sk);
> +	struct sock *src_sk = can_skb_prv(oskb)->src_sk;
>  	struct sockaddr_can *addr;
>  	struct sk_buff *skb;
>  	unsigned int *pflags;
>  
>  	/* check the received tx sock reference */
> -	if (!ro->recv_own_msgs && oskb->sk == sk)
> +	if (!ro->recv_own_msgs && src_sk == sk)
>  		return;
>  
>  	/* do not pass frames with DLC > 8 to a legacy socket */
> @@ -150,9 +151,9 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
>  	/* add CAN specific message flags for raw_recvmsg() */
>  	pflags = raw_flags(skb);
>  	*pflags = 0;
> -	if (oskb->sk)
> +	if (src_sk)
>  		*pflags |= MSG_DONTROUTE;
> -	if (oskb->sk == sk)
> +	if (src_sk == sk)
>  		*pflags |= MSG_CONFIRM;
>  
>  	if (sock_queue_rcv_skb(sk, skb) < 0)
> @@ -705,17 +706,15 @@ static int raw_sendmsg(struct kiocb *iocb, struct socket *sock,
>  		goto put_dev;
>  
>  	can_skb_reserve(skb);
> -	can_skb_prv(skb)->ifindex = dev->ifindex;
> -
>  	err = memcpy_fromiovec(skb_put(skb, size), msg->msg_iov, size);
>  	if (err < 0)
>  		goto free_skb;
>  
>  	sock_tx_timestamp(sk, &skb_shinfo(skb)->tx_flags);
>  
> +	can_skb_prv(skb)->ifindex = dev->ifindex;
> +	can_skb_prv(skb)->src_sk = sk;
>  	skb->dev = dev;
> -	skb->sk  = sk;
> -
>  	err = can_send(skb, ro->loopback);
>  
>  	dev_put(dev);
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: [PATCH stable 3.11+] can: use private sk reference to detect originating socket

[Oliver Hartkopp]

On 29.01.2014 16:47, Eric Dumazet wrote:
> On Wed, 2014-01-29 at 07:30 -0800, Eric Dumazet wrote:
>> On Wed, 2014-01-29 at 07:02 -0800, Eric Dumazet wrote:
>>
>>> Thats how every protocol does this, with variants.
>>>
>>> Check l2tp_sock_wfree()/l2tp_skb_set_owner_w() for an example
>>
>> Also keep in mind this patch is needed for all kernels, not only 3.11+
>>
>> So better keep it as simple as possible, to ease backports.
> 
> Please try the following patch.
> 

Hello Eric,

there were at least two problems with your patch:

1. Only the stuff in net/can was addressed (missing drivers/net/can)

2. The check for sk and the additional skb_orphan() is not needed as the
can_skb_set_owner() is invoked only after 'fresh' skb allocation or after
clone/skb_orphan() - so we know the skb state very good.

I moved your suggested inline functions to include/linux/can/skb.h where
all users (net/can and drivers) can access them.

So what has been done:

- can_skb_set_owner() is invoked on new created skbuffs (tx path)
- can_skb_set_owner() is invoked on orphaned/cloned skbs (echo in drvs)

I did some tests with real CAN hardware and everything seems fine.

Please take a look if it looks correct to you.

If so I'll send a proper patch for it.

Regards,
Oliver

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 13a9098..17a0a00 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -325,17 +325,20 @@ 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;
+		if (skb_shared(skb)) {
+			struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
 
-			skb = skb_clone(old_skb, GFP_ATOMIC);
-			kfree_skb(old_skb);
-			if (!skb)
+			if (likely(nskb))
+				consume_skb(skb);
+			else {
+				kfree_skb(skb);
 				return;
+			}
+			skb = nskb;
 		} else
 			skb_orphan(skb);
 
-		skb->sk = srcsk;
+		can_skb_set_owner(skb, srcsk);
 
 		/* make settings for echo to reduce code in irq context */
 		skb->protocol = htons(ETH_P_CAN);
diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
index e24e669..ee48cb1 100644
--- a/drivers/net/can/janz-ican3.c
+++ b/drivers/net/can/janz-ican3.c
@@ -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>
@@ -1135,18 +1136,20 @@ 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;
+	if (skb_shared(skb)) {
+		struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
 
-		skb = skb_clone(old_skb, GFP_ATOMIC);
-		kfree_skb(old_skb);
-		if (!skb)
+		if (likely(nskb))
+			consume_skb(skb);
+		else {
+			kfree_skb(skb);
 			return;
-	} else {
+		}
+		skb = nskb;
+	} else
 		skb_orphan(skb);
-	}
 
-	skb->sk = srcsk;
+	can_skb_set_owner(skb, srcsk);
 
 	/* save this skb for tx interrupt echo handling */
 	skb_queue_tail(&mod->echoq, skb);
diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c
index 0a2a5ee..f764f00 100644
--- a/drivers/net/can/vcan.c
+++ b/drivers/net/can/vcan.c
@@ -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>
 
@@ -118,12 +119,22 @@ static netdev_tx_t vcan_tx(struct sk_buff *skb, struct net_device *dev)
 	if (loop) {
 		struct sock *srcsk = skb->sk;
 
-		skb = skb_share_check(skb, GFP_ATOMIC);
-		if (!skb)
-			return NETDEV_TX_OK;
+		if (skb_shared(skb)) {
+			struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
+
+			if (likely(nskb))
+				consume_skb(skb);
+			else {
+				kfree_skb(skb);
+				return NETDEV_TX_OK;
+			}
+			skb = nskb;
+		} else
+			skb_orphan(skb);
+
+		can_skb_set_owner(skb, srcsk);
 
 		/* receive with packet counting */
-		skb->sk = srcsk;
 		vcan_rx(skb, dev);
 	} else {
 		/* no looped packets => no counting */
diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
index 2f0543f..0429d36 100644
--- a/include/linux/can/skb.h
+++ b/include/linux/can/skb.h
@@ -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,16 @@ 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)
+{
+	sock_hold(sk);
+	skb->destructor = can_skb_destructor;
+	skb->sk = sk;
+}
+
 #endif /* CAN_SKB_H */
diff --git a/net/can/af_can.c b/net/can/af_can.c
index d249874..7e4101b 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -290,7 +290,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;
 		}
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 3fc737b..dcb75c0 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -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);
 

Re: [PATCH stable 3.11+] can: use private sk reference to detect originating socket

[Eric Dumazet]

On Wed, 2014-01-29 at 20:30 +0100, Oliver Hartkopp wrote:
> 
> On 29.01.2014 16:47, Eric Dumazet wrote:
> > On Wed, 2014-01-29 at 07:30 -0800, Eric Dumazet wrote:
> >> On Wed, 2014-01-29 at 07:02 -0800, Eric Dumazet wrote:
> >>
> >>> Thats how every protocol does this, with variants.
> >>>
> >>> Check l2tp_sock_wfree()/l2tp_skb_set_owner_w() for an example
> >>
> >> Also keep in mind this patch is needed for all kernels, not only 3.11+
> >>
> >> So better keep it as simple as possible, to ease backports.
> > 
> > Please try the following patch.
> > 
> 
> Hello Eric,
> 
> there were at least two problems with your patch:
> 
> 1. Only the stuff in net/can was addressed (missing drivers/net/can)
> 
> 2. The check for sk and the additional skb_orphan() is not needed as the
> can_skb_set_owner() is invoked only after 'fresh' skb allocation or after
> clone/skb_orphan() - so we know the skb state very good.

Yep, but then you added skb_orphan() calls. when skb is not shared.

> 
> I moved your suggested inline functions to include/linux/can/skb.h where
> all users (net/can and drivers) can access them.
> 
> So what has been done:
> 
> - can_skb_set_owner() is invoked on new created skbuffs (tx path)
> - can_skb_set_owner() is invoked on orphaned/cloned skbs (echo in drvs)
> 
> I did some tests with real CAN hardware and everything seems fine.
> 
> Please take a look if it looks correct to you.
> 
> If so I'll send a proper patch for it.
> 
> Regards,
> Oliver
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index 13a9098..17a0a00 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -325,17 +325,20 @@ void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
>  	if (!priv->echo_skb[idx]) {
>  		struct sock *srcsk = skb->sk;
>  

This is still buggy.

You should not consume_skb() before having 
a ref count on skb->sk.


> -		if (atomic_read(&skb->users) != 1) {
> -			struct sk_buff *old_skb = skb;
> +		if (skb_shared(skb)) {
> +			struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
>  
> -			skb = skb_clone(old_skb, GFP_ATOMIC);
> -			kfree_skb(old_skb);
> -			if (!skb)
> +			if (likely(nskb))
> +				consume_skb(skb);
> +			else {
> +				kfree_skb(skb);
>  				return;
> +			}
> +			skb = nskb;
>  		} else
>  			skb_orphan(skb);
>  
> -		skb->sk = srcsk;
> +		can_skb_set_owner(skb, srcsk);

Only after this point you can do the :

consume_skb(original_skb);



>  
>  		/* make settings for echo to reduce code in irq context */
>  		skb->protocol = htons(ETH_P_CAN);
> diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
> index e24e669..ee48cb1 100644
> --- a/drivers/net/can/janz-ican3.c
> +++ b/drivers/net/can/janz-ican3.c
> @@ -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>
> @@ -1135,18 +1136,20 @@ static void ican3_put_echo_skb(struct ican3_dev *mod, struct sk_buff *skb)
>  {
>  	struct sock *srcsk = skb->sk;
>  

Same problem here.

> -	if (atomic_read(&skb->users) != 1) {
> -		struct sk_buff *old_skb = skb;
> +	if (skb_shared(skb)) {
> +		struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
>  
> -		skb = skb_clone(old_skb, GFP_ATOMIC);
> -		kfree_skb(old_skb);
> -		if (!skb)
> +		if (likely(nskb))
> +			consume_skb(skb);
> +		else {
> +			kfree_skb(skb);
>  			return;
> -	} else {
> +		}
> +		skb = nskb;
> +	} else
>  		skb_orphan(skb);
> -	}
>  
> -	skb->sk = srcsk;
> +	can_skb_set_owner(skb, srcsk);
>  
>  	/* save this skb for tx interrupt echo handling */
>  	skb_queue_tail(&mod->echoq, skb);
> diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c
> index 0a2a5ee..f764f00 100644
> --- a/drivers/net/can/vcan.c
> +++ b/drivers/net/can/vcan.c
> @@ -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>
>  
> @@ -118,12 +119,22 @@ static netdev_tx_t vcan_tx(struct sk_buff *skb, struct net_device *dev)
>  	if (loop) {
>  		struct sock *srcsk = skb->sk;
>  

Same problem

> -		skb = skb_share_check(skb, GFP_ATOMIC);
> -		if (!skb)
> -			return NETDEV_TX_OK;
> +		if (skb_shared(skb)) {
> +			struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);
> +
> +			if (likely(nskb))
> +				consume_skb(skb);
> +			else {
> +				kfree_skb(skb);
> +				return NETDEV_TX_OK;
> +			}
> +			skb = nskb;
> +		} else
> +			skb_orphan(skb);
> +
> +		can_skb_set_owner(skb, srcsk);
>  
>  		/* receive with packet counting */
> -		skb->sk = srcsk;
>  		vcan_rx(skb, dev);
>  	} else {
>  		/* no looped packets => no counting */


I think you really should have a helper instead of copying this 3 times.


/*
 * like skb_share_check(), but transfert the skb->sk ownership
 */
static inline struct sk_buff *can_skb_share_check(struct sk_buff *skb)
{
	if (skb_shared(skb)) {
		struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);

		if (likely(nskb)) {
			if (skb->sk)
				can_skb_set_owner(nskb, skb->sk);
			consume_skb(skb);
			return nskb;
		}
		kfree_skb(skb);
		return NULL;
	}
	return skb;
}

Re: [PATCH stable 3.11+] can: use private sk reference to detect originating socket

[Oliver Hartkopp]

On 29.01.2014 20:55, Eric Dumazet wrote:

>> 2. The check for sk and the additional skb_orphan() is not needed as the
>> can_skb_set_owner() is invoked only after 'fresh' skb allocation or after
>> clone/skb_orphan() - so we know the skb state very good.
> 
> Yep, but then you added skb_orphan() calls. when skb is not shared.

I did that e.g. in the sources placed in driver/net/can because I assume that
I can not be sure that I always get skbs created by AF_CAN.

E.g. one could also use AF_PACKET to send CAN frames to the CAN interfaces.

Btw. assuming AF_PACKET does it correct too with the destructor I will omit
the skb_orphan() / can_skb_set_owner() sequence in drivers/net/can in the next
attempt.

Tnx
Oliver

> 
> I think you really should have a helper instead of copying this 3 times.
> 

ok

Re: [PATCH stable 3.11+] can: use private sk reference to detect originating socket

[Oliver Hartkopp]

So here we are:

Introduced can_create_echo_skb() which creates a skb which is properly
owned to be send back (echo'ed) into the network stack. 

Regards,
Oliver

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 13a9098..fc59bc6 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -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);
diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
index e24e669..2124c679 100644
--- a/drivers/net/can/janz-ican3.c
+++ b/drivers/net/can/janz-ican3.c
@@ -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);
diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c
index 0a2a5ee..4e94057 100644
--- a/drivers/net/can/vcan.c
+++ b/drivers/net/can/vcan.c
@@ -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;
 }
diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
index 2f0543f..f9bbbb4 100644
--- a/include/linux/can/skb.h
+++ b/include/linux/can/skb.h
@@ -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 */
diff --git a/net/can/af_can.c b/net/can/af_can.c
index d249874..a27f8aa 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -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;
 		}
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 3fc737b..dcb75c0 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -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);
 




On 29.01.2014 21:40, Oliver Hartkopp wrote:
> On 29.01.2014 20:55, Eric Dumazet wrote:
> 
>>> 2. The check for sk and the additional skb_orphan() is not needed as the
>>> can_skb_set_owner() is invoked only after 'fresh' skb allocation or after
>>> clone/skb_orphan() - so we know the skb state very good.
>>
>> Yep, but then you added skb_orphan() calls. when skb is not shared.
> 
> I did that e.g. in the sources placed in driver/net/can because I assume that
> I can not be sure that I always get skbs created by AF_CAN.
> 
> E.g. one could also use AF_PACKET to send CAN frames to the CAN interfaces.
> 
> Btw. assuming AF_PACKET does it correct too with the destructor I will omit
> the skb_orphan() / can_skb_set_owner() sequence in drivers/net/can in the next
> attempt.
> 
> Tnx
> Oliver
> 
>>
>> I think you really should have a helper instead of copying this 3 times.
>>
> 
> ok
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH stable 3.11+] can: use private sk reference to detect originating socket

[Eric Dumazet]

On Wed, 2014-01-29 at 22:00 +0100, Oliver Hartkopp wrote:
> So here we are:
> 
> Introduced can_create_echo_skb() which creates a skb which is properly
> owned to be send back (echo'ed) into the network stack. 
> 
> Regards,
> Oliver

This seems much better ;)

Re: [PATCH stable 3.11+] can: use private sk reference to detect originating socket

[Oliver Hartkopp]

On 29.01.2014 22:06, Eric Dumazet wrote:
> On Wed, 2014-01-29 at 22:00 +0100, Oliver Hartkopp wrote:
>> So here we are:
>>
>> Introduced can_create_echo_skb() which creates a skb which is properly
>> owned to be send back (echo'ed) into the network stack. 
>>
>> Regards,
>> Oliver
> 
> This seems much better ;)
> 

Great!

I'll give it some more testing on my Laptop and will send the patch with a
proper description tomorrow morning.

Thanks for your support fixing this issue!

Regards,
Oliver

Re: [PATCH stable 3.11+] can: use private sk reference to detect originating socket

[Andre Naujoks]

On 29.01.2014 22:00, schrieb Oliver Hartkopp:
> So here we are:
> 
> Introduced can_create_echo_skb() which creates a skb which is properly
> owned to be send back (echo'ed) into the network stack. 


I see no crashes with this and it works as expected.

You can add my:

Tested-by: Andre Naujoks <nautsch2@gmail.com>

Regards
  Andre
> 
> Regards,
> Oliver
>