netdev - Re: Deadlock, L2TP over IP are not working, 3.4.1

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 10 Jun 2012 15:31:01 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Hong zhi guo <honkiko@...il.com>
Cc:	davem@...emloft.net, Denys Fedoryshchenko <denys@...p.net.lb>,
	Benjamin LaHaise <bcrl@...ck.org>,
	Francois Romieu <romieu@...zoreil.com>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: Deadlock, L2TP over IP are not working, 3.4.1

On Sun, 2012-06-10 at 20:14 +0800, Hong zhi guo wrote:
> Is there any conclusion about the problem?  Will bh_lock_sock_nested
> fix the lockdep violation?

lockdep violation could indeed be fixed like that, but LLTX seems more
appropriate.

But there is still a bug because skb->len is used after 
l2tp_xmit_skb(session, skb, session->hdr_len);

There is also a bug in l2tp_core.c, because it assumes RX path is not
reentrant. That should be fixed eventually.

I believe we could avoid percpu stuf and new locking, adding the
following unions in net_device_stats. It seems enough to have machine
long words stats, no need to force 64bit stats on 32bit arches.

 include/linux/netdevice.h |   40 ++++++++++++++++++++++++++++--------
 net/l2tp/l2tp_eth.c       |   29 +++++++++++++-------------
 2 files changed, 47 insertions(+), 22 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index d94cb14..1dee75a 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -171,14 +171,38 @@ static inline bool dev_xmit_complete(int rc)
  */
 
 struct net_device_stats {
-	unsigned long	rx_packets;
-	unsigned long	tx_packets;
-	unsigned long	rx_bytes;
-	unsigned long	tx_bytes;
-	unsigned long	rx_errors;
-	unsigned long	tx_errors;
-	unsigned long	rx_dropped;
-	unsigned long	tx_dropped;
+	union {
+		unsigned long	rx_packets;
+		atomic_long_t   rx_packets_atomic;
+	};
+	union {
+		unsigned long	tx_packets;
+		atomic_long_t	tx_packets_atomic;
+	};
+	union {
+		unsigned long	rx_bytes;
+		atomic_long_t	rx_bytes_atomic;
+	};
+	union {
+		unsigned long	tx_bytes;
+		atomic_long_t	tx_bytes_atomic;
+	};
+	union {
+		unsigned long	rx_errors;
+		atomic_long_t	rx_errors_atomic;
+	};
+	union {
+		unsigned long	tx_errors;
+		atomic_long_t	tx_errors_atomic;
+	};
+	union {
+		unsigned long	rx_dropped;
+		atomic_long_t	rx_dropped_atomic;
+	};
+	union {
+		unsigned long	tx_dropped;
+		atomic_long_t	tx_dropped_atomic;
+	};
 	unsigned long	multicast;
 	unsigned long	collisions;
 	unsigned long	rx_length_errors;
diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
index 185f12f..9e80ec4 100644
--- a/net/l2tp/l2tp_eth.c
+++ b/net/l2tp/l2tp_eth.c
@@ -83,20 +83,20 @@ static void l2tp_eth_dev_uninit(struct net_device *dev)
 	dev_put(dev);
 }
 
-static int l2tp_eth_dev_xmit(struct sk_buff *skb, struct net_device *dev)
+static netdev_tx_t l2tp_eth_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct l2tp_eth *priv = netdev_priv(dev);
 	struct l2tp_session *session = priv->session;
 
-	l2tp_xmit_skb(session, skb, session->hdr_len);
+	atomic_long_add(skb->len, &dev->stats.tx_bytes_atomic);
+	atomic_long_inc(&dev->stats.tx_packets_atomic);
 
-	dev->stats.tx_bytes += skb->len;
-	dev->stats.tx_packets++;
+	l2tp_xmit_skb(session, skb, session->hdr_len);
 
-	return 0;
+	return NETDEV_TX_OK;
 }
 
-static struct net_device_ops l2tp_eth_netdev_ops = {
+static const struct net_device_ops l2tp_eth_netdev_ops = {
 	.ndo_init		= l2tp_eth_dev_init,
 	.ndo_uninit		= l2tp_eth_dev_uninit,
 	.ndo_start_xmit		= l2tp_eth_dev_xmit,
@@ -106,8 +106,9 @@ static void l2tp_eth_dev_setup(struct net_device *dev)
 {
 	ether_setup(dev);
 	dev->priv_flags &= ~IFF_TX_SKB_SHARING;
-	dev->netdev_ops		= &l2tp_eth_netdev_ops;
-	dev->destructor		= free_netdev;
+	dev->features	|= NETIF_F_LLTX;
+	dev->netdev_ops	= &l2tp_eth_netdev_ops;
+	dev->destructor	= free_netdev;
 }
 
 static void l2tp_eth_dev_recv(struct l2tp_session *session, struct sk_buff *skb, int data_len)
@@ -139,15 +140,15 @@ static void l2tp_eth_dev_recv(struct l2tp_session *session, struct sk_buff *skb,
 	nf_reset(skb);
 
 	if (dev_forward_skb(dev, skb) == NET_RX_SUCCESS) {
-		dev->stats.rx_packets++;
-		dev->stats.rx_bytes += data_len;
-	} else
-		dev->stats.rx_errors++;
-
+		atomic_long_inc(&dev->stats.rx_packets_atomic);
+		atomic_long_add(data_len, &dev->stats.rx_bytes_atomic);
+	} else {
+		atomic_long_inc(&dev->stats.rx_errors_atomic);
+	}
 	return;
 
 error:
-	dev->stats.rx_errors++;
+	atomic_long_inc(&dev->stats.rx_errors_atomic);
 	kfree_skb(skb);
 }
 


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html