netdev - 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: <20220927002252.mwrxp3wicew3vz6p@skbuf>
Date:   Tue, 27 Sep 2022 00:22:53 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     Vladimir Oltean <olteanv@...il.com>,
        "netdev@...r.kernel.org" <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" <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" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 net-next 02/12] tsnep: deny tc-taprio changes to per-tc
 max SDU

On Mon, Sep 26, 2022 at 04:29:34PM -0700, Jakub Kicinski wrote:
> I usually put a capability field into the ops themselves.

Do you also have an example for the 'usual' manner?

> 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;

actually s/u32/enum tc_setup_type/

inception....

>  };

> 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.

You also need to see the flip side. You're making code more self-maintainable
by adding bureaucracy to the run time itself. Whereas things could have
been sorted out between the qdisc and the driver in just one ndo_setup_tc()
call via the straightforward approach (every driver rejects what it
doesn't like), now you need two calls for the normal case when the
driver will accept a valid configuration.

I get the point and I think this won't probably make a big difference
for a slow path like qdisc offload (at least it won't for me), but from
an API perspective, once the mechanism will go in, it will become quite
ossified, so it's best to ask some questions about it now.

Like for example you're funneling the caps through ndo_setup_tc(), which
has these comments in its description:

 * int (*ndo_setup_tc)(struct net_device *dev, enum tc_setup_type type,
 *		       void *type_data);
 *	Called to setup any 'tc' scheduler, classifier or action on @dev.
 *	This is always called from the stack with the rtnl lock held and netif
 *	tx queues stopped. This allows the netdevice to perform queue
 *	management safely.

Do we need to offer guarantees of rtnl lock and stopped TX queues to a
function which just queries capabilities (and likely doesn't need them),
or would it be better to devise a new ndo? Generally, when you have a
separate method to query caps vs to actually do the work, different
calling contexts is generally the justification to do that, as opposed
to piggy-backing the caps that the driver acted upon through the same
struct tc_taprio_qopt_offload.