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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220926174333.15dbca47@kernel.org>
Date:   Mon, 26 Sep 2022 17:43:33 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Vladimir Oltean <vladimir.oltean@....com>
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>
Subject: Re: [PATCH v2 net-next 02/12] tsnep: deny tc-taprio changes to
 per-tc max SDU

On Tue, 27 Sep 2022 00:22:53 +0000 Vladimir Oltean wrote:
> 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?

struct devlink_ops {
	/**
	 * @supported_flash_update_params:
	 * mask of parameters supported by the driver's .flash_update
	 * implemementation.
	 */
	u32 supported_flash_update_params;
	unsigned long reload_actions;
	unsigned long reload_limits;

struct ethtool_ops {
	u32     cap_link_lanes_supported:1;
	u32	supported_coalesce_params;
	u32	supported_ring_params;

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

Right, the lack of a structure we can put it in is quite unfortunate :(
But I do not dare suggesting we add a structure with qdisc and cls
specific callbacks instead of the mux-y ndo_setup_tc :)
I guess we could take a shortcut and put a pointer in netdev_ops for
just the caps for now, hm.

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

The queues stopped part is not true already for classifier offloads :(

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

If we add a new pointer for netdev_ops I'd go with a struct pointer
rather than an op, for consistency if nothing else. But if you feel
strongly either way will work.