rtnetlink: a bunch of fixes for userspace notifications in changing dev properties

PATCH In review Xin Long 6 participants 23 messages Oct 15, 2017 – Oct 27, 2017

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Xin Long

author

2017-10-15 10:13 UTC #0 lore raw

cover rtnetlink: a bunch of fixes for userspace notifications in changing dev properties

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(-)

Signature · 1 line

2.1.0

Xin Long

author

2017-10-15 10:14 UTC #1 lore raw

PATCH 1/6 rtnetlink: bring NETDEV_CHANGEMTU event process back in rtnetlink_event

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 <[email protected]>

---
 net/core/rtnetlink.c | 1 +
 1 file changed, 1 insertion(+)

net/core/rtnetlink.c +1 -0

index d4bcdcc..72053ed 100644
@@ -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:

Signature · 1 line

2.1.0

Xin Long

author

2017-10-15 10:14 UTC #2 lore raw

PATCH 2/6 rtnetlink: bring NETDEV_CHANGE_TX_QUEUE_LEN event process back in rtnetlink_event

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 <[email protected]>

---
 net/core/rtnetlink.c | 1 +
 1 file changed, 1 insertion(+)

net/core/rtnetlink.c +1 -0

index 72053ed..bf47360 100644
@@ -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;

Signature · 1 line

2.1.0

Xin Long

author

2017-10-15 10:14 UTC #3 lore raw

PATCH 3/6 rtnetlink: bring NETDEV_POST_TYPE_CHANGE event process back in rtnetlink_event

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 <[email protected]>

---
 net/core/rtnetlink.c | 1 +
 1 file changed, 1 insertion(+)

net/core/rtnetlink.c +1 -0

index bf47360..8e44fd5 100644
@@ -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:

Signature · 1 line

2.1.0

Xin Long

author

2017-10-15 10:14 UTC #4 lore raw

PATCH 4/6 rtnetlink: bring NETDEV_CHANGEUPPER event process back in rtnetlink_event

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 <[email protected]>

---
 net/core/rtnetlink.c | 1 +
 1 file changed, 1 insertion(+)

net/core/rtnetlink.c +1 -0

index 8e44fd5..ab98c1c 100644
@@ -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:

Signature · 1 line

2.1.0

Xin Long

author

2017-10-15 10:14 UTC #5 lore raw

PATCH 5/6 rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink

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 <[email protected]>

---
 net/core/rtnetlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/core/rtnetlink.c +1 -1

index ab98c1c..3e98fb5 100644
@@ -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)

Signature · 1 line

2.1.0

Xin Long

author

2017-10-15 10:14 UTC #6 lore raw

PATCH 6/6 rtnetlink: do not set notification for tx_queue_len in do_setlink

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 <[email protected]>

---
 net/core/rtnetlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/core/rtnetlink.c +1 -1

index 3e98fb5..a6bcf86 100644
@@ -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;
 		}
 	}

Signature · 1 line

2.1.0

David Ahern

Acked-by on v1 6/6

2017-10-16 01:17 UTC #7 lore raw

Re: [PATCH net 6/6] rtnetlink: do not set notification for tx_queue_len in do_setlink

Quoted 18 lines · Xin Long

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 <[email protected]>
---
 net/core/rtnetlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/core/rtnetlink.c +0 -0
index 3e98fb5..a6bcf86 100644

net/core/rtnetlink.c static int do_setlink(const struct sk_buff *skb, · line 2099

Quoted 8 lines

net/core/rtnetlink.c +1 -1

@@ -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 <[email protected]>

David Ahern

Acked-by on v1 5/6

2017-10-16 01:17 UTC #8 lore raw

Re: [PATCH net 5/6] rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink

In reply to Xin Long

[ cc'ed Nicolas ]

Quoted 26 lines · Xin Long

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 <[email protected]>
---
 net/core/rtnetlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/core/rtnetlink.c +0 -0
index ab98c1c..3e98fb5 100644

net/core/rtnetlink.c static int do_setlink(const struct sk_buff *skb, · line 2254

Quoted 8 lines

net/core/rtnetlink.c +1 -1

@@ -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 <[email protected]>

Nicolas Dichtel

Acked-by

2017-10-16 12:20 UTC #9 lore raw

Re: [PATCH net 5/6] rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink

Le 16/10/2017 à 03:17, David Ahern a écrit :

Quoted 19 lines

[ cc'ed Nicolas ]
Quoted 16 lines · Xin Long
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 <[email protected]>

Good catch, thank you.

Acked-by Nicolas Dichtel <[email protected]>

Vlad Yasevich

2017-10-26 06:52 UTC #10 lore raw

Re: [PATCH net 5/6] rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink

Quoted 15 lines · Nicolas Dichtel

Quoted 14 lines · David Ahern
[ cc'ed Nicolas ]
Quoted 11 lines · Xin Long
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

Quoted 9 lines

Quoted 5 lines
Quoted 5 lines
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 <[email protected]>
Good catch, thank you.
Acked-by Nicolas Dichtel <[email protected]>

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

Nicolas Dichtel

2017-10-26 07:41 UTC #11 lore raw

Re: [PATCH net 5/6] rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink

Le 26/10/2017 à 08:52, Vlad Yasevich a écrit :

Quoted 21 lines · Nicolas Dichtel

Quoted 20 lines · David Ahern
Quoted 19 lines
[ cc'ed Nicolas ]
Quoted 16 lines · Xin Long
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 <[email protected]>

Quoted 8 lines

Quoted 4 lines
Good catch, thank you.
Acked-by Nicolas Dichtel <[email protected]>
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

Nicolas Dichtel

2017-10-26 08:22 UTC #12 lore raw

PATCH 1/1 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 <[email protected]>

CC Xin Long <[email protected]>

Reported-by Vlad Yasevich <[email protected]>

Signed-off-by Nicolas Dichtel <[email protected]>

---
 net/core/rtnetlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/core/rtnetlink.c +1 -1

index 5ace48926b19..52689c399b6c 100644
@@ -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)

Signature · 1 line

2.13.2

Xin Long

author

2017-10-26 09:10 UTC #13 lore raw

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

Quoted 18 lines · Nicolas Dichtel

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 <[email protected]>
CC Xin Long <[email protected]>
Reported-by Vlad Yasevich <[email protected]>
Signed-off-by Nicolas Dichtel <[email protected]>
---
 net/core/rtnetlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/core/rtnetlink.c +0 -0
index 5ace48926b19..52689c399b6c 100644

net/core/rtnetlink.c static int do_setlink(const struct sk_buff *skb, · line 2254

Quoted 5 lines

net/core/rtnetlink.c +1 -1

@@ -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 ?

Quoted 6 lines

net/core/rtnetlink.c +0 -1

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

2.13.2

Nicolas Dichtel

2017-10-26 10:18 UTC #14 lore raw

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

Le 26/10/2017 à 11:10, Xin Long a écrit :
[snip]

Quoted 11 lines

net/core/rtnetlink.c static int do_setlink(const struct sk_buff *skb, · line 2254
Quoted 8 lines
net/core/rtnetlink.c +1 -1
@@ -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.

Quoted 3 lines

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.

Quoted 2 lines

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

David Ahern

2017-10-26 15:22 UTC #15 lore raw

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

In reply to Nicolas Dichtel

Quoted 5 lines · Nicolas Dichtel

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.

Quoted 8 lines

CC David Ahern <[email protected]>
CC Xin Long <[email protected]>
Reported-by Vlad Yasevich <[email protected]>
Signed-off-by Nicolas Dichtel <[email protected]>
---
 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.

David Miller

maintainer

2017-10-27 13:27 UTC #16 lore raw

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

In reply to Nicolas Dichtel

From: Nicolas Dichtel <[email protected]>
Date: Thu, 26 Oct 2017 10:21:53 +0200

Quoted 9 lines

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 <[email protected]>
CC Xin Long <[email protected]>
Reported-by Vlad Yasevich <[email protected]>
Signed-off-by Nicolas Dichtel <[email protected]>

As requested I've dropped this patch.

Thank you.

David Ahern

2017-10-26 14:59 UTC #17 lore raw

Re: [PATCH net 5/6] rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink

In reply to Nicolas Dichtel

Quoted 7 lines · Nicolas Dichtel

Quoted 3 lines
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.

David Ahern

Acked-by on v1 4/6

2017-10-16 01:16 UTC #18 lore raw

Re: [PATCH net 4/6] rtnetlink: bring NETDEV_CHANGEUPPER event process back in rtnetlink_event

In reply to Xin Long

Quoted 17 lines · Xin Long

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 <[email protected]>
---
 net/core/rtnetlink.c | 1 +
 1 file changed, 1 insertion(+)
net/core/rtnetlink.c +0 -0
index 8e44fd5..ab98c1c 100644

net/core/rtnetlink.c static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi · line 4292

Quoted 7 lines

net/core/rtnetlink.c +1 -0

@@ -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 <[email protected]>

David Ahern

Acked-by on v1 3/6

2017-10-16 01:16 UTC #19 lore raw

Re: [PATCH net 3/6] rtnetlink: bring NETDEV_POST_TYPE_CHANGE event process back in rtnetlink_event

In reply to Xin Long

Quoted 20 lines · Xin Long

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 <[email protected]>
---
 net/core/rtnetlink.c | 1 +
 1 file changed, 1 insertion(+)
net/core/rtnetlink.c +0 -0
index bf47360..8e44fd5 100644

net/core/rtnetlink.c static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi · line 4290

Quoted 7 lines

net/core/rtnetlink.c +1 -0

@@ -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 <[email protected]>

David Ahern

Acked-by on v1 2/6

2017-10-16 01:16 UTC #20 lore raw

Re: [PATCH net 2/6] rtnetlink: bring NETDEV_CHANGE_TX_QUEUE_LEN event process back in rtnetlink_event

In reply to Xin Long

Quoted 20 lines · Xin Long

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 <[email protected]>
---
 net/core/rtnetlink.c | 1 +
 1 file changed, 1 insertion(+)
net/core/rtnetlink.c +0 -0
index 72053ed..bf47360 100644

net/core/rtnetlink.c static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi · line 4293

Quoted 7 lines

net/core/rtnetlink.c +1 -0

@@ -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 <[email protected]>

David Miller

maintainer

2017-10-16 19:49 UTC #21 lore raw

In reply to Xin Long

From: Xin Long <[email protected]>
Date: Sun, 15 Oct 2017 18:13:40 +0800

Quoted 10 lines

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.

Quoted 6 lines

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.

Stefano Brivio

2017-10-24 20:44 UTC #22 lore raw

In reply to Xin Long

Quoted 17 lines · On Sun, 15 Oct 2017 18:13:40 +0800 Xin Long <[email protected]> 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.

Quoted 11 lines

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.

Signature · 1 line

Stefano