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]

Message-ID: <8736nkidje.fsf@mellanox.com>
Date:   Mon, 18 Mar 2019 12:15:56 +0000
From:   Petr Machata <petrm@...lanox.com>
To:     Roopa Prabhu <roopa@...ulusnetworks.com>
CC:     "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>,
        "jakub.kicinski@...ronome.com" <jakub.kicinski@...ronome.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


Roopa Prabhu <roopa@...ulusnetworks.com> writes:

> This looks good and will be use-full. But i have some comments on the
> implementation below.
> Also, carrier can go down due to protocol down (IFLA_PROTODOWN). And I
> get asked about supporting
> reason codes or protocol owner for such protodown reason (I have not
> given it much thought yet. I will see if there is a way
> to use your series for that as well).

My thinking was that since protocol down is set from userspace, it's
under admin control, and that's where the reason signalling should be.

>> ---
>>  include/net/rtnetlink.h      |  3 +++
>>  include/uapi/linux/if_link.h | 16 ++++++++++++++++
>>  net/core/rtnetlink.c         | 22 ++++++++++++++++++++++
>>  3 files changed, 41 insertions(+)
>>
>> 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);
>>  };
>
> Any reason to restrict this to network interfaces which support rtnl_link_ops ?.
> I also saw that you added rtnl_link_ops to mlxsw for this. Which also
> means every driver will now have to declare rtnl_link_ops to use this
> ?.
> I think we should keep rtnl_link_ops to logical links  like bridge,
> bonds etc (ie which support newlink and dellink).
>
> Can't we use netdev_ops for this ?. That will allow any driver to just
> support this readily.

I guess you are right.

>> +enum rtnl_link_down_reason_major {
>> +       RTNL_LDR_OTHER,
>> +       RTNL_LDR_NO_CABLE,
>> +       RTNL_LDR_UNSUPPORTED_CABLE,
>> +       RTNL_LDR_AUTONEG_FAILURE,
>> +       RTNL_LDR_NO_LINK_PARTNER,
>> +       RTNL_LDR_LINK_TRAINING_FAILURE,
>> +       RTNL_LDR_LOGICAL_MISMATCH,
>> +       RTNL_LDR_REMOTE_FAULT,
>> +       RTNL_LDR_BAD_SIGNAL_INTEGRITY,
>> +       RTNL_LDR_CALIBRATION_FAILURE,
>> +       RTNL_LDR_POWER_BUDGET_EXCEEDED,
>> +};
>
> prefer LINK_DOWN_REASON_* or LINKDOWN_REASON_*
> (Though there is no predefined convention, the prefix RTNL makes it
> feel like a top-level attribute when its really a value for an IFLA_*
> attribute.)

OK.