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]
Date:   Mon, 26 Sep 2022 17:43:33 -0700
From:   Jakub Kicinski <>
To:     Vladimir Oltean <>
Cc:     Vladimir Oltean <>,
        "" <>,
        "David S. Miller" <>,
        Eric Dumazet <>,
        Paolo Abeni <>,
        Xiaoliang Yang <>,
        Rui Sousa <>,
        Claudiu Manoil <>,
        Alexandre Belloni <>,
        "" <>,
        Horatiu Vultur <>,
        Andrew Lunn <>,
        Vivien Didelot <>,
        Florian Fainelli <>,
        Michael Walle <>,
        Vinicius Costa Gomes <>,
        Maxim Kochetkov <>,
        Colin Foster <>,
        Richie Pearn <>,
        Kurt Kanzenbach <>,
        Jesse Brandeburg <>,
        Tony Nguyen <>,
        Giuseppe Cavallaro <>,
        Alexandre Torgue <>,
        Jose Abreu <>,
        Jamal Hadi Salim <>,
        Cong Wang <>,
        Jiri Pirko <>,
        Gerhard Engleder <>
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.

Powered by blists - more mailing lists