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
| ||
|
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.
Powered by blists - more mailing lists