From: Wen Yang <wenyang@linux.alibaba.com>
To: davem@davemloft.net
Cc: zhiche.yy@alibaba-inc.com, xlpang@linux.alibaba.com,
Wen Yang <wenyang@linux.alibaba.com>,
Eric Dumazet <edumazet@google.com>,
Cong Wang <xiyou.wangcong@gmail.com>,
Jamal Hadi Salim <jhs@mojatatu.com>,
John Fastabend <john.fastabend@gmail.com>,
Kevin Athey <kda@google.com>, Xiaotian Pei <xiaotian@google.com>,
netdev@vger.kernel.org, bpf@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: [PATCH] net: core: fix unbalanced qdisc_run_begin/qdisc_run_end
Date: Sun, 10 Nov 2019 10:01:49 +0800 [thread overview]
Message-ID: <20191110020149.65307-1-wenyang@linux.alibaba.com> (raw)
3598 static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
3599 struct net_device *dev,
3600 struct netdev_queue *txq)
3601 {
...
3650 } else if ((q->flags & TCQ_F_CAN_BYPASS) && !qdisc_qlen(q) &&
3651 qdisc_run_begin(q)) {
---> Those multiple *and conditions* in this if statement are not
necessarily executed sequentially. If the qdisc_run_begin(q)
statement is executed first and the other conditions are not
satisfied, qdisc_run_end will have no chance to be executed,
and the lowest bit of q->running will always be 1.
This may lead to a softlockup:
https://bugzilla.kernel.org/show_bug.cgi?id=205427
...
3657
3658 qdisc_bstats_update(q, skb);
...
3661 if (unlikely(contended)) {
3662 spin_unlock(&q->busylock);
3663 contended = false;
3664 }
3665 __qdisc_run(q);
3666 }
3667
3668 qdisc_run_end(q);
3669 rc = NET_XMIT_SUCCESS;
3670 }
...
We ensure the correct execution order by explicitly
specifying those dependencies.
Fixes: edb09eb17ed8 ("net: sched: do not acquire qdisc spinlock in qdisc/class stats dump")
Signed-off-by: Wen Yang <wenyang@linux.alibaba.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Kevin Athey <kda@google.com>
Cc: Xiaotian Pei <xiaotian@google.com>
Cc: netdev@vger.kernel.org
Cc: bpf@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
net/core/dev.c | 63 ++++++++++++++++++++++++++++++----------------------------
1 file changed, 33 insertions(+), 30 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 20c7a67..d2690ee 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3602,27 +3602,28 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
spinlock_t *root_lock = qdisc_lock(q);
struct sk_buff *to_free = NULL;
bool contended;
- int rc;
+ int rc = NET_XMIT_SUCCESS;
qdisc_calculate_pkt_len(skb, q);
if (q->flags & TCQ_F_NOLOCK) {
- if ((q->flags & TCQ_F_CAN_BYPASS) && q->empty &&
- qdisc_run_begin(q)) {
- if (unlikely(test_bit(__QDISC_STATE_DEACTIVATED,
- &q->state))) {
- __qdisc_drop(skb, &to_free);
- rc = NET_XMIT_DROP;
- goto end_run;
- }
- qdisc_bstats_cpu_update(q, skb);
+ if ((q->flags & TCQ_F_CAN_BYPASS) && q->empty) {
+ if (qdisc_run_begin(q)) {
+ if (unlikely(test_bit(__QDISC_STATE_DEACTIVATED,
+ &q->state))) {
+ __qdisc_drop(skb, &to_free);
+ rc = NET_XMIT_DROP;
+ goto end_run;
+ }
+ qdisc_bstats_cpu_update(q, skb);
- rc = NET_XMIT_SUCCESS;
- if (sch_direct_xmit(skb, q, dev, txq, NULL, true))
- __qdisc_run(q);
+ if (sch_direct_xmit(skb, q, dev, txq, NULL,
+ true))
+ __qdisc_run(q);
end_run:
- qdisc_run_end(q);
+ qdisc_run_end(q);
+ }
} else {
rc = q->enqueue(skb, q, &to_free) & NET_XMIT_MASK;
qdisc_run(q);
@@ -3647,26 +3648,28 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
if (unlikely(test_bit(__QDISC_STATE_DEACTIVATED, &q->state))) {
__qdisc_drop(skb, &to_free);
rc = NET_XMIT_DROP;
- } else if ((q->flags & TCQ_F_CAN_BYPASS) && !qdisc_qlen(q) &&
- qdisc_run_begin(q)) {
- /*
- * This is a work-conserving queue; there are no old skbs
- * waiting to be sent out; and the qdisc is not running -
- * xmit the skb directly.
- */
+ } else if ((q->flags & TCQ_F_CAN_BYPASS) && !qdisc_qlen(q)) {
+ if (qdisc_run_begin(q)) {
+ /* This is a work-conserving queue;
+ * there are no old skbs waiting to be sent out;
+ * and the qdisc is not running -
+ * xmit the skb directly.
+ */
- qdisc_bstats_update(q, skb);
+ qdisc_bstats_update(q, skb);
- if (sch_direct_xmit(skb, q, dev, txq, root_lock, true)) {
- if (unlikely(contended)) {
- spin_unlock(&q->busylock);
- contended = false;
+ if (sch_direct_xmit(skb, q, dev, txq, root_lock,
+ true)) {
+ if (unlikely(contended)) {
+ spin_unlock(&q->busylock);
+ contended = false;
+ }
+ __qdisc_run(q);
}
- __qdisc_run(q);
- }
- qdisc_run_end(q);
- rc = NET_XMIT_SUCCESS;
+ qdisc_run_end(q);
+ rc = NET_XMIT_SUCCESS;
+ }
} else {
rc = q->enqueue(skb, q, &to_free) & NET_XMIT_MASK;
if (qdisc_run_begin(q)) {
--
1.8.3.1
next reply other threads:[~2019-11-10 2:02 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-10 2:01 Wen Yang [this message]
2019-11-10 13:53 ` [PATCH] net: core: fix unbalanced qdisc_run_begin/qdisc_run_end Wen Yang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191110020149.65307-1-wenyang@linux.alibaba.com \
--to=wenyang@linux.alibaba.com \
--cc=bpf@vger.kernel.org \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=jhs@mojatatu.com \
--cc=john.fastabend@gmail.com \
--cc=kda@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=xiaotian@google.com \
--cc=xiyou.wangcong@gmail.com \
--cc=xlpang@linux.alibaba.com \
--cc=zhiche.yy@alibaba-inc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).