netdev - Re: [ANN] netdev call

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: <Ze9bFepLhiQIBmio@lzaremba-mobl.ger.corp.intel.com>
Date: Mon, 11 Mar 2024 20:27:17 +0100
From: Larysa Zaremba <larysa.zaremba@...el.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: <netdev@...r.kernel.org>, <netdev-driver-reviewers@...r.kernel.org>,
	Maciej Fijalkowski <maciej.fijalkowski@...el.com>
Subject: Re: [ANN] netdev call - Mar 5th

On Mon, Mar 04, 2024 at 10:36:57AM -0800, Jakub Kicinski wrote:
> On Mon, 4 Mar 2024 10:35:38 -0800 Jakub Kicinski wrote:
> > Hi,
> > 
> > The bi-weekly netdev call at https://bbb.lwn.net/b/jak-wkr-seg-hjn
> > is scheduled tomorrow at 8:30 am (PT) / 5:30 pm (~EU).
> > 
> > No topics so far, please reach out if you have something to discuss,
> > otherwise we'll cancel.
> > 
> > The reviewer of the week is.. Meta!
> 
> Sorry, ignore that, ENOCOFFEE.
> The next call is scheduled for the 12th of March, not tomorrow 🤦️
>

We have a topic for the 12.03.2024 netdev call:

How set_channels() input should be interpreted

Mostly, implemetations of .set_channels() are tailored for only one of the
following inputs:
1. ethtool -L <ifname> combined <n>
2. ethtool -L <ifname> rx <n> tx <m>

There are also exceptions, e.g.
* bnxt: input 1 puts interface in a shared channels mode, input 2 puts device
  into a dedicated channels mode
* xgbe: ethtool -L <ifname> combined <n> rx <m> tx <l>, <m> and <l> cannot both
  be non-zero at the same time
* ice and idpf: accept both inputs, but try to create as many queue pairs as
  possible

In idpf and ice, we have encountered a problem, where the checks (listed below)
in ethnl_set_channels() do not trigger for us, because our set_channels() operates
under an assumption that if a value in ethtool_channels stays the same, it was
not provided by user and therefore defaults to '0' [0].

The ethnl_set_channels() checks and how they fail us:
1. Ensure there is at least 1 RX and 1 TX channel [1]
        ethtool -l <ifname> -> combined 40
        ethtool -L <ifname> combined 40
   ice triggers an internal check, which was removed from idpf after review [2],
   so idpf tries to set 0 RX and
                                           0 TX queues
2. Prevent disabling a channel with an AF_XDP socket attached [3]
        ethtool -l <ifname> -> combined 40
        ./xdpsock -i <ifname> -q35
        ethtool -L <ifname> rx 30 tx 30
  ice destroys the queue the socket is attached to, despite there being a check
  in the core code that must prevent this [3]
  In theory, the same should be possible to do the same on e.g. mlx5, by doing
        ethtool -L <ifname> combined 30 rx 10 tx 10

So, we would like to discuss, how can we solve the above problem and also
establish how should an ideal set_channels() input interpretation look.

Possible solutions:
1. Convert our drivers to whatever is a preferred way to handle the input.
2. Add .fix_channels() callback to ethtool_ops
        [...]
        mod |= mod_combined;
        if (!mod)
                return 0;

        /* Here ice and idpf would change unmodified values to 0 and convert
         * separate overlapping rx/tx channels into combined,
         * mlx5 and bnxt would set rx_count and tx_count to 0, etc.
         */
        if (dev->ethtool_ops->fix_num_channels)
                dev->ethtool_ops->fix_num_channels(dev, &channels);

        /* ethtool does generic checks on the modified values and calls
         * set_channels() after
         */
        [...]

3. Use the abovementioned assumption made by ice driver [0] in ethnl_set_channels().
   We propose this, because no doubt about it was expressed during the community
   review of the both affected drivers [2][4].

[0] https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/net/ethernet/intel/ice/ice_ethtool.c#L3541
[1] https://elixir.bootlin.com/linux/v6.8-rc7/source/net/ethtool/channels.c#L154
[2] https://lore.kernel.org/netdev/20230821140205.4d3bc797@kernel.org/
[3] https://elixir.bootlin.com/linux/v6.8-rc7/source/net/ethtool/channels.c#L186
[4] https://lore.kernel.org/netdev/20191121144246.04adde1a@cakuba.netronome.com/