lkml.org 
[lkml]   [2022]   [Sep]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [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.

    \
     
     \ /
      Last update: 2022-09-27 01:30    [W:3.029 / U:0.268 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site