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

[Jakub Kicinski <kuba@kernel.org>]

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.