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

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

[Oliver Hartkopp]

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

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

[Eric Dumazet]

On Wed, 2014-01-29 at 14:44 +0100, 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.

I think this is opening yet another can of bugs.

CAN calls dev_queue_xmit()  : This layer can zap skb->cb[] totally.

Please hold a reference on the socket instead. Thats a 10 lines patch,
instead of yet another complex patch.

void can_destructor(struct sk_buff *skb)
{
	sock_put(skb->sk);
}

void can_set_owner(struct sk_buff *skb, struct sock *sk)
{
	skb_orphan(skb);
	skb->destructor = can_skb_destructor;
	skb->sk = sk;
	sock_hold(sk);
}

Thats how every protocol does this, with variants.

Check l2tp_sock_wfree()/l2tp_skb_set_owner_w() for an example

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

[Eric Dumazet]

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.

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

[Eric Dumazet]

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.

diff --git a/net/can/af_can.c b/net/can/af_can.c
index d249874a366d..98d74bd6030f 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -290,7 +290,8 @@ int can_send(struct sk_buff *skb, int loop)
 				return -ENOMEM;
 			}
 
-			newskb->sk = skb->sk;
+			if (skb->sk)
+				can_skb_set_owner(newskb, skb->sk);
 			newskb->ip_summed = CHECKSUM_UNNECESSARY;
 			newskb->pkt_type = PACKET_BROADCAST;
 		}
diff --git a/net/can/af_can.h b/net/can/af_can.h
index 6de58b40535c..e9cd045ba1cc 100644
--- a/net/can/af_can.h
+++ b/net/can/af_can.h
@@ -45,6 +45,7 @@
 #include <linux/list.h>
 #include <linux/rcupdate.h>
 #include <linux/can.h>
+#include <net/sock.h>
 
 /* af_can rx dispatcher structures */
 
@@ -118,4 +119,17 @@ extern struct s_stats    can_stats;        /* packet statistics */
 extern struct s_pstats   can_pstats;       /* receive list statistics */
 extern struct hlist_head can_rx_dev_list;  /* rx dispatcher structures */
 
+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)
+{
+	skb_orphan(skb);
+	skb->destructor = can_skb_destructor;
+	skb->sk = sk;
+	sock_hold(sk);
+}
+
 #endif /* AF_CAN_H */
diff --git a/net/can/bcm.c b/net/can/bcm.c
index 3fc737b214c7..b31480f3a97c 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -59,6 +59,7 @@
 #include <linux/slab.h>
 #include <net/sock.h>
 #include <net/net_namespace.h>
+#include "af_can.h"
 
 /*
  * To send multiple CAN frame content within TX_SETUP or to filter
@@ -268,7 +269,8 @@ static void bcm_can_tx(struct bcm_op *op)
 
 	/* send with loopback */
 	skb->dev = dev;
-	skb->sk = op->sk;
+	if (op->sk)
+		can_skb_set_owner(skb, op->sk);
 	can_send(skb, 1);
 
 	/* update statistics */

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

[Eric Dumazet]

On Wed, 2014-01-29 at 14:44 +0100, Oliver Hartkopp wrote:

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

I find worrying it took 6 months for CAN developers to hit this problem.

A WARN_ON_ONCE() has almost no chance being noticed in the embedded
world, and makes no pressure on developers to fix the bugs.

Patch is 6 months old, and will be reverted when we have confidence that
all offenders are fixed. Your feedback shows that we shall wait 10 years
just to make sure, and maybe we wont revert it.

All BUG() in the kernel have the property to potentially halt a
perfectly working machine, at a bad time, yes. But they make us
build a better kernel.

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
>