netdev - ethtool discrepancy between observed behaviour and comments

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-next>] [day] [month] [year] [list]

Message-ID: <YMhJDzNrRNNeObly@linux-dev>
Date:   Tue, 15 Jun 2021 07:30:39 +0100
From:   Kev Jackson <foamdino@...il.com>
To:     mkubecek@...e.cz, netdev@...r.kernel.org
Subject: ethtool discrepancy between observed behaviour and comments

Hi,

I have been working with ethtool, ioctl and Mellanox multi-queue NICs and I think
I have discovered an issue with either the code or the comments that describe
the code or recent changes.

My focus here is simply the ethtool -L command to set the channels for a
multi-queue nic.  Running this command with the following params on a host with
a Mellanox NIC works fine:

ethtool -L eth0 combined 4

The code however seems to check that one of rx or tx must be set as part of the
command (channels.c):

        /* ensure there is at least one RX and one TX channel */
        if (!channels.combined_count && !channels.rx_count)
                err_attr = ETHTOOL_A_CHANNELS_RX_COUNT;
        else if (!channels.combined_count && !channels.tx_count)
                err_attr = ETHTOOL_A_CHANNELS_TX_COUNT;
        else
                err_attr = 0;

and (ioctl.c):

        /* ensure there is at least one RX and one TX channel */
        if (!channels.combined_count &&
            (!channels.rx_count || !channels.tx_count))
                return -EINVAL;

This check was added in commit 7be9251, with the comment:
"Having a channel config with no ability to RX or TX traffic is
clearly wrong. Check for this in the core so the drivers don't
have to."

However this comment and check is contradicted by a (much) older comment from
commit 8b5933c, "Most multiqueue drivers pair up RX and TX
queues so that most channels combine RX and TX work"

After working with ioctl and using a ETHTOOL_SCHANNELS command to try and set
the number of channels (ETHTOOL_GCHANNELS works perfectly fine), I noticed I was
always getting -EINVAL from ethtool_set_channels which led me to uncover these
differences between the behaviour of the ethtool binary and using the
ETHTOOL_SCHANNELS command via code.

After seeing this change I discovered the code in the latest ethtool is using
netlink.c commands and nl_schannels is the command used to set the channels (if
the kernel supports netlink).

nl_schannels was added in this commit: dd3ab09, which doesn't seem to have any
checks for setting rx_count/tx_count/combined_count (although I could have
missed them).

>From putting all of this together, I have come to the conclusion that:
* ioctl / ETHTOOL_SCHANNELS is a legacy method of setting channels
* nl_schannels is the new / preferred method of setting channels
* ethtool has fallback code to run ioctl functions for commands which don't yet
* have a netlink equivalent

Our user experience is that ethtool -L currently does support (and should continue to
support) just setting combined_count rather than having to set combined_count +
one of rx_count/tx_count, which would mean removing the check in the ioctl.c,
ethtool_set_channels code to make the netlink.c and ioctl.c commands consistent.

Obviously the other approach is to add the check for setting one of rx_count /
tx_count into the nl_schannels function.

We're happy to provide a patch for either approach, but would like to raise this
as potentially a bug in the current code.

Thanks,
Kev