can: use private sk reference to detect originating socket
Subsystems (3)
PATCH 1/1 can: use private sk reference to detect originating socket
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.
---
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.
drivers/net/can/dev.c +1 -3
@@ -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 ));
drivers/net/can/janz-ican3.c +1 -6
@@ -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);
drivers/net/can/slcan.c +1 -0
@@ -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 ));
drivers/net/can/vcan.c +0 -3
@@ -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 */
include/linux/can/skb.h +2 -0
@@ -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 ]; };
net/can/af_can.c +0 -10
@@ -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 ; }
net/can/bcm.c +18 -9
@@ -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)
net/can/raw.c +6 -7
@@ -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);
Marc Kleine-Budde
maintainerPlease put linux-can on Cc for CAN related patches.
Marc
Quoted 293 lines · Oliver Hartkopp
Commit
376c7311bdb6(net: add a temporary sanity check inskb_orphan()) leads to a BUG incan_put_echo_skb()whenskb_orphan()is executed.The use of
skb->skto detect the originating socket can lead to side effects in net/core and net/sched. Therefore the reference needed innet/can/raw.cis now stored in a CAN private skbuff space in structcan_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 <[email protected]>---
Here's my idea to fix up the original
skb->skhandling 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 inbcm.c. Andre please check if you see any functional differences on your machine.Tnx & regards,
Oliverps. I would suggest to move the
BUG()inWARN_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 usingBUG()IMO.drivers/net/can/dev.c +1
-3@@ -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 ));drivers/net/can/janz-ican3.c +1
-6@@ -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);drivers/net/can/slcan.c +1
-0@@ -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 ));drivers/net/can/vcan.c +0
-3@@ -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 */ include/linux/can/skb.h +2
-0@@ -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 ]; };net/can/af_can.c +0
-10@@ -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 ; }net/can/bcm.c +18
-9@@ -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)net/can/raw.c +6
-8@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 |
Quoted 14 lines · Oliver Hartkopp
Commit
376c7311bdb6(net: add a temporary sanity check inskb_orphan()) leads to a BUG incan_put_echo_skb()whenskb_orphan()is executed.The use of
skb->skto detect the originating socket can lead to side effects in net/core and net/sched. Therefore the reference needed innet/can/raw.cis now stored in a CAN private skbuff space in structcan_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 <[email protected]>
Quoted 7 lines
---
Here's my idea to fix up the original
skb->skhandling 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 inbcm.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
Quoted 3 lines · Eric Dumazet
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.
Quoted 9 lines · Eric Dumazet
Quoted 3 lines · Eric Dumazet
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.
net/can/af_can.c +2 -1
@@ -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 ; }
net/can/af_can.h +14 -0
@@ -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 */
net/can/bcm.c +3 -1
@@ -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 */
Quoted 5 lines · Eric Dumazet
Quoted 4 lines · Eric Dumazet
Quoted 2 lines · Eric Dumazet
Thats how every protocol does this, with variants.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
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
drivers/net/can/dev.c +9 -6
@@ -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 );
drivers/net/can/janz-ican3.c +11 -8
@@ -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);
drivers/net/can/vcan.c +15 -4
@@ -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 */
include/linux/can/skb.h +14 -0
@@ -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 */
net/can/af_can.c +1 -1
@@ -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 ; }
net/can/bcm.c +2 -2
@@ -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);
Quoted 18 lines · Oliver Hartkopp
Quoted 5 lines · Eric Dumazet
Quoted 4 lines · Eric Dumazet
Quoted 2 lines · Eric Dumazet
Thats how every protocol does this, with variants.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
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,
Quoted 7 lines
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 thecan_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.
Quoted 23 lines
I moved your suggested inline functions to
include/linux/can/skb.hwhere 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,
Oliverdrivers/net/can/dev.c +0
-0
drivers/net/can/dev.c void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, · line 327
Quoted 3 lines
drivers/net/can/dev.c +0
-0@@ -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.
Quoted 10 lines
drivers/net/can/dev.c +4
-5-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);
drivers/net/can/dev.c void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, · line 341
Quoted 10 lines
drivers/net/can/dev.c +5
-1+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);
Quoted 16 lines
drivers/net/can/dev.c +0
-0/* make settings for echo to reduce code in irq context */ skb->protocol =htons (ETH_P_CAN );drivers/net/can/janz-ican3.c +1
-0@@ -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>
drivers/net/can/janz-ican3.c static void ican3_put_echo_skb(struct ican3_dev *mod, struct sk_buff *skb) · line 1138
Quoted 3 lines
drivers/net/can/janz-ican3.c +0
-0@@ -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.
Quoted 38 lines
drivers/net/can/janz-ican3.c +10
-8-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);drivers/net/can/vcan.c +1
-0@@ -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>
drivers/net/can/vcan.c static netdev_tx_t vcan_tx(struct sk_buff *skb, struct net_device *dev) · line 121
Quoted 3 lines
drivers/net/can/vcan.c +0
-0@@ -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
Quoted 13 lines
drivers/net/can/vcan.c +10
-3- 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;
drivers/net/can/vcan.c static netdev_tx_t vcan_tx(struct sk_buff *skb, struct net_device *dev) · line 140
Quoted 10 lines
drivers/net/can/vcan.c +4
-1+ }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;
}Quoted 5 lines · Eric Dumazet
Quoted 3 lines
2. The check for sk and the additional
skb_orphan()is not needed as thecan_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
Quoted 3 lines
I think you really should have a helper instead of copying this 3 times.
ok
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
drivers/net/can/dev.c +3 -12
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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);
Quoted 31 lines · Oliver Hartkopp
Quoted 5 lines · Eric Dumazet
Quoted 3 lines
2. The check for sk and the additional
skb_orphan()is not needed as thecan_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_PACKETto send CAN frames to the CAN interfaces.Btw. assuming
AF_PACKETdoes it correct too with the destructor I will omit theskb_orphan()/can_skb_set_owner()sequence indrivers/net/canin the next attempt.Tnx
OliverQuoted 3 lines
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 7 lines · 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
This seems much better ;)
Quoted 11 lines · Eric Dumazet
Quoted 7 lines · 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,
OliverThis 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
Andre Naujoks
Tested-byOn 29.01.2014 22:00, schrieb Oliver Hartkopp:
Quoted 4 lines
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:
Regards
Andre
Quoted 4 lines
Regards,
Oliver
Quoted 5 lines · Oliver Hartkopp
ps. I would suggest to move the
BUG()inWARN_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 usingBUG()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.