netdev - Re: [PATCH net] Revert "rtnetlink: check DO_SETLINK_NOTIFY correctly in do

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: <adf3cc8e-7ff2-2abd-2de3-05bc442ac78f@6wind.com>
Date:   Thu, 26 Oct 2017 12:18:08 +0200
From:   Nicolas Dichtel <nicolas.dichtel@...nd.com>
To:     Xin Long <lucien.xin@...il.com>
Cc:     davem <davem@...emloft.net>, network dev <netdev@...r.kernel.org>,
        Hannes Frederic Sowa <hannes@...essinduktion.org>,
        Vlad Yasevich <vyasevic@...hat.com>,
        David Ahern <dsahern@...il.com>
Subject: Re: [PATCH net] Revert "rtnetlink: check DO_SETLINK_NOTIFY correctly
 in do_setlink"

Le 26/10/2017 à 11:10, Xin Long a écrit :
[snip]
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
>> @@ -2251,7 +2251,7 @@ static int do_setlink(const struct sk_buff *skb,
>>
>>  errout:
>>         if (status & DO_SETLINK_MODIFIED) {
>> -               if ((status & DO_SETLINK_NOTIFY) == DO_SETLINK_NOTIFY)
>> +               if (status & DO_SETLINK_NOTIFY)
> Just few questions about this ?
> 
> 1. the check is meaningless here. As it would also return true.
Right.

> 
> 2. why do you think it should be done only for the changes via netlink,
>     what about the changes via net-sysfs, dev_ioctl ?
I don't think it should be done for netlink only. I fixed the problem I
saw/reproduced.

> 
> 3. how about the duplicated notifications issue ?
In fact, it seems it's a bit hard for me to remember exactly how I fixed this in
the past :/

The explanation is in the initial patch:

"The new flag has been set only when the change did not cause a call to the
notifier chain and/or to the netlink notification functions."

It means that each time DO_SETLINK_MODIFY is set, we know that a netlink message
was already sent (by a call to call_netdevice_notifiers() with a event in the
white list of rtnetlink_event() or by a call to rtmsg_ifinfo_event()).

Thus:
1/ your patch is good and this revert is wrong (sorry for that)

2/ When the patch was done, rtnetlink_event() had a black list, not a white list
(see commit 5138e86f1760 ("rtnetlink: Convert rtnetlink_event to white list")).
So, I audit the places where DO_SETLINK_MODIFY is used. A event (in the white
list) is effectively sent when this flag is set.

3/ Vlad, did you find a change for which a netlink message is missing?

Comments are welcomed ;-)

Regards,
Nicolas