Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Vladimir Oltean <vladimir.oltean@nxp.com>
To:     netdev@vger.kernel.org
Cc:     Vinicius Costa Gomes <vinicius.gomes@intel.com>,
        Jamal Hadi Salim <jhs@mojatatu.com>,
        Cong Wang <xiyou.wangcong@gmail.com>,
        Jiri Pirko <jiri@resnulli.us>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Voon Weifeng <weifeng.voon@intel.com>,
        Vladimir Oltean <olteanv@gmail.com>,
        Kurt Kanzenbach <kurt@linutronix.de>,
        linux-kernel@vger.kernel.org
Subject: [PATCH net 1/3] net/sched: taprio: avoid disabling offload when it was never enabled
Date:   Wed, 14 Sep 2022 17:34:37 +0300
Message-Id: <20220914143439.1544363-2-vladimir.oltean@nxp.com>
In-Reply-To: <20220914143439.1544363-1-vladimir.oltean@nxp.com>
References: <20220914143439.1544363-1-vladimir.oltean@nxp.com>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?1BnE4FkYyGZ6RAIimw9c60DBX7CgnvOp0rKSP6LZjKW2YW/qzpx97h1KZ6Y/?=
 =?us-ascii?Q?TDPvatFAd3HlRnShtu3HV/ELs+Cnmor2s972PKZ9PSc8geabzPaEbTop5lkW?=
 =?us-ascii?Q?COOg0nQTtTGOq3Z/Ns55o5D6C8Tzg9DF0auFlxL/6M7sBqYnId94HzTyybwH?=
 =?us-ascii?Q?vm3wVWrid9+A506H/LjH5OBtEjSlBkfMNl+XUAG+mJNLGECfYh0aoCNhycXT?=
 =?us-ascii?Q?uq6m41I6gb+6tHnfwyP3Gvi/RJswcVACaQulx8moWpJp1tQKN3YXRMqsQlXw?=
 =?us-ascii?Q?lqfD293BfhLOD/Z+NVfLAcmkkunfL8tSacw7c84caYPlefvzC/evudSjcVNB?=
 =?us-ascii?Q?iZBl+aUzkBZzZ5ECXzZRB6h5hygY6cJOjohIFq2gO14nmg9aVa5PsZWCQRLV?=
 =?us-ascii?Q?2b7H+h4UIVNbg3aJ1bBnBMqtCIqsyJ3Ocv0Q8NgBtQzj+/trJ9uJQehV5K2N?=
 =?us-ascii?Q?rXjdXrnEBBoeh2dJzyE/I9r9mDP2kW+akwoXBbrM8U2BAZR7ts9aG7cjtS0U?=
 =?us-ascii?Q?jdK6v9QV539lpj7lOdU7YH1DGKNoXczTyToeQylcQQoD/oFWvZf7my4p/U8M?=
 =?us-ascii?Q?kVNHa9WH0mmcif1N6mbt3taETTOjDyhP9tXnYzPuuafEqPYSop3r5NzvNEpv?=
 =?us-ascii?Q?fEBQMNWJk0jUx99Wln+CSMayqDLgwtTnPY30PCYpXuz0aWckckq+t2zr8uho?=
 =?us-ascii?Q?JTSqMAtP2SWSfOnQZB8ZrMxcQqyzdizhcCGmilwJb8EWkzu03aGAf/vhC5Dq?=
 =?us-ascii?Q?iHO5aQJTLqEwX8fHStdflAfSMwFGvH3Q9g1jqcDNMP9FD5f3l8TI4iauYCBl?=
 =?us-ascii?Q?K/SCjnVV78bgcRQ3sYPh5u3zFauHKHELgru00TCFu0ahEDWNELKzJyYzbuA3?=
 =?us-ascii?Q?d6T8n6mjrynAlvTgHBfnxzAnd2Oak216HyUqhweAOU903ZA89p/tcLBNBFP1?=
 =?us-ascii?Q?KQo9TGXRdybdKy2z3PtpqiH3XZySocYjWWYWrEeNZ7GK2tPaMzYn8HQ7aela?=
 =?us-ascii?Q?0OAqyCtbeBdJz/9UR8h6TkwFgZgRon4zoBBzRdDX73riD3JYzozAzFey8KK9?=
 =?us-ascii?Q?eXst8gf5JMjlSeVJP+y4deg6qHhvq27OW0neS1LNKJ7Bm6wDrFm/z+pijonv?=
 =?us-ascii?Q?ccgvC34FtXfDg7XJ3wWNXkdjpmG0yIIN3xvQn0fOcF2T6qsPDAVX12LadlhM?=
 =?us-ascii?Q?YrHKdispkqmNlRG43VDjIEe8LssSBcw6VqZNn6UtHnixjb+b6BV9tdjd6wC/?=
 =?us-ascii?Q?C+BsMoiPPGK2XYKmd+R1C/04vZOyROZL3S77w9QkbX7l17asI/qIuX4JGWLD?=
 =?us-ascii?Q?PrKsnPL1nvYG2A0Yxb1dodENfEPZfPo8NPjT1dD3Sn74qqC4TtPFRfYiJgYL?=
 =?us-ascii?Q?f5Q5EIim+rOgZwdMJ1QWJ7g/e7GB+TPnEoYHbQinoHJhCd+usNghMLNFzeQ0?=
 =?us-ascii?Q?isXJilRdJEwSrYNS+bRs0Mi2YdCYKzQM/KjhSu68B1cP5gVY7S0aNZZ/HScj?=
 =?us-ascii?Q?aJN7kOu15gbGUF2onxdiupi9WtbD3OOHCjRWAdVxr9Ulu8Laiyurg+kacOYH?=
 =?us-ascii?Q?K9JN2zversnD14wOsJY9RwRMsmv8y/K1XxagHTT5yDWM4GVsPvuooGVCiJz/?=
 =?us-ascii?Q?2Q=3D=3D?=
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 37a62248-6897-4c1f-5124-08da965e4cda
X-MS-Exchange-CrossTenant-AuthSource: VI1PR04MB5136.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Sep 2022 14:34:58.9627
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 5uTvK3i9Iyg86YpjPxlkjzxJCFVvZ9ezFVTl9Qj+11Al/JHK4TMabqgzLwSBZzWv6TxuHdQM4AEeBFglZAVMlg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB8PR04MB6777
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

In an incredibly strange API design decision, qdisc->destroy() gets
called even if qdisc->init() never succeeded, not exclusively since
commit 87b60cfacf9f ("net_sched: fix error recovery at qdisc creation"),
but apparently also earlier (in the case of qdisc_create_dflt()).

The taprio qdisc does not fully acknowledge this when it attempts full
offload, because it starts off with q->flags = TAPRIO_FLAGS_INVALID in
taprio_init(), then it replaces q->flags with TCA_TAPRIO_ATTR_FLAGS
parsed from netlink (in taprio_change(), tail called from taprio_init()).

But in taprio_destroy(), we call taprio_disable_offload(), and this
determines what to do based on FULL_OFFLOAD_IS_ENABLED(q->flags).

But looking at the implementation of FULL_OFFLOAD_IS_ENABLED()
(a bitwise check of bit 1 in q->flags), it is invalid to call this macro
on q->flags when it contains TAPRIO_FLAGS_INVALID, because that is set
to U32_MAX, and therefore FULL_OFFLOAD_IS_ENABLED() will return true on
an invalid set of flags.

As a result, it is possible to crash the kernel if user space forces an
error between setting q->flags = TAPRIO_FLAGS_INVALID, and the calling
of taprio_enable_offload(). This is because drivers do not expect the
offload to be disabled when it was never enabled.

The error that we force here is to attach taprio as a non-root qdisc,
but instead as child of an mqprio root qdisc:

$ tc qdisc add dev swp0 root handle 1: \
	mqprio 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 hw 0
$ tc qdisc replace dev swp0 parent 1:1 \
	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 base-time 0 \
	sched-entry S 0x7f 990000 sched-entry S 0x80 100000 \
	flags 0x0 clockid CLOCK_TAI
Unable to handle kernel paging request at virtual address fffffffffffffff8
[fffffffffffffff8] pgd=0000000000000000, p4d=0000000000000000
Internal error: Oops: 96000004 [#1] PREEMPT SMP
Call trace:
 taprio_dump+0x27c/0x310
 vsc9959_port_setup_tc+0x1f4/0x460
 felix_port_setup_tc+0x24/0x3c
 dsa_slave_setup_tc+0x54/0x27c
 taprio_disable_offload.isra.0+0x58/0xe0
 taprio_destroy+0x80/0x104
 qdisc_create+0x240/0x470
 tc_modify_qdisc+0x1fc/0x6b0
 rtnetlink_rcv_msg+0x12c/0x390
 netlink_rcv_skb+0x5c/0x130
 rtnetlink_rcv+0x1c/0x2c

Fix this by keeping track of the operations we made, and undo the
offload only if we actually did it.

I've added "bool offloaded" inside a 4 byte hole between "int clockid"
and "atomic64_t picos_per_byte". Now the first cache line looks like
below:

$ pahole -C taprio_sched net/sched/sch_taprio.o
struct taprio_sched {
        struct Qdisc * *           qdiscs;               /*     0     8 */
        struct Qdisc *             root;                 /*     8     8 */
        u32                        flags;                /*    16     4 */
        enum tk_offsets            tk_offset;            /*    20     4 */
        int                        clockid;              /*    24     4 */
        bool                       offloaded;            /*    28     1 */

        /* XXX 3 bytes hole, try to pack */

        atomic64_t                 picos_per_byte;       /*    32     0 */

        /* XXX 8 bytes hole, try to pack */

        spinlock_t                 current_entry_lock;   /*    40     0 */

        /* XXX 8 bytes hole, try to pack */

        struct sched_entry *       current_entry;        /*    48     8 */
        struct sched_gate_list *   oper_sched;           /*    56     8 */
        /* --- cacheline 1 boundary (64 bytes) --- */

Fixes: 9c66d1564676 ("taprio: Add support for hardware offloading")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/sched/sch_taprio.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index db88a692ef81..a3b4f92a9937 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -67,6 +67,7 @@ struct taprio_sched {
 	u32 flags;
 	enum tk_offsets tk_offset;
 	int clockid;
+	bool offloaded;
 	atomic64_t picos_per_byte; /* Using picoseconds because for 10Gbps+
 				    * speeds it's sub-nanoseconds per byte
 				    */
@@ -1279,6 +1280,8 @@ static int taprio_enable_offload(struct net_device *dev,
 		goto done;
 	}
 
+	q->offloaded = true;
+
 done:
 	taprio_offload_free(offload);
 
@@ -1293,12 +1296,9 @@ static int taprio_disable_offload(struct net_device *dev,
 	struct tc_taprio_qopt_offload *offload;
 	int err;
 
-	if (!FULL_OFFLOAD_IS_ENABLED(q->flags))
+	if (!q->offloaded)
 		return 0;
 
-	if (!ops->ndo_setup_tc)
-		return -EOPNOTSUPP;
-
 	offload = taprio_offload_alloc(0);
 	if (!offload) {
 		NL_SET_ERR_MSG(extack,
@@ -1314,6 +1314,8 @@ static int taprio_disable_offload(struct net_device *dev,
 		goto out;
 	}
 
+	q->offloaded = false;
+
 out:
 	taprio_offload_free(offload);
 
-- 
2.34.1