[net] Revert "rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink"

This reverts commit 64ff90cc2e6f42596d7a0c37e41dc95292bb63b1.

The initial test was right. The goal is to advertised any modifications of
any setting of a link. The test ensures that the user won't lose an update.

CC: David Ahern <dsahern@gmail.com>
CC: Xin Long <lucien.xin@gmail.com>
Reported-by: Vlad Yasevich <vyasevic@redhat.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/core/rtnetlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Thu, Oct 26, 2017 at 4:21 PM, Nicolas Dichtel
<nicolas.dichtel@6wind.com> wrote:
> This reverts commit 64ff90cc2e6f42596d7a0c37e41dc95292bb63b1.
>
> The initial test was right. The goal is to advertised any modifications of
> any setting of a link. The test ensures that the user won't lose an update.
>
> CC: David Ahern <dsahern@gmail.com>
> CC: Xin Long <lucien.xin@gmail.com>
> Reported-by: Vlad Yasevich <vyasevic@redhat.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>  net/core/rtnetlink.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 5ace48926b19..52689c399b6c 100644
> --- 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.

2. why do you think it should be done only for the changes via netlink,
    what about the changes via net-sysfs, dev_ioctl ?

3. how about the duplicated notifications issue ?

>                         netdev_state_change(dev);
>
>                 if (err < 0)
> --
> 2.13.2
>

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

On 10/26/17 2:21 AM, Nicolas Dichtel wrote:
> This reverts commit 64ff90cc2e6f42596d7a0c37e41dc95292bb63b1.
> 
> The initial test was right. The goal is to advertised any modifications of
> any setting of a link. The test ensures that the user won't lose an update.
> 
> CC: David Ahern <dsahern@gmail.com>
> CC: Xin Long <lucien.xin@gmail.com>
> Reported-by: Vlad Yasevich <vyasevic@redhat.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>  net/core/rtnetlink.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

To be explicit based on the followup comments, Dave, please drop this patch.

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Thu, 26 Oct 2017 10:21:53 +0200

> This reverts commit 64ff90cc2e6f42596d7a0c37e41dc95292bb63b1.
> 
> The initial test was right. The goal is to advertised any modifications of
> any setting of a link. The test ensures that the user won't lose an update.
> 
> CC: David Ahern <dsahern@gmail.com>
> CC: Xin Long <lucien.xin@gmail.com>
> Reported-by: Vlad Yasevich <vyasevic@redhat.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

As requested I've dropped this patch.

Thank you.