netdev - Re: [RFC PATCH net-next 1/3] net: rtnetlink: Add link-down reason to RTNL messages

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]

Date:   Sun, 17 Mar 2019 01:24:04 +0100
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     Petr Machata <petrm@...lanox.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jiri Pirko <jiri@...lanox.com>,
        Ido Schimmel <idosch@...lanox.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        Tariq Toukan <tariqt@...lanox.com>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "stephen@...workplumber.org" <stephen@...workplumber.org>
Subject: Re: [RFC PATCH net-next 1/3] net: rtnetlink: Add link-down reason to
 RTNL messages

On Fri, Mar 15, 2019 at 07:26:11PM -0700, Jakub Kicinski wrote:
> On Fri, 15 Mar 2019 17:56:07 +0000, Petr Machata wrote:
> > diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
> > index e2091bb2b3a8..cfd9e86ff0ca 100644
> > --- a/include/net/rtnetlink.h
> > +++ b/include/net/rtnetlink.h
> > @@ -110,6 +110,9 @@ struct rtnl_link_ops {
> >  	int			(*fill_linkxstats)(struct sk_buff *skb,
> >  						   const struct net_device *dev,
> >  						   int *prividx, int attr);
> > +	size_t			(*link_down_reason_get_size)(const struct net_device *dev);
> > +	int			(*fill_link_down_reason)(struct sk_buff *skb,
> > +							 const struct net_device *dev);
> 
> IMHO the API is a little heavy for returning, what is effectively a u64
> value sliced in two..

Agreed. Unless we can expect more attributes to follow, repeating the
same code to generate netlink attributes in each driver doesn't feel
right.

> Perhaps the core can just assume the reason will be provided if the NDO
> is present?  And the "fill" NDO should probably fill in the reason
> structure, rather than getting the skb passed and dealing with netlink
> directly.

We could always reserve e.g. 0 as "none" which driver would use e.g. if
it only provides major reason id and not minor.

> Also perhaps this would be a ethtool-nl candidate (which would
> hopefully land soon after the merge window)?

Looking at the suggested values for major reasons, it does indeed seem
to belong to link related information provided by ETHTOOL_GLINKSETTINGS
request. Maybe nesting the reason with link state, i.e.

    ETHA_SETTINGS_LINK_STATE		(nested)
        ETHA_LINKSTATE_LINK		    (u8)
        ETHA_LINKSTATE_DOWN_REASON_MAJOR    (u32)
        ETHA_LINKSTATE_DOWN_REASON_MINOR    (u32)

Michal