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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ