linux-kernel - Re: [PATCH v2 net-next 02/12] tsnep: deny tc-taprio changes to per-tc max SDU

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]

Message-ID: <20220926162934.58bf38a6@kernel.org>
Date:   Mon, 26 Sep 2022 16:29:34 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Vladimir Oltean <vladimir.oltean@....com>, netdev@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Xiaoliang Yang <xiaoliang.yang_1@....com>,
        Rui Sousa <rui.sousa@....com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        UNGLinuxDriver@...rochip.com,
        Horatiu Vultur <horatiu.vultur@...rochip.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Michael Walle <michael@...le.cc>,
        Vinicius Costa Gomes <vinicius.gomes@...el.com>,
        Maxim Kochetkov <fido_max@...ox.ru>,
        Colin Foster <colin.foster@...advantage.com>,
        Richie Pearn <richard.pearn@....com>,
        Kurt Kanzenbach <kurt@...utronix.de>,
        Jesse Brandeburg <jesse.brandeburg@...el.com>,
        Tony Nguyen <anthony.l.nguyen@...el.com>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        Jose Abreu <joabreu@...opsys.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Gerhard Engleder <gerhard@...leder-embedded.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 net-next 02/12] tsnep: deny tc-taprio changes to
 per-tc max SDU

On Tue, 27 Sep 2022 00:50:49 +0300 Vladimir Oltean wrote:
> > Don't all the driver patches make you wanna turn this into an opt-in?  
> 
> Presumably you're thinking of a way through which the caller of
> ndo_setup_tc(TC_SETUP_QDISC_TAPRIO, struct tc_taprio_qopt_offload *)
> knows whether the driver took the new max_sdu field into consideration,
> and not just accepted it blindly?
> 
> I'm not exactly up to date with all the techniques which can achieve
> that without changes in drivers, and I haven't noticed other qdisc
> offloads doing it either... but this would be a great trick to learn for
> sure. Do you have any idea?

I usually put a capability field into the ops themselves. But since tc
offloads don't have real ops (heh) we need to do the command callback
thing. This is my knee-jerk coding of something:

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 9f42fc871c3b..2d043def76d8 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -960,6 +960,11 @@ enum tc_setup_type {
 	TC_SETUP_QDISC_FIFO,
 	TC_SETUP_QDISC_HTB,
 	TC_SETUP_ACT,
+	TC_QUERY_CAPS,
+};
+
+struct tc_query_caps {
+	u32 cmd;
 };
 
 /* These structures hold the attributes of bpf state that are being passed
diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
index 2ff80cd04c5c..2416151a23db 100644
--- a/include/net/pkt_sched.h
+++ b/include/net/pkt_sched.h
@@ -155,6 +155,12 @@ struct tc_etf_qopt_offload {
 	s32 queue;
 };
 
+struct tc_taprio_drv_caps {
+	struct tc_query_caps base;
+
+	bool accept_max_sdu;
+};
+
 struct tc_taprio_sched_entry {
 	u8 command; /* TC_TAPRIO_CMD_* */
 
diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 136ae21ebce9..68302ee33937 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -1219,6 +1219,7 @@ static int taprio_enable_offload(struct net_device *dev,
 				 struct sched_gate_list *sched,
 				 struct netlink_ext_ack *extack)
 {
+	struct tc_taprio_drv_caps caps = { { .cmd = TC_SETUP_QDISC_TAPRIO, }, };
 	const struct net_device_ops *ops = dev->netdev_ops;
 	struct tc_taprio_qopt_offload *offload;
 	int err = 0;
@@ -1229,6 +1230,12 @@ static int taprio_enable_offload(struct net_device *dev,
 		return -EOPNOTSUPP;
 	}
 
+	ops->ndo_setup_tc(dev, TC_QUERY_CAPS, &caps);
+	if (!caps.accept_max_sdu && taprio_is_max_sdu_used(...))  {
+		NL_SET_ERR_MSG(extack, "nope.");
+		return -EOPNOTSUPP;
+	}
+
 	offload = taprio_offload_alloc(sched->num_entries);
 	if (!offload) {
 		NL_SET_ERR_MSG(extack,

> > What are the chances we'll catch all drivers missing the validation 
> > in review?  
> 
> Not that slim I think, they are all identifiable if you search for
> TC_SETUP_QDISC_TAPRIO.

Right, but that's what's in the tree _now_. Experience teaches that
people may have out of tree code which implements TAPRIO and may send
it for upstream review without as much as testing it against net-next :(
As time passes and our memories fade the chances we'd catch such code
when posted upstream go down, perhaps from high to medium but still,
the explicit opt-in is more foolproof.