netdev - Re: [PATCH net-next 1/8] rtnetlink: Do not generate notifications for MTU events

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: <63e0a6c1-ec09-4f04-7e41-edd0adf871b2@cumulusnetworks.com>
Date:   Sat, 8 Apr 2017 14:13:55 -0400
From:   David Ahern <dsa@...ulusnetworks.com>
To:     Roopa Prabhu <roopa@...ulusnetworks.com>
Cc:     netdev@...r.kernel.org, vyasevic@...hat.com, davem@...emloft.net
Subject: Re: [PATCH net-next 1/8] rtnetlink: Do not generate notifications for
 MTU events

On 4/8/17 2:06 PM, Roopa Prabhu wrote:
> On 4/7/17, 2:25 PM, David Ahern wrote:
>> Changing MTU on a link currently causes 3 messages to be sent to userspace:
>>
>> [LINK]11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1490 qdisc noqueue state UNKNOWN group default event PRE_CHANGE_MTU
>>     link/ether f2:52:5c:6d:21:f3 brd ff:ff:ff:ff:ff:ff
>>
>> [LINK]11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default event CHANGE_MTU
>>     link/ether f2:52:5c:6d:21:f3 brd ff:ff:ff:ff:ff:ff
>>
>> [LINK]11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default
>>     link/ether f2:52:5c:6d:21:f3 brd ff:ff:ff:ff:ff:ff
>>
>> Remove the PRE_CHANGE_MTU and CHANGE_MTU messages.
>>
>>
> 
> This change is good... multiple notifications for the same event does not help in large scale links setups. However, this
> reverts what vlad was trying to do with his patchset. Vlad's patch-set relies on the rtnl notifications generated from
> notifiers (rtnetlink_event) to add  specific event (IFLA_EVENT) in notifications.
> 
> The third notification in your example above is the correct one and is an aggregate notification for a set of changes, but
> it cannot really fill in all types of events in the single IFLA_EVENT attribute as it stands today.  IFLA_EVENT should be
> a bitmask to include all events in this case (i had indicated this in vlads first version).
> 

Agreed. I think it would be best to revert def12888c161 before the UAPI
goes out.

The change can instead add the IFLA_EVENT as a bitmask mentioned here to
note the changes in a setlink. On top of that, remove the notifications
for the events I mentioned in this set to reduce the overhead on userspace.