* [PATCH net 0/6] rtnetlink: a bunch of fixes for userspace notifications in changing dev properties
@ 2017-10-15 10:13 Xin Long
2017-10-15 10:13 ` [PATCH net 1/6] rtnetlink: bring NETDEV_CHANGEMTU event process back in rtnetlink_event Xin Long
` (2 more replies)
0 siblings, 3 replies; 23+ messages in thread
From: Xin Long @ 2017-10-15 10:13 UTC (permalink / raw)
To: network dev; +Cc: davem, David Ahern, hannes
Whenever any property of a link, address, route, etc. changes by whatever way,
kernel should notify the programs that listen for such events in userspace.
The patchet "rtnetlink: Cleanup user notifications for netdev events" tried to
fix a redundant notifications issue, but it also introduced a side effect.
After that, user notifications could only be sent when changing dev properties
via netlink api. As it removed some events process in rtnetlink_event where
the notifications was sent to users.
It resulted in no notification generated when dev properties are changed via
other ways, like ioctl, sysfs, etc. It may cause some user programs doesn't
work as expected because of the missing notifications.
This patchset will fix it by bringing some of these netdev events back and
also fix the old redundant notifications issue with a proper way.
Xin Long (6):
rtnetlink: bring NETDEV_CHANGEMTU event process back in
rtnetlink_event
rtnetlink: bring NETDEV_CHANGE_TX_QUEUE_LEN event process back in
rtnetlink_event
rtnetlink: bring NETDEV_POST_TYPE_CHANGE event process back in
rtnetlink_event
rtnetlink: bring NETDEV_CHANGEUPPER event process back in
rtnetlink_event
rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink
rtnetlink: do not set notification for tx_queue_len in do_setlink
net/core/rtnetlink.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
--
2.1.0
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH net 1/6] rtnetlink: bring NETDEV_CHANGEMTU event process back in rtnetlink_event
2017-10-15 10:13 [PATCH net 0/6] rtnetlink: a bunch of fixes for userspace notifications in changing dev properties Xin Long
@ 2017-10-15 10:13 ` Xin Long
2017-10-15 10:13 ` [PATCH net 2/6] rtnetlink: bring NETDEV_CHANGE_TX_QUEUE_LEN " Xin Long
2017-10-16 19:49 ` [PATCH net 0/6] rtnetlink: a bunch of fixes for userspace notifications in changing dev properties David Miller
2017-10-24 20:44 ` Stefano Brivio
2 siblings, 1 reply; 23+ messages in thread
From: Xin Long @ 2017-10-15 10:13 UTC (permalink / raw)
To: network dev; +Cc: davem, David Ahern, hannes
Commit 085e1a65f04f ("rtnetlink: Do not generate notifications for MTU
events") tried to fix the redundant notifications issue when ip link
set mtu by removing NETDEV_CHANGEMTU event process in rtnetlink_event.
But it also resulted in no notification generated when dev's mtu is
changed via other methods, like:
'ifconfig eth1 mtu 1400' or 'echo 1400 > /sys/class/net/eth1/mtu'
It would cause users not to be notified by this change.
This patch is to fix it by bringing NETDEV_CHANGEMTU event back into
rtnetlink_event, and the redundant notifications issue will be fixed
in the later patch 'rtnetlink: check DO_SETLINK_NOTIFY correctly in
do_setlink'.
Fixes: 085e1a65f04f ("rtnetlink: Do not generate notifications for MTU events")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/core/rtnetlink.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index d4bcdcc..72053ed 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -4279,6 +4279,7 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi
switch (event) {
case NETDEV_REBOOT:
+ case NETDEV_CHANGEMTU:
case NETDEV_CHANGEADDR:
case NETDEV_CHANGENAME:
case NETDEV_FEAT_CHANGE:
--
2.1.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH net 2/6] rtnetlink: bring NETDEV_CHANGE_TX_QUEUE_LEN event process back in rtnetlink_event
2017-10-15 10:13 ` [PATCH net 1/6] rtnetlink: bring NETDEV_CHANGEMTU event process back in rtnetlink_event Xin Long
@ 2017-10-15 10:13 ` Xin Long
2017-10-15 10:13 ` [PATCH net 3/6] rtnetlink: bring NETDEV_POST_TYPE_CHANGE " Xin Long
2017-10-16 1:16 ` [PATCH net 2/6] rtnetlink: bring NETDEV_CHANGE_TX_QUEUE_LEN " David Ahern
0 siblings, 2 replies; 23+ messages in thread
From: Xin Long @ 2017-10-15 10:13 UTC (permalink / raw)
To: network dev; +Cc: davem, David Ahern, hannes
The same fix for changing mtu in the patch 'rtnetlink: bring
NETDEV_CHANGEMTU event process back in rtnetlink_event' is
needed for changing tx_queue_len.
Note that the redundant notifications issue for tx_queue_len
will be fixed in the later patch 'rtnetlink: do not send
notification for tx_queue_len in do_setlink'.
Fixes: 27b3b551d8a7 ("rtnetlink: Do not generate notifications for NETDEV_CHANGE_TX_QUEUE_LEN event")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/core/rtnetlink.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 72053ed..bf47360 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -4287,6 +4287,7 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi
case NETDEV_NOTIFY_PEERS:
case NETDEV_RESEND_IGMP:
case NETDEV_CHANGEINFODATA:
+ case NETDEV_CHANGE_TX_QUEUE_LEN:
rtmsg_ifinfo_event(RTM_NEWLINK, dev, 0, rtnl_get_event(event),
GFP_KERNEL);
break;
--
2.1.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH net 3/6] rtnetlink: bring NETDEV_POST_TYPE_CHANGE event process back in rtnetlink_event
2017-10-15 10:13 ` [PATCH net 2/6] rtnetlink: bring NETDEV_CHANGE_TX_QUEUE_LEN " Xin Long
@ 2017-10-15 10:13 ` Xin Long
2017-10-15 10:13 ` [PATCH net 4/6] rtnetlink: bring NETDEV_CHANGEUPPER " Xin Long
2017-10-16 1:16 ` [PATCH net 3/6] rtnetlink: bring NETDEV_POST_TYPE_CHANGE " David Ahern
2017-10-16 1:16 ` [PATCH net 2/6] rtnetlink: bring NETDEV_CHANGE_TX_QUEUE_LEN " David Ahern
1 sibling, 2 replies; 23+ messages in thread
From: Xin Long @ 2017-10-15 10:13 UTC (permalink / raw)
To: network dev; +Cc: davem, David Ahern, hannes
As I said in patch 'rtnetlink: bring NETDEV_CHANGEMTU event process back
in rtnetlink_event', removing NETDEV_POST_TYPE_CHANGE event was not the
right fix for the redundant notifications issue.
So bring this event process back to rtnetlink_event and the old redundant
notifications issue would be fixed in the later patch 'rtnetlink: check
DO_SETLINK_NOTIFY correctly in do_setlink'.
Fixes: aef091ae58aa ("rtnetlink: Do not generate notifications for POST_TYPE_CHANGE event")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/core/rtnetlink.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index bf47360..8e44fd5 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -4284,6 +4284,7 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi
case NETDEV_CHANGENAME:
case NETDEV_FEAT_CHANGE:
case NETDEV_BONDING_FAILOVER:
+ case NETDEV_POST_TYPE_CHANGE:
case NETDEV_NOTIFY_PEERS:
case NETDEV_RESEND_IGMP:
case NETDEV_CHANGEINFODATA:
--
2.1.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH net 4/6] rtnetlink: bring NETDEV_CHANGEUPPER event process back in rtnetlink_event
2017-10-15 10:13 ` [PATCH net 3/6] rtnetlink: bring NETDEV_POST_TYPE_CHANGE " Xin Long
@ 2017-10-15 10:13 ` Xin Long
2017-10-15 10:13 ` [PATCH net 5/6] rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink Xin Long
2017-10-16 1:16 ` [PATCH net 4/6] rtnetlink: bring NETDEV_CHANGEUPPER event process back in rtnetlink_event David Ahern
2017-10-16 1:16 ` [PATCH net 3/6] rtnetlink: bring NETDEV_POST_TYPE_CHANGE " David Ahern
1 sibling, 2 replies; 23+ messages in thread
From: Xin Long @ 2017-10-15 10:13 UTC (permalink / raw)
To: network dev; +Cc: davem, David Ahern, hannes
libteam needs this event notification in userspace when dev's master
dev has been changed. After this, the redundant notifications issue
would be fixed in the later patch 'rtnetlink: check DO_SETLINK_NOTIFY
correctly in do_setlink'.
Fixes: b6b36eb23a46 ("rtnetlink: Do not generate notifications for NETDEV_CHANGEUPPER event")
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/core/rtnetlink.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 8e44fd5..ab98c1c 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -4286,6 +4286,7 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi
case NETDEV_BONDING_FAILOVER:
case NETDEV_POST_TYPE_CHANGE:
case NETDEV_NOTIFY_PEERS:
+ case NETDEV_CHANGEUPPER:
case NETDEV_RESEND_IGMP:
case NETDEV_CHANGEINFODATA:
case NETDEV_CHANGE_TX_QUEUE_LEN:
--
2.1.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH net 5/6] rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink
2017-10-15 10:13 ` [PATCH net 4/6] rtnetlink: bring NETDEV_CHANGEUPPER " Xin Long
@ 2017-10-15 10:13 ` Xin Long
2017-10-15 10:13 ` [PATCH net 6/6] rtnetlink: do not set notification for tx_queue_len " Xin Long
2017-10-16 1:17 ` [PATCH net 5/6] rtnetlink: check DO_SETLINK_NOTIFY correctly " David Ahern
2017-10-16 1:16 ` [PATCH net 4/6] rtnetlink: bring NETDEV_CHANGEUPPER event process back in rtnetlink_event David Ahern
1 sibling, 2 replies; 23+ messages in thread
From: Xin Long @ 2017-10-15 10:13 UTC (permalink / raw)
To: network dev; +Cc: davem, David Ahern, hannes
The check 'status & DO_SETLINK_NOTIFY' in do_setlink doesn't really
work after status & DO_SETLINK_MODIFIED, as:
DO_SETLINK_MODIFIED 0x1
DO_SETLINK_NOTIFY 0x3
Considering that notifications are suppposed to be sent only when
status have the flag DO_SETLINK_NOTIFY, the right check would be:
(status & DO_SETLINK_NOTIFY) == DO_SETLINK_NOTIFY
This would avoid lots of duplicated notifications when setting some
properties of a link.
Fixes: ba9989069f4e ("rtnl/do_setlink(): notify when a netdev is modified")
Signed-off-by: Xin Long <lucien.xin@gmail.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 ab98c1c..3e98fb5 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2248,7 +2248,7 @@ static int do_setlink(const struct sk_buff *skb,
errout:
if (status & DO_SETLINK_MODIFIED) {
- if (status & DO_SETLINK_NOTIFY)
+ if ((status & DO_SETLINK_NOTIFY) == DO_SETLINK_NOTIFY)
netdev_state_change(dev);
if (err < 0)
--
2.1.0
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH net 6/6] rtnetlink: do not set notification for tx_queue_len in do_setlink
2017-10-15 10:13 ` [PATCH net 5/6] rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink Xin Long
@ 2017-10-15 10:13 ` Xin Long
2017-10-16 1:17 ` David Ahern
2017-10-16 1:17 ` [PATCH net 5/6] rtnetlink: check DO_SETLINK_NOTIFY correctly " David Ahern
1 sibling, 1 reply; 23+ messages in thread
From: Xin Long @ 2017-10-15 10:13 UTC (permalink / raw)
To: network dev; +Cc: davem, David Ahern, hannes
NETDEV_CHANGE_TX_QUEUE_LEN event process in rtnetlink_event would
send a notification for userspace and tx_queue_len's setting in
do_setlink would trigger NETDEV_CHANGE_TX_QUEUE_LEN.
So it shouldn't set DO_SETLINK_NOTIFY status for this change to
send a notification any more.
Signed-off-by: Xin Long <lucien.xin@gmail.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 3e98fb5..a6bcf86 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2093,7 +2093,7 @@ static int do_setlink(const struct sk_buff *skb,
dev->tx_queue_len = orig_len;
goto errout;
}
- status |= DO_SETLINK_NOTIFY;
+ status |= DO_SETLINK_MODIFIED;
}
}
--
2.1.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH net 6/6] rtnetlink: do not set notification for tx_queue_len in do_setlink
2017-10-15 10:13 ` [PATCH net 6/6] rtnetlink: do not set notification for tx_queue_len " Xin Long
@ 2017-10-16 1:17 ` David Ahern
0 siblings, 0 replies; 23+ messages in thread
From: David Ahern @ 2017-10-16 1:17 UTC (permalink / raw)
To: Xin Long, network dev; +Cc: davem, hannes
On 10/15/17 4:13 AM, Xin Long wrote:
> NETDEV_CHANGE_TX_QUEUE_LEN event process in rtnetlink_event would
> send a notification for userspace and tx_queue_len's setting in
> do_setlink would trigger NETDEV_CHANGE_TX_QUEUE_LEN.
>
> So it shouldn't set DO_SETLINK_NOTIFY status for this change to
> send a notification any more.
>
> Signed-off-by: Xin Long <lucien.xin@gmail.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 3e98fb5..a6bcf86 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -2093,7 +2093,7 @@ static int do_setlink(const struct sk_buff *skb,
> dev->tx_queue_len = orig_len;
> goto errout;
> }
> - status |= DO_SETLINK_NOTIFY;
> + status |= DO_SETLINK_MODIFIED;
> }
> }
>
>
Acked-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH net 5/6] rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink
2017-10-15 10:13 ` [PATCH net 5/6] rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink Xin Long
2017-10-15 10:13 ` [PATCH net 6/6] rtnetlink: do not set notification for tx_queue_len " Xin Long
@ 2017-10-16 1:17 ` David Ahern
2017-10-16 12:20 ` Nicolas Dichtel
1 sibling, 1 reply; 23+ messages in thread
From: David Ahern @ 2017-10-16 1:17 UTC (permalink / raw)
To: Xin Long, network dev; +Cc: davem, hannes, Nicolas Dichtel
[ cc'ed Nicolas ]
On 10/15/17 4:13 AM, Xin Long wrote:
> The check 'status & DO_SETLINK_NOTIFY' in do_setlink doesn't really
> work after status & DO_SETLINK_MODIFIED, as:
>
> DO_SETLINK_MODIFIED 0x1
> DO_SETLINK_NOTIFY 0x3
>
> Considering that notifications are suppposed to be sent only when
> status have the flag DO_SETLINK_NOTIFY, the right check would be:
>
> (status & DO_SETLINK_NOTIFY) == DO_SETLINK_NOTIFY
>
> This would avoid lots of duplicated notifications when setting some
> properties of a link.
>
> Fixes: ba9989069f4e ("rtnl/do_setlink(): notify when a netdev is modified")
> Signed-off-by: Xin Long <lucien.xin@gmail.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 ab98c1c..3e98fb5 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -2248,7 +2248,7 @@ static int do_setlink(const struct sk_buff *skb,
>
> errout:
> if (status & DO_SETLINK_MODIFIED) {
> - if (status & DO_SETLINK_NOTIFY)
> + if ((status & DO_SETLINK_NOTIFY) == DO_SETLINK_NOTIFY)
> netdev_state_change(dev);
>
> if (err < 0)
>
Good catch.
Acked-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH net 5/6] rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink
2017-10-16 1:17 ` [PATCH net 5/6] rtnetlink: check DO_SETLINK_NOTIFY correctly " David Ahern
@ 2017-10-16 12:20 ` Nicolas Dichtel
2017-10-26 6:52 ` Vlad Yasevich
0 siblings, 1 reply; 23+ messages in thread
From: Nicolas Dichtel @ 2017-10-16 12:20 UTC (permalink / raw)
To: David Ahern, Xin Long, network dev; +Cc: davem, hannes
Le 16/10/2017 à 03:17, David Ahern a écrit :
> [ cc'ed Nicolas ]
>
> On 10/15/17 4:13 AM, Xin Long wrote:
>> The check 'status & DO_SETLINK_NOTIFY' in do_setlink doesn't really
>> work after status & DO_SETLINK_MODIFIED, as:
>>
>> DO_SETLINK_MODIFIED 0x1
>> DO_SETLINK_NOTIFY 0x3
>>
>> Considering that notifications are suppposed to be sent only when
>> status have the flag DO_SETLINK_NOTIFY, the right check would be:
>>
>> (status & DO_SETLINK_NOTIFY) == DO_SETLINK_NOTIFY
>>
>> This would avoid lots of duplicated notifications when setting some
>> properties of a link.
>>
>> Fixes: ba9989069f4e ("rtnl/do_setlink(): notify when a netdev is modified")
>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Good catch, thank you.
Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH net 5/6] rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink
2017-10-16 12:20 ` Nicolas Dichtel
@ 2017-10-26 6:52 ` Vlad Yasevich
2017-10-26 7:41 ` Nicolas Dichtel
0 siblings, 1 reply; 23+ messages in thread
From: Vlad Yasevich @ 2017-10-26 6:52 UTC (permalink / raw)
To: nicolas.dichtel, David Ahern, Xin Long, network dev; +Cc: davem, hannes
On 10/16/2017 02:20 PM, Nicolas Dichtel wrote:
> Le 16/10/2017 à 03:17, David Ahern a écrit :
>> [ cc'ed Nicolas ]
>>
>> On 10/15/17 4:13 AM, Xin Long wrote:
>>> The check 'status & DO_SETLINK_NOTIFY' in do_setlink doesn't really
>>> work after status & DO_SETLINK_MODIFIED, as:
>>>
>>> DO_SETLINK_MODIFIED 0x1
>>> DO_SETLINK_NOTIFY 0x3
>>>
>>> Considering that notifications are suppposed to be sent only when
>>> status have the flag DO_SETLINK_NOTIFY, the right check would be:
>>>
>>> (status & DO_SETLINK_NOTIFY) == DO_SETLINK_NOTIFY
>>>
>>> This would avoid lots of duplicated notifications when setting some
>>> properties of a link.
>>>
>>> Fixes: ba9989069f4e ("rtnl/do_setlink(): notify when a netdev is modified")
>>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> Good catch, thank you.
>
> Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>
So I found this the first timer around when looking at this code, but was told that
notification are expected anytime we modified any setting thus the code was simply
checking for MODIFIED bit. Has that thinking changed?
-vlad
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH net 5/6] rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink
2017-10-26 6:52 ` Vlad Yasevich
@ 2017-10-26 7:41 ` Nicolas Dichtel
2017-10-26 8:21 ` [PATCH net] Revert "rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink" Nicolas Dichtel
2017-10-26 14:59 ` [PATCH net 5/6] rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink David Ahern
0 siblings, 2 replies; 23+ messages in thread
From: Nicolas Dichtel @ 2017-10-26 7:41 UTC (permalink / raw)
To: vyasevic, David Ahern, Xin Long, network dev; +Cc: davem, hannes
Le 26/10/2017 à 08:52, Vlad Yasevich a écrit :
> On 10/16/2017 02:20 PM, Nicolas Dichtel wrote:
>> Le 16/10/2017 à 03:17, David Ahern a écrit :
>>> [ cc'ed Nicolas ]
>>>
>>> On 10/15/17 4:13 AM, Xin Long wrote:
>>>> The check 'status & DO_SETLINK_NOTIFY' in do_setlink doesn't really
>>>> work after status & DO_SETLINK_MODIFIED, as:
>>>>
>>>> DO_SETLINK_MODIFIED 0x1
>>>> DO_SETLINK_NOTIFY 0x3
>>>>
>>>> Considering that notifications are suppposed to be sent only when
>>>> status have the flag DO_SETLINK_NOTIFY, the right check would be:
>>>>
>>>> (status & DO_SETLINK_NOTIFY) == DO_SETLINK_NOTIFY
>>>>
>>>> This would avoid lots of duplicated notifications when setting some
>>>> properties of a link.
>>>>
>>>> Fixes: ba9989069f4e ("rtnl/do_setlink(): notify when a netdev is modified")
>>>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>> Good catch, thank you.
>>
>> Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>>
>
> So I found this the first timer around when looking at this code, but was told that
> notification are expected anytime we modified any setting thus the code was simply
> checking for MODIFIED bit. Has that thinking changed?
No, you're right, thank you for pointing this out. I was focus on the duplicated
notifications and forget the initial goal.
I will ask for a revert.
Regards,
Nicolas
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH net] Revert "rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink"
2017-10-26 7:41 ` Nicolas Dichtel
@ 2017-10-26 8:21 ` Nicolas Dichtel
2017-10-26 9:10 ` Xin Long
` (2 more replies)
2017-10-26 14:59 ` [PATCH net 5/6] rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink David Ahern
1 sibling, 3 replies; 23+ messages in thread
From: Nicolas Dichtel @ 2017-10-26 8:21 UTC (permalink / raw)
To: davem; +Cc: netdev, hannes, vyasevic, Nicolas Dichtel, David Ahern, Xin Long
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)
netdev_state_change(dev);
if (err < 0)
--
2.13.2
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH net] Revert "rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink"
2017-10-26 8:21 ` [PATCH net] Revert "rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink" Nicolas Dichtel
@ 2017-10-26 9:10 ` Xin Long
2017-10-26 10:18 ` Nicolas Dichtel
2017-10-26 15:22 ` David Ahern
2017-10-27 13:27 ` David Miller
2 siblings, 1 reply; 23+ messages in thread
From: Xin Long @ 2017-10-26 9:10 UTC (permalink / raw)
To: Nicolas Dichtel
Cc: davem, network dev, Hannes Frederic Sowa, Vlad Yasevich,
David Ahern
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
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH net] Revert "rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink"
2017-10-26 9:10 ` Xin Long
@ 2017-10-26 10:18 ` Nicolas Dichtel
0 siblings, 0 replies; 23+ messages in thread
From: Nicolas Dichtel @ 2017-10-26 10:18 UTC (permalink / raw)
To: Xin Long
Cc: davem, network dev, Hannes Frederic Sowa, Vlad Yasevich,
David Ahern
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH net] Revert "rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink"
2017-10-26 8:21 ` [PATCH net] Revert "rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink" Nicolas Dichtel
2017-10-26 9:10 ` Xin Long
@ 2017-10-26 15:22 ` David Ahern
2017-10-27 13:27 ` David Miller
2 siblings, 0 replies; 23+ messages in thread
From: David Ahern @ 2017-10-26 15:22 UTC (permalink / raw)
To: Nicolas Dichtel, davem; +Cc: netdev, hannes, vyasevic, Xin Long
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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH net] Revert "rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink"
2017-10-26 8:21 ` [PATCH net] Revert "rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink" Nicolas Dichtel
2017-10-26 9:10 ` Xin Long
2017-10-26 15:22 ` David Ahern
@ 2017-10-27 13:27 ` David Miller
2 siblings, 0 replies; 23+ messages in thread
From: David Miller @ 2017-10-27 13:27 UTC (permalink / raw)
To: nicolas.dichtel; +Cc: netdev, hannes, vyasevic, dsahern, lucien.xin
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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH net 5/6] rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink
2017-10-26 7:41 ` Nicolas Dichtel
2017-10-26 8:21 ` [PATCH net] Revert "rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink" Nicolas Dichtel
@ 2017-10-26 14:59 ` David Ahern
1 sibling, 0 replies; 23+ messages in thread
From: David Ahern @ 2017-10-26 14:59 UTC (permalink / raw)
To: nicolas.dichtel, vyasevic, Xin Long, network dev; +Cc: davem, hannes
On 10/26/17 1:41 AM, Nicolas Dichtel wrote:
>> So I found this the first timer around when looking at this code, but was told that
>> notification are expected anytime we modified any setting thus the code was simply
>> checking for MODIFIED bit. Has that thinking changed?
> No, you're right, thank you for pointing this out. I was focus on the duplicated
> notifications and forget the initial goal.
>
> I will ask for a revert.
For everyone of the MODIFIED but not NOTIFY is there already a message
sent? If so, no revert is needed. If not, then perhaps those need the
NOTIFY bit set.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH net 4/6] rtnetlink: bring NETDEV_CHANGEUPPER event process back in rtnetlink_event
2017-10-15 10:13 ` [PATCH net 4/6] rtnetlink: bring NETDEV_CHANGEUPPER " Xin Long
2017-10-15 10:13 ` [PATCH net 5/6] rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink Xin Long
@ 2017-10-16 1:16 ` David Ahern
1 sibling, 0 replies; 23+ messages in thread
From: David Ahern @ 2017-10-16 1:16 UTC (permalink / raw)
To: Xin Long, network dev; +Cc: davem, hannes
On 10/15/17 4:13 AM, Xin Long wrote:
> libteam needs this event notification in userspace when dev's master
> dev has been changed. After this, the redundant notifications issue
> would be fixed in the later patch 'rtnetlink: check DO_SETLINK_NOTIFY
> correctly in do_setlink'.
>
> Fixes: b6b36eb23a46 ("rtnetlink: Do not generate notifications for NETDEV_CHANGEUPPER event")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
> net/core/rtnetlink.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 8e44fd5..ab98c1c 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -4286,6 +4286,7 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi
> case NETDEV_BONDING_FAILOVER:
> case NETDEV_POST_TYPE_CHANGE:
> case NETDEV_NOTIFY_PEERS:
> + case NETDEV_CHANGEUPPER:
> case NETDEV_RESEND_IGMP:
> case NETDEV_CHANGEINFODATA:
> case NETDEV_CHANGE_TX_QUEUE_LEN:
>
Acked-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH net 3/6] rtnetlink: bring NETDEV_POST_TYPE_CHANGE event process back in rtnetlink_event
2017-10-15 10:13 ` [PATCH net 3/6] rtnetlink: bring NETDEV_POST_TYPE_CHANGE " Xin Long
2017-10-15 10:13 ` [PATCH net 4/6] rtnetlink: bring NETDEV_CHANGEUPPER " Xin Long
@ 2017-10-16 1:16 ` David Ahern
1 sibling, 0 replies; 23+ messages in thread
From: David Ahern @ 2017-10-16 1:16 UTC (permalink / raw)
To: Xin Long, network dev; +Cc: davem, hannes
On 10/15/17 4:13 AM, Xin Long wrote:
> As I said in patch 'rtnetlink: bring NETDEV_CHANGEMTU event process back
> in rtnetlink_event', removing NETDEV_POST_TYPE_CHANGE event was not the
> right fix for the redundant notifications issue.
>
> So bring this event process back to rtnetlink_event and the old redundant
> notifications issue would be fixed in the later patch 'rtnetlink: check
> DO_SETLINK_NOTIFY correctly in do_setlink'.
>
> Fixes: aef091ae58aa ("rtnetlink: Do not generate notifications for POST_TYPE_CHANGE event")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
> net/core/rtnetlink.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index bf47360..8e44fd5 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -4284,6 +4284,7 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi
> case NETDEV_CHANGENAME:
> case NETDEV_FEAT_CHANGE:
> case NETDEV_BONDING_FAILOVER:
> + case NETDEV_POST_TYPE_CHANGE:
> case NETDEV_NOTIFY_PEERS:
> case NETDEV_RESEND_IGMP:
> case NETDEV_CHANGEINFODATA:
>
Acked-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH net 2/6] rtnetlink: bring NETDEV_CHANGE_TX_QUEUE_LEN event process back in rtnetlink_event
2017-10-15 10:13 ` [PATCH net 2/6] rtnetlink: bring NETDEV_CHANGE_TX_QUEUE_LEN " Xin Long
2017-10-15 10:13 ` [PATCH net 3/6] rtnetlink: bring NETDEV_POST_TYPE_CHANGE " Xin Long
@ 2017-10-16 1:16 ` David Ahern
1 sibling, 0 replies; 23+ messages in thread
From: David Ahern @ 2017-10-16 1:16 UTC (permalink / raw)
To: Xin Long, network dev; +Cc: davem, hannes
On 10/15/17 4:13 AM, Xin Long wrote:
> The same fix for changing mtu in the patch 'rtnetlink: bring
> NETDEV_CHANGEMTU event process back in rtnetlink_event' is
> needed for changing tx_queue_len.
>
> Note that the redundant notifications issue for tx_queue_len
> will be fixed in the later patch 'rtnetlink: do not send
> notification for tx_queue_len in do_setlink'.
>
> Fixes: 27b3b551d8a7 ("rtnetlink: Do not generate notifications for NETDEV_CHANGE_TX_QUEUE_LEN event")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
> net/core/rtnetlink.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 72053ed..bf47360 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -4287,6 +4287,7 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi
> case NETDEV_NOTIFY_PEERS:
> case NETDEV_RESEND_IGMP:
> case NETDEV_CHANGEINFODATA:
> + case NETDEV_CHANGE_TX_QUEUE_LEN:
> rtmsg_ifinfo_event(RTM_NEWLINK, dev, 0, rtnl_get_event(event),
> GFP_KERNEL);
> break;
>
Acked-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH net 0/6] rtnetlink: a bunch of fixes for userspace notifications in changing dev properties
2017-10-15 10:13 [PATCH net 0/6] rtnetlink: a bunch of fixes for userspace notifications in changing dev properties Xin Long
2017-10-15 10:13 ` [PATCH net 1/6] rtnetlink: bring NETDEV_CHANGEMTU event process back in rtnetlink_event Xin Long
@ 2017-10-16 19:49 ` David Miller
2017-10-24 20:44 ` Stefano Brivio
2 siblings, 0 replies; 23+ messages in thread
From: David Miller @ 2017-10-16 19:49 UTC (permalink / raw)
To: lucien.xin; +Cc: netdev, dsahern, hannes
From: Xin Long <lucien.xin@gmail.com>
Date: Sun, 15 Oct 2017 18:13:40 +0800
> Whenever any property of a link, address, route, etc. changes by whatever way,
> kernel should notify the programs that listen for such events in userspace.
>
> The patchet "rtnetlink: Cleanup user notifications for netdev events" tried to
> fix a redundant notifications issue, but it also introduced a side effect.
>
> After that, user notifications could only be sent when changing dev properties
> via netlink api. As it removed some events process in rtnetlink_event where
> the notifications was sent to users.
>
> It resulted in no notification generated when dev properties are changed via
> other ways, like ioctl, sysfs, etc. It may cause some user programs doesn't
> work as expected because of the missing notifications.
>
> This patchset will fix it by bringing some of these netdev events back and
> also fix the old redundant notifications issue with a proper way.
Series applied, thank you.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH net 0/6] rtnetlink: a bunch of fixes for userspace notifications in changing dev properties
2017-10-15 10:13 [PATCH net 0/6] rtnetlink: a bunch of fixes for userspace notifications in changing dev properties Xin Long
2017-10-15 10:13 ` [PATCH net 1/6] rtnetlink: bring NETDEV_CHANGEMTU event process back in rtnetlink_event Xin Long
2017-10-16 19:49 ` [PATCH net 0/6] rtnetlink: a bunch of fixes for userspace notifications in changing dev properties David Miller
@ 2017-10-24 20:44 ` Stefano Brivio
2 siblings, 0 replies; 23+ messages in thread
From: Stefano Brivio @ 2017-10-24 20:44 UTC (permalink / raw)
To: davem; +Cc: Xin Long, network dev, David Ahern, hannes
On Sun, 15 Oct 2017 18:13:40 +0800
Xin Long <lucien.xin@gmail.com> wrote:
> Whenever any property of a link, address, route, etc. changes by whatever way,
> kernel should notify the programs that listen for such events in userspace.
>
> The patchet "rtnetlink: Cleanup user notifications for netdev events" tried to
> fix a redundant notifications issue, but it also introduced a side effect.
>
> After that, user notifications could only be sent when changing dev properties
> via netlink api. As it removed some events process in rtnetlink_event where
> the notifications was sent to users.
>
> It resulted in no notification generated when dev properties are changed via
> other ways, like ioctl, sysfs, etc. It may cause some user programs doesn't
> work as expected because of the missing notifications.
>
> This patchset will fix it by bringing some of these netdev events back and
> also fix the old redundant notifications issue with a proper way.
>
> Xin Long (6):
> rtnetlink: bring NETDEV_CHANGEMTU event process back in
> rtnetlink_event
> rtnetlink: bring NETDEV_CHANGE_TX_QUEUE_LEN event process back in
> rtnetlink_event
> rtnetlink: bring NETDEV_POST_TYPE_CHANGE event process back in
> rtnetlink_event
> rtnetlink: bring NETDEV_CHANGEUPPER event process back in
> rtnetlink_event
> rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink
> rtnetlink: do not set notification for tx_queue_len in do_setlink
I guess this should be considered for -stable (back to 4.12), as it
fixes quite a few critical issues in notification behaviours userspace
might rely on.
--
Stefano
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2017-10-27 13:27 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-15 10:13 [PATCH net 0/6] rtnetlink: a bunch of fixes for userspace notifications in changing dev properties Xin Long
2017-10-15 10:13 ` [PATCH net 1/6] rtnetlink: bring NETDEV_CHANGEMTU event process back in rtnetlink_event Xin Long
2017-10-15 10:13 ` [PATCH net 2/6] rtnetlink: bring NETDEV_CHANGE_TX_QUEUE_LEN " Xin Long
2017-10-15 10:13 ` [PATCH net 3/6] rtnetlink: bring NETDEV_POST_TYPE_CHANGE " Xin Long
2017-10-15 10:13 ` [PATCH net 4/6] rtnetlink: bring NETDEV_CHANGEUPPER " Xin Long
2017-10-15 10:13 ` [PATCH net 5/6] rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink Xin Long
2017-10-15 10:13 ` [PATCH net 6/6] rtnetlink: do not set notification for tx_queue_len " Xin Long
2017-10-16 1:17 ` David Ahern
2017-10-16 1:17 ` [PATCH net 5/6] rtnetlink: check DO_SETLINK_NOTIFY correctly " David Ahern
2017-10-16 12:20 ` Nicolas Dichtel
2017-10-26 6:52 ` Vlad Yasevich
2017-10-26 7:41 ` Nicolas Dichtel
2017-10-26 8:21 ` [PATCH net] Revert "rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink" Nicolas Dichtel
2017-10-26 9:10 ` Xin Long
2017-10-26 10:18 ` Nicolas Dichtel
2017-10-26 15:22 ` David Ahern
2017-10-27 13:27 ` David Miller
2017-10-26 14:59 ` [PATCH net 5/6] rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink David Ahern
2017-10-16 1:16 ` [PATCH net 4/6] rtnetlink: bring NETDEV_CHANGEUPPER event process back in rtnetlink_event David Ahern
2017-10-16 1:16 ` [PATCH net 3/6] rtnetlink: bring NETDEV_POST_TYPE_CHANGE " David Ahern
2017-10-16 1:16 ` [PATCH net 2/6] rtnetlink: bring NETDEV_CHANGE_TX_QUEUE_LEN " David Ahern
2017-10-16 19:49 ` [PATCH net 0/6] rtnetlink: a bunch of fixes for userspace notifications in changing dev properties David Miller
2017-10-24 20:44 ` Stefano Brivio
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).