netdev - [PATCH net] net: enforce xmit_recursion for devices with a queue

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-next>] [day] [month] [year] [list]

Message-Id: <6d9ed6c448a5c855e05abf19c205f33a66b6ff40.1552557395.git.sd@queasysnail.net>
Date:   Thu, 14 Mar 2019 11:15:50 +0100
From:   Sabrina Dubroca <sd@...asysnail.net>
To:     netdev@...r.kernel.org
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        Sabrina Dubroca <sd@...asysnail.net>,
        Jianlin Shi <jishi@...hat.com>,
        Stefano Brivio <sbrivio@...hat.com>
Subject: [PATCH net] net: enforce xmit_recursion for devices with a queue

Commit 745e20f1b626 ("net: add a recursion limit in xmit path")
introduced a recursion limit, but it only applies to devices without a
queue. Virtual devices with a queue (either because they don't have
the IFF_NO_QUEUE flag, or because the administrator added one) can
still cause an unbounded recursion, via __dev_queue_xmit ->
__dev_xmit_skb -> qdisc_run -> __qdisc_run -> qdisc_restart ->
sch_direct_xmit -> dev_hard_start_xmit . Jianlin reported this in a
setup with 16 gretap devices stacked on top of one another.

This patch prevents the stack overflow by incrementing xmit_recursion in
code paths that can call dev_hard_start_xmit() (like commit 745e20f1b626
did). If the recursion limit is exceeded, the packet is enqueued and the
qdisc is scheduled.

Reported-by: Jianlin Shi <jishi@...hat.com>
Signed-off-by: Sabrina Dubroca <sd@...asysnail.net>
Reviewed-by: Stefano Brivio <sbrivio@...hat.com>
---
No fixes tag, I think this is at least as old as what 745e20f1b626
("net: add a recursion limit in xmit path") fixed.

 include/net/pkt_sched.h | 14 ++++++++++++++
 net/core/dev.c          |  4 +++-
 net/sched/sch_generic.c |  7 +++++++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
index a16fbe9a2a67..9384c1749bf3 100644
--- a/include/net/pkt_sched.h
+++ b/include/net/pkt_sched.h
@@ -113,6 +113,20 @@ bool sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
 		     struct net_device *dev, struct netdev_queue *txq,
 		     spinlock_t *root_lock, bool validate);
 
+static inline bool sch_direct_xmit_rec(struct sk_buff *skb, struct Qdisc *q,
+				       struct net_device *dev,
+				       struct netdev_queue *txq,
+				       spinlock_t *root_lock, bool validate)
+{
+	bool ret;
+
+	__this_cpu_inc(xmit_recursion);
+	ret = sch_direct_xmit(skb, q, dev, txq, root_lock, validate);
+	__this_cpu_dec(xmit_recursion);
+
+	return ret;
+}
+
 void __qdisc_run(struct Qdisc *q);
 
 static inline void qdisc_run(struct Qdisc *q)
diff --git a/net/core/dev.c b/net/core/dev.c
index 2b67f2aa59dd..74b77a773712 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3493,6 +3493,8 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
 		__qdisc_drop(skb, &to_free);
 		rc = NET_XMIT_DROP;
 	} else if ((q->flags & TCQ_F_CAN_BYPASS) && !qdisc_qlen(q) &&
+		   likely(__this_cpu_read(xmit_recursion) <=
+			  XMIT_RECURSION_LIMIT) &&
 		   qdisc_run_begin(q)) {
 		/*
 		 * This is a work-conserving queue; there are no old skbs
@@ -3502,7 +3504,7 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
 
 		qdisc_bstats_update(q, skb);
 
-		if (sch_direct_xmit(skb, q, dev, txq, root_lock, true)) {
+		if (sch_direct_xmit_rec(skb, q, dev, txq, root_lock, true)) {
 			if (unlikely(contended)) {
 				spin_unlock(&q->busylock);
 				contended = false;
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index a117d9260558..2565ef18d4cc 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -395,6 +395,12 @@ void __qdisc_run(struct Qdisc *q)
 	int quota = dev_tx_weight;
 	int packets;
 
+	if (unlikely(__this_cpu_read(xmit_recursion) > XMIT_RECURSION_LIMIT)) {
+		__netif_schedule(q);
+		return;
+	}
+
+	__this_cpu_inc(xmit_recursion);
 	while (qdisc_restart(q, &packets)) {
 		/*
 		 * Ordered by possible occurrence: Postpone processing if
@@ -407,6 +413,7 @@ void __qdisc_run(struct Qdisc *q)
 			break;
 		}
 	}
+	__this_cpu_dec(xmit_recursion);
 }
 
 unsigned long dev_trans_start(struct net_device *dev)
-- 
2.21.0