linux-kernel - Re: [PATCH net-next v5 05/22] ethtool: introduce ethtool netlink interface

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: <20190328093746.GA26076@unicorn.suse.cz>
Date:   Thu, 28 Mar 2019 10:37:46 +0100
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Andrew Lunn <andrew@...n.ch>,
        John Linville <linville@...driver.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v5 05/22] ethtool: introduce ethtool netlink
 interface

On Thu, Mar 28, 2019 at 09:10:10AM +0100, Jiri Pirko wrote:
> Thu, Mar 28, 2019 at 03:05:14AM CET, f.fainelli@...il.com wrote:
> >
> >
> >On 3/27/2019 2:50 AM, Jiri Pirko wrote:
> >> 
> >> Why don't you have ETHTOOL_MSG_SET_FOO for set? I think that for
> >> kerne->userspace the ETHTOOL_MSG_FOO if fine. I would change the
> >> ordering of words thought, but it is cosmetics:
> >> ETHTOOL_MSG_FOO /* kernel->userspace messages - replies, notifications */
> >> ETHTOOL_MSG_FOO_GET
> >> ETHTOOL_MSG_FOO_SET
> >> ETHTOOL_MSG_FOO_ACT
> >> 
> >> What do you think?
> >
> >We could even name the notification explicitly with: ETHTOOL_MSG_NOTIF
> >or ETHTOOL_MSG_NTF just so we spell out exactly what those messages are.
> 
> Sound good. Something like:
> 
> ETHTOOL_MSG_FOO_GET
> ETHTOOL_MSG_FOO_GET_RPLY /* kernel->userspace replies to get */
> ETHTOOL_MSG_FOO_SET
> ETHTOOL_MSG_FOO_ACT
> ETHTOOL_MSG_FOO_NTF /* kernel->userspace async messages - notifications */

The names sound fine to me and having different message ids would still
allow processing messages by the same handler easily.

But there is one potential issue I would like to point out: this way we
spend 4 message ids for a get/set pair rather than 2. These message ids
(genlmsghdr::cmd) are u8, i.e. the resource is not as infinite as one
would wish. There are 80 ioctl commands (43 "get" and 29 "set") at the
moment.

Netlink API should be less greedy in general. I already combined some
ioctl commands into one netlink request type and with an easy way to add
new attributes to existing commands, we won't need to add new commands
as often (certainly not in a way which left us with 9 "get" and 9 "set"
ioctl commands for netdev features). So even with 4 ids per get/set
pair, we might be safe for reasonably long time. But it's still
something to keep in mind.

Michal