From patchwork Fri Mar 10 19:25:38 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Philip Cox X-Patchwork-Id: 1755538 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=lists.ubuntu.com (client-ip=91.189.94.19; helo=huckleberry.canonical.com; envelope-from=kernel-team-bounces@lists.ubuntu.com; receiver=) Authentication-Results: legolas.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=canonical.com header.i=@canonical.com header.a=rsa-sha256 header.s=20210705 header.b=K5McV2pK; dkim-atps=neutral Received: from huckleberry.canonical.com (huckleberry.canonical.com [91.189.94.19]) (using TLSv1.2 with cipher ECDHE-ECDSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4PYGLS0b8bz246Z for ; Sat, 11 Mar 2023 06:28:31 +1100 (AEDT) Received: from localhost ([127.0.0.1] helo=huckleberry.canonical.com) by huckleberry.canonical.com with esmtp (Exim 4.86_2) (envelope-from ) id 1paiPb-0003nu-2T; Fri, 10 Mar 2023 19:28:19 +0000 Received: from smtp-relay-internal-0.internal ([10.131.114.225] helo=smtp-relay-internal-0.canonical.com) by huckleberry.canonical.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1paiPY-0003nm-PC for kernel-team@lists.ubuntu.com; Fri, 10 Mar 2023 19:28:16 +0000 Received: from mail-qt1-f200.google.com (mail-qt1-f200.google.com [209.85.160.200]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by smtp-relay-internal-0.canonical.com (Postfix) with ESMTPS id 4AB0440799 for ; Fri, 10 Mar 2023 19:28:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=canonical.com; s=20210705; t=1678476496; bh=VkFu3wgBF7dVjK4ulWYletI/kHY51NXbpX0QjUe9zoI=; h=From:To:Subject:Date:Message-Id:MIME-Version; b=K5McV2pKVNo0rp1Ovi0BC7tuOAEoL9pErfg7wVO8xydrDm8B09d92vhzAY9V0ZngZ XbslFmAmuwbIps8v0/wWMsi7djz+NI2opvdVqcOG1aR2tbnphKglw+fVm1kUxV+x3C 2ENve1Zpemm262Amgm2Yav6K/BP/GIWSh/ZF+TXZPBRbHnxTjDSdIyx4B7CQ9lzXQL fTH1uoGKpJw64kQiBfPN/Ld411GS/i1WHJh2HEEzCNgFrr8i9+7NTns6l9c8ApPFuy U1/pXYpflYYBlnPbUF3ZbIYXRfDGatBZSWtaYHa43YFk5v4LhDXx3drDMK60xD0qa3 4SH5vCI+IBduQ== Received: by mail-qt1-f200.google.com with SMTP id c11-20020ac85a8b000000b003bfdd43ac76so3444499qtc.5 for ; Fri, 10 Mar 2023 11:28:16 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678476495; h=content-transfer-encoding:mime-version:message-id:date:subject:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=VkFu3wgBF7dVjK4ulWYletI/kHY51NXbpX0QjUe9zoI=; b=0y3H4z8WGESmc0orGovRfZ4D2rCkhfUnGEnaKTYMhLCegCtYDonCsXj1SijY4uwi0t frly2TORqu/7lUjmqBk7W2Q9MFuqGkI5Vlx8Q9parZDzn4jpXTyaTNTkddSGhhTUQaur w/QWu3j4jaHvXxYXwgLIoXL8LweyKPVWsomI/IuM8f6YJ7f+CcDeYhRqGHjkYcmyGRh4 FRYN39vcxx28g/FZVfHC81KZITZYls/tni9fK5chlH1ieyxbVShOb1URl61+2NG67vdH IqDe/81moontYrsglSHRXrwJgzbXLE7EfR2PrVbxrXPArkJ5h0z8z8YMndoRluEAXgWc y6GQ== X-Gm-Message-State: AO0yUKWuZcuzHe2hb6PnHSyDjGhRG+LcdtgqUrTGL0NJ3PsMfFNjVoLH KTQ2scJrWD0f74vZyWbuEqhP1OACXZDPn4sLEP2SdCM7Jt4XojA50YOPBo0cPY9r6XRoqPmha0Z N3xWNrt6WgT6ex0p316H7tcI5ho3kNlA8jaRqsXuBAjixkUa8Fw== X-Received: by 2002:ac8:5988:0:b0:3bf:c371:789e with SMTP id e8-20020ac85988000000b003bfc371789emr45163493qte.14.1678476494940; Fri, 10 Mar 2023 11:28:14 -0800 (PST) X-Google-Smtp-Source: AK7set+5Htz1f/2zMbFhyMz4LXzOSES3cnQXZjBXSRFKtrxvwFWqiukKE9s11o1ZpQzUawUOJInqGg== X-Received: by 2002:ac8:5988:0:b0:3bf:c371:789e with SMTP id e8-20020ac85988000000b003bfc371789emr45163456qte.14.1678476494385; Fri, 10 Mar 2023 11:28:14 -0800 (PST) Received: from localhost.localdomain (65-110-221-142.cpe.pppoe.ca. [65.110.221.142]) by smtp.gmail.com with ESMTPSA id v125-20020a379383000000b0074341cb30d0sm342966qkd.62.2023.03.10.11.28.13 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 10 Mar 2023 11:28:14 -0800 (PST) From: Philip Cox To: kernel-team@lists.ubuntu.com Subject: [jammy:linux-intel-iotg][PATCH 1/1] Revert "net/sched: taprio: make qdisc_leaf() see the per-netdev-queue pfifo child qdiscs" Date: Fri, 10 Mar 2023 14:25:38 -0500 Message-Id: <20230310192538.617109-1-philip.cox@canonical.com> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.20 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: "kernel-team" From: Vladimir Oltean BugLink: https://bugs.launchpad.net/bugs/1997525 taprio_attach() has this logic at the end, which should have been removed with the blamed patch (which is now being reverted): /* access to the child qdiscs is not needed in offload mode */ if (FULL_OFFLOAD_IS_ENABLED(q->flags)) { kfree(q->qdiscs); q->qdiscs = NULL; } because otherwise, we make use of q->qdiscs[] even after this array was deallocated, namely in taprio_leaf(). Therefore, whenever one would try to attach a valid child qdisc to a fully offloaded taprio root, one would immediately dereference a NULL pointer. $ tc qdisc replace dev eno0 handle 8001: parent root taprio \ num_tc 8 \ map 0 1 2 3 4 5 6 7 \ queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7 \ max-sdu 0 0 0 0 0 200 0 0 \ base-time 200 \ sched-entry S 80 20000 \ sched-entry S a0 20000 \ sched-entry S 5f 60000 \ flags 2 $ max_frame_size=1500 $ data_rate_kbps=20000 $ port_transmit_rate_kbps=1000000 $ idleslope=$data_rate_kbps $ sendslope=$(($idleslope - $port_transmit_rate_kbps)) $ locredit=$(($max_frame_size * $sendslope / $port_transmit_rate_kbps)) $ hicredit=$(($max_frame_size * $idleslope / $port_transmit_rate_kbps)) $ tc qdisc replace dev eno0 parent 8001:7 cbs \ idleslope $idleslope \ sendslope $sendslope \ hicredit $hicredit \ locredit $locredit \ offload 0 Unable to handle kernel NULL pointer dereference at virtual address 0000000000000030 pc : taprio_leaf+0x28/0x40 lr : qdisc_leaf+0x3c/0x60 Call trace: taprio_leaf+0x28/0x40 tc_modify_qdisc+0xf0/0x72c rtnetlink_rcv_msg+0x12c/0x390 netlink_rcv_skb+0x5c/0x130 rtnetlink_rcv+0x1c/0x2c The solution is not as obvious as the problem. The code which deallocates q->qdiscs[] is in fact copied and pasted from mqprio, which also deallocates the array in mqprio_attach() and never uses it afterwards. Therefore, the identical cleanup logic of priv->qdiscs[] that mqprio_destroy() has is deceptive because it will never take place at qdisc_destroy() time, but just at raw ops->destroy() time (otherwise said, priv->qdiscs[] do not last for the entire lifetime of the mqprio root), but rather, this is just the twisted way in which the Qdisc API understands error path cleanup should be done (Qdisc_ops :: destroy() is called even when Qdisc_ops :: init() never succeeded). Side note, in fact this is also what the comment in mqprio_init() says: /* pre-allocate qdisc, attachment can't fail */ Or reworded, mqprio's priv->qdiscs[] scheme is only meant to serve as data passing between Qdisc_ops :: init() and Qdisc_ops :: attach(). [ this comment was also copied and pasted into the initial taprio commit, even though taprio_attach() came way later ] The problem is that taprio also makes extensive use of the q->qdiscs[] array in the software fast path (taprio_enqueue() and taprio_dequeue()), but it does not keep a reference of its own on q->qdiscs[i] (you'd think that since it creates these Qdiscs, it holds the reference, but nope, this is not completely true). To understand the difference between taprio_destroy() and mqprio_destroy() one must look before commit 13511704f8d7 ("net: taprio offload: enforce qdisc to netdev queue mapping"), because that just muddied the waters. In the "original" taprio design, taprio always attached itself (the root Qdisc) to all netdev TX queues, so that dev_qdisc_enqueue() would go through taprio_enqueue(). It also called qdisc_refcount_inc() on itself for as many times as there were netdev TX queues, in order to counter-balance what tc_get_qdisc() does when destroying a Qdisc (simplified for brevity below): if (n->nlmsg_type == RTM_DELQDISC) err = qdisc_graft(dev, parent=NULL, new=NULL, q, extack); qdisc_graft(where "new" is NULL so this deletes the Qdisc): for (i = 0; i < num_q; i++) { struct netdev_queue *dev_queue; dev_queue = netdev_get_tx_queue(dev, i); old = dev_graft_qdisc(dev_queue, new); if (new && i > 0) qdisc_refcount_inc(new); qdisc_put(old); ~~~~~~~~~~~~~~ this decrements taprio's refcount once for each TX queue } notify_and_destroy(net, skb, n, classid, rtnl_dereference(dev->qdisc), new); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ and this finally decrements it to zero, making qdisc_put() call qdisc_destroy() The q->qdiscs[] created using qdisc_create_dflt() (or their replacements, if taprio_graft() was ever to get called) were then privately freed by taprio_destroy(). This is still what is happening after commit 13511704f8d7 ("net: taprio offload: enforce qdisc to netdev queue mapping"), but only for software mode. In full offload mode, the per-txq "qdisc_put(old)" calls from qdisc_graft() now deallocate the child Qdiscs rather than decrement taprio's refcount. So when notify_and_destroy(taprio) finally calls taprio_destroy(), the difference is that the child Qdiscs were already deallocated. And this is exactly why the taprio_attach() comment "access to the child qdiscs is not needed in offload mode" is deceptive too. Not only the q->qdiscs[] array is not needed, but it is also necessary to get rid of it as soon as possible, because otherwise, we will also call qdisc_put() on the child Qdiscs in qdisc_destroy() -> taprio_destroy(), and this will cause a nasty use-after-free/refcount-saturate/whatever. In short, the problem is that since the blamed commit, taprio_leaf() needs q->qdiscs[] to not be freed by taprio_attach(), while qdisc_destroy() -> taprio_destroy() does need q->qdiscs[] to be freed by taprio_attach() for full offload. Fixing one problem triggers the other. All of this can be solved by making taprio keep its q->qdiscs[i] with a refcount elevated at 2 (in offloaded mode where they are attached to the netdev TX queues), both in taprio_attach() and in taprio_graft(). The generic qdisc_graft() would just decrement the child qdiscs' refcounts to 1, and taprio_destroy() would give them the final coup de grace. However the rabbit hole of changes is getting quite deep, and the complexity increases. The blamed commit was supposed to be a bug fix in the first place, and the bug it addressed is not so significant so as to justify further rework in stable trees. So I'd rather just revert it. I don't know enough about multi-queue Qdisc design to make a proper judgement right now regarding what is/isn't idiomatic use of Qdisc concepts in taprio. I will try to study the problem more and come with a different solution in net-next. Fixes: 1461d212ab27 ("net/sched: taprio: make qdisc_leaf() see the per-netdev-queue pfifo child qdiscs") Reported-by: Muhammad Husaini Zulkifli Reported-by: Vinicius Costa Gomes Signed-off-by: Vladimir Oltean Reviewed-by: Vinicius Costa Gomes Link: https://lore.kernel.org/r/20221004220100.1650558-1-vladimir.oltean@nxp.com Signed-off-by: Jakub Kicinski (cherry picked from commit af7b29b1deaac6da3bb7637f0e263dfab7bfc7a3) Signed-off-by: Philip Cox --- net/sched/sch_taprio.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c index 708f3131e15c..2c0f0e7676ae 100644 --- a/net/sched/sch_taprio.c +++ b/net/sched/sch_taprio.c @@ -1971,14 +1971,12 @@ static int taprio_dump(struct Qdisc *sch, struct sk_buff *skb) static struct Qdisc *taprio_leaf(struct Qdisc *sch, unsigned long cl) { - struct taprio_sched *q = qdisc_priv(sch); - struct net_device *dev = qdisc_dev(sch); - unsigned int ntx = cl - 1; + struct netdev_queue *dev_queue = taprio_queue_get(sch, cl); - if (ntx >= dev->num_tx_queues) + if (!dev_queue) return NULL; - return q->qdiscs[ntx]; + return dev_queue->qdisc_sleeping; } static unsigned long taprio_find(struct Qdisc *sch, u32 classid)