Messages in this thread Patch in this message | | | Date | Mon, 26 Sep 2022 16:29:34 -0700 | From | Jakub Kicinski <> | 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.
| |