[PATCH v3 net-next 0/2] rtnetlink: Updates to rtnetlink_event()

[PATCH v3 net-next 0/2] rtnetlink: Updates to rtnetlink_event()

[Vladislav Yasevich]

This is a version 3 series came out of the conversation that started
as a result my first attempt to add netdevice event info to netlink messages.

First, let's clean-up the duplicate notifications for NETDEV_CHANGNAME
event.  We get a message from the event handler as well as a result of
do_setlink().  The two messages are identical, so remove the event.

Second, update the original patch to add IFLA_EVENT attribute
to the link message to only support currently white-listed events.
Like before, this is just an attribute that gets added to the rtnetlink
message only when the messaged was generated as a result of a netdev event.
In my case, this is necessary since I want to trap NETDEV_NOTIFY_PEERS
event (also possibly NETDEV_RESEND_IGMP event) and perform certain actions
in user space.  This is not possible since the messages generated as
a result of netdev events do not usually contain any changed data.  They
are just notifications.  This patch exposes this notification type to
userspace.

I will also update my patch to iproute that will show this data
through 'ip monitor'. 


V3: Rebased.  Cleaned-up duplicate event.

V2: Added missed events (from David Ahern)

Vladislav Yasevich (2):
  rtnetlink: Convert rtnetlink_event to white list
  rtnl: Add support for netdev event to link messages

 include/linux/rtnetlink.h    |   3 +-
 include/uapi/linux/if_link.h |  21 ++++++++
 net/core/dev.c               |   2 +-
 net/core/rtnetlink.c         | 121 +++++++++++++++++++++++++++++++++++--------
 4 files changed, 123 insertions(+), 24 deletions(-)

-- 
2.7.4


Vladislav Yasevich (2):
  rtnetlink: Disable notification for NETDEV_NAMECHANGE event
  rtnl: Add support for netdev event to link messages

 include/linux/rtnetlink.h    |  3 ++-
 include/uapi/linux/if_link.h | 11 ++++++++
 net/core/dev.c               |  2 +-
 net/core/rtnetlink.c         | 63 +++++++++++++++++++++++++++++++++++++-------
 4 files changed, 67 insertions(+), 12 deletions(-)

-- 
2.7.4

[PATCH net-next 1/2] rtnetlink: Disable notification for NETDEV_NAMECHANGE event

[Vladislav Yasevich]

The data signaling name change is already provided at
the end of do_setlink().  This event handler just generates
a duplicate announcement.  Disable it.

CC: David Ahern <dsa@cumulusnetworks.com>
Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 net/core/rtnetlink.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 0ee5479..e8e6816 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -4123,7 +4123,6 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi
 
 	switch (event) {
 	case NETDEV_REBOOT:
-	case NETDEV_CHANGENAME:
 	case NETDEV_FEAT_CHANGE:
 	case NETDEV_BONDING_FAILOVER:
 	case NETDEV_NOTIFY_PEERS:
-- 
2.7.4

Re: [PATCH net-next 1/2] rtnetlink: Disable notification for NETDEV_NAMECHANGE event

[David Ahern]

On 4/21/17 11:31 AM, Vladislav Yasevich wrote:
> The data signaling name change is already provided at
> the end of do_setlink().  This event handler just generates
> a duplicate announcement.  Disable it.
> 
> CC: David Ahern <dsa@cumulusnetworks.com>
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> ---
>  net/core/rtnetlink.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 0ee5479..e8e6816 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -4123,7 +4123,6 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi
>  
>  	switch (event) {
>  	case NETDEV_REBOOT:
> -	case NETDEV_CHANGENAME:
>  	case NETDEV_FEAT_CHANGE:
>  	case NETDEV_BONDING_FAILOVER:
>  	case NETDEV_NOTIFY_PEERS:
> 


I only see one using the ip monitor.

$ ip li set foobar name fubar

generates these 3 messages:

[LINK]12: fubar: <BROADCAST,NOARP> mtu 1500 qdisc noqueue state DOWN
group default
    link/ether 76:cd:72:dd:2a:cb brd ff:ff:ff:ff:ff:ff
Unknown message: type=0x00000051(81) flags=0x00000000(0)len=0x0000001c(28)
[NETCONF]ipv4 dev dummy2 forwarding on rp_filter off mc_forwarding off
proxy_neigh off ignore_routes_with_linkdown off
Unknown message: type=0x00000051(81) flags=0x00000000(0)len=0x0000001c(28)
[NETCONF]ipv6 dev dummy2 forwarding on mc_forwarding off proxy_neigh off
ignore_routes_with_linkdown off

do_setlink only sets DO_SETLINK_MODIFIED so a name change alone will not
generate 2 messages.

Re: [PATCH net-next 1/2] rtnetlink: Disable notification for NETDEV_NAMECHANGE event

[Vlad Yasevich]

On 04/21/2017 02:08 PM, David Ahern wrote:
> On 4/21/17 11:31 AM, Vladislav Yasevich wrote:
>> The data signaling name change is already provided at
>> the end of do_setlink().  This event handler just generates
>> a duplicate announcement.  Disable it.
>>
>> CC: David Ahern <dsa@cumulusnetworks.com>
>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>> ---
>>  net/core/rtnetlink.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> index 0ee5479..e8e6816 100644
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
>> @@ -4123,7 +4123,6 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi
>>  
>>  	switch (event) {
>>  	case NETDEV_REBOOT:
>> -	case NETDEV_CHANGENAME:
>>  	case NETDEV_FEAT_CHANGE:
>>  	case NETDEV_BONDING_FAILOVER:
>>  	case NETDEV_NOTIFY_PEERS:
>>
> 
> 
> I only see one using the ip monitor.
> 
> $ ip li set foobar name fubar
> 
> generates these 3 messages:
> 
> [LINK]12: fubar: <BROADCAST,NOARP> mtu 1500 qdisc noqueue state DOWN
> group default
>     link/ether 76:cd:72:dd:2a:cb brd ff:ff:ff:ff:ff:ff
> Unknown message: type=0x00000051(81) flags=0x00000000(0)len=0x0000001c(28)
> [NETCONF]ipv4 dev dummy2 forwarding on rp_filter off mc_forwarding off
> proxy_neigh off ignore_routes_with_linkdown off
> Unknown message: type=0x00000051(81) flags=0x00000000(0)len=0x0000001c(28)
> [NETCONF]ipv6 dev dummy2 forwarding on mc_forwarding off proxy_neigh off
> ignore_routes_with_linkdown off
> 
> do_setlink only sets DO_SETLINK_MODIFIED so a name change alone will not
> generate 2 messages.
> 

Actually, it has nothing to do with above flag.  Setting DO_SETLINK_MODIFIED
will still generate notifications, but only if the device is UP.  However,
it looks like link name change can only be done when the link is down.  As
a result, netdev_state_change will not report it, so we only see the 'event'
one.

So this is patch isn't needed, but only as a kind-of side-effect..

-vlad

[PATCH net-next V3 2/2] rtnl: Add support for netdev event attribute to link messages

[Vladislav Yasevich]

When netdev events happen, a rtnetlink_event() handler will send
messages for every event in it's white list.  These messages contain
current information about a particular device, but they do not include
the iformation about which event just happened.  So, it is impossible
to tell what just happend for these events.

This patch adds a new extension to RTM_NEWLINK message called IFLA_EVENT
that would have an encoding of event that triggered this
message.  This would allow the the message consumer to easily determine
if it needs to perform certain actions.

Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 include/linux/rtnetlink.h    |  3 ++-
 include/uapi/linux/if_link.h | 11 ++++++++
 net/core/dev.c               |  2 +-
 net/core/rtnetlink.c         | 62 +++++++++++++++++++++++++++++++++++++-------
 4 files changed, 67 insertions(+), 11 deletions(-)

diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
index 57e5484..0459018 100644
--- a/include/linux/rtnetlink.h
+++ b/include/linux/rtnetlink.h
@@ -18,7 +18,8 @@ extern int rtnl_put_cacheinfo(struct sk_buff *skb, struct dst_entry *dst,
 
 void rtmsg_ifinfo(int type, struct net_device *dev, unsigned change, gfp_t flags);
 struct sk_buff *rtmsg_ifinfo_build_skb(int type, struct net_device *dev,
-				       unsigned change, gfp_t flags);
+				       unsigned change, unsigned long event,
+				       gfp_t flags);
 void rtmsg_ifinfo_send(struct sk_buff *skb, struct net_device *dev,
 		       gfp_t flags);
 
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 8b405af..aceb766 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -157,6 +157,7 @@ enum {
 	IFLA_GSO_MAX_SIZE,
 	IFLA_PAD,
 	IFLA_XDP,
+	IFLA_EVENT,
 	__IFLA_MAX
 };
 
@@ -899,4 +900,14 @@ enum {
 
 #define IFLA_XDP_MAX (__IFLA_XDP_MAX - 1)
 
+enum {
+	IFLA_EVENT_UNSPEC,
+	IFLA_EVENT_REBOOT,
+	IFLA_EVENT_FEAT_CHANGE,
+	IFLA_EVENT_BONDING_FAILOVER,
+	IFLA_EVENT_NOTIFY_PEERS,
+	IFLA_EVENT_RESEND_IGMP,
+	IFLA_EVENT_CHANGE_INFO_DATA,
+};
+
 #endif /* _UAPI_LINUX_IF_LINK_H */
diff --git a/net/core/dev.c b/net/core/dev.c
index ef9fe60e..7efb417 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6840,7 +6840,7 @@ static void rollback_registered_many(struct list_head *head)
 
 		if (!dev->rtnl_link_ops ||
 		    dev->rtnl_link_state == RTNL_LINK_INITIALIZED)
-			skb = rtmsg_ifinfo_build_skb(RTM_DELLINK, dev, ~0U,
+			skb = rtmsg_ifinfo_build_skb(RTM_DELLINK, dev, ~0U, 0,
 						     GFP_KERNEL);
 
 		/*
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index e8e6816..9082cdd 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -944,6 +944,7 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev,
 	       + nla_total_size(MAX_PHYS_ITEM_ID_LEN) /* IFLA_PHYS_SWITCH_ID */
 	       + nla_total_size(IFNAMSIZ) /* IFLA_PHYS_PORT_NAME */
 	       + rtnl_xdp_size(dev) /* IFLA_XDP */
+	       + nla_total_size(4)  /* IFLA_EVENT */
 	       + nla_total_size(1); /* IFLA_PROTO_DOWN */
 
 }
@@ -1276,9 +1277,40 @@ static int rtnl_xdp_fill(struct sk_buff *skb, struct net_device *dev)
 	return err;
 }
 
+static int rtnl_fill_link_event(struct sk_buff *skb, unsigned long event)
+{
+	u32 rtnl_event;
+
+	switch (event) {
+	case NETDEV_REBOOT:
+		rtnl_event = IFLA_EVENT_REBOOT;
+		break;
+	case NETDEV_FEAT_CHANGE:
+		rtnl_event = IFLA_EVENT_FEAT_CHANGE;
+		break;
+	case NETDEV_BONDING_FAILOVER:
+		rtnl_event = IFLA_EVENT_BONDING_FAILOVER;
+		break;
+	case NETDEV_NOTIFY_PEERS:
+		rtnl_event = IFLA_EVENT_NOTIFY_PEERS;
+		break;
+	case NETDEV_RESEND_IGMP:
+		rtnl_event = IFLA_EVENT_RESEND_IGMP;
+		break;
+	case NETDEV_CHANGEINFODATA:
+		rtnl_event = IFLA_EVENT_CHANGE_INFO_DATA;
+		break;
+	default:
+		return 0;
+	}
+
+	return nla_put_u32(skb, IFLA_EVENT, rtnl_event);
+}
+
 static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			    int type, u32 pid, u32 seq, u32 change,
-			    unsigned int flags, u32 ext_filter_mask)
+			    unsigned int flags, u32 ext_filter_mask,
+			    unsigned long event)
 {
 	struct ifinfomsg *ifm;
 	struct nlmsghdr *nlh;
@@ -1327,6 +1359,9 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 	    nla_put_u8(skb, IFLA_PROTO_DOWN, dev->proto_down))
 		goto nla_put_failure;
 
+	if (rtnl_fill_link_event(skb, event))
+		goto nla_put_failure;
+
 	if (rtnl_fill_link_ifmap(skb, dev))
 		goto nla_put_failure;
 
@@ -1461,6 +1496,7 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
 	[IFLA_LINK_NETNSID]	= { .type = NLA_S32 },
 	[IFLA_PROTO_DOWN]	= { .type = NLA_U8 },
 	[IFLA_XDP]		= { .type = NLA_NESTED },
+	[IFLA_EVENT]		= { .type = NLA_U32 },
 };
 
 static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
@@ -1620,7 +1656,7 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb)
 					       NETLINK_CB(cb->skb).portid,
 					       cb->nlh->nlmsg_seq, 0,
 					       flags,
-					       ext_filter_mask);
+					       ext_filter_mask, 0);
 			/* If we ran out of room on the first message,
 			 * we're in trouble
 			 */
@@ -2715,7 +2751,7 @@ static int rtnl_getlink(struct sk_buff *skb, struct nlmsghdr* nlh)
 		return -ENOBUFS;
 
 	err = rtnl_fill_ifinfo(nskb, dev, RTM_NEWLINK, NETLINK_CB(skb).portid,
-			       nlh->nlmsg_seq, 0, 0, ext_filter_mask);
+			       nlh->nlmsg_seq, 0, 0, ext_filter_mask, 0);
 	if (err < 0) {
 		/* -EMSGSIZE implies BUG in if_nlmsg_size */
 		WARN_ON(err == -EMSGSIZE);
@@ -2787,7 +2823,8 @@ static int rtnl_dump_all(struct sk_buff *skb, struct netlink_callback *cb)
 }
 
 struct sk_buff *rtmsg_ifinfo_build_skb(int type, struct net_device *dev,
-				       unsigned int change, gfp_t flags)
+				       unsigned int change,
+				       unsigned long event, gfp_t flags)
 {
 	struct net *net = dev_net(dev);
 	struct sk_buff *skb;
@@ -2798,7 +2835,7 @@ struct sk_buff *rtmsg_ifinfo_build_skb(int type, struct net_device *dev,
 	if (skb == NULL)
 		goto errout;
 
-	err = rtnl_fill_ifinfo(skb, dev, type, 0, 0, change, 0, 0);
+	err = rtnl_fill_ifinfo(skb, dev, type, 0, 0, change, 0, 0, event);
 	if (err < 0) {
 		/* -EMSGSIZE implies BUG in if_nlmsg_size() */
 		WARN_ON(err == -EMSGSIZE);
@@ -2819,18 +2856,25 @@ void rtmsg_ifinfo_send(struct sk_buff *skb, struct net_device *dev, gfp_t flags)
 	rtnl_notify(skb, net, 0, RTNLGRP_LINK, NULL, flags);
 }
 
-void rtmsg_ifinfo(int type, struct net_device *dev, unsigned int change,
-		  gfp_t flags)
+static void rtmsg_ifinfo_event(int type, struct net_device *dev,
+			       unsigned int change, unsigned long event,
+			       gfp_t flags)
 {
 	struct sk_buff *skb;
 
 	if (dev->reg_state != NETREG_REGISTERED)
 		return;
 
-	skb = rtmsg_ifinfo_build_skb(type, dev, change, flags);
+	skb = rtmsg_ifinfo_build_skb(type, dev, change, event, flags);
 	if (skb)
 		rtmsg_ifinfo_send(skb, dev, flags);
 }
+
+void rtmsg_ifinfo(int type, struct net_device *dev, unsigned int change,
+		  gfp_t flags)
+{
+	rtmsg_ifinfo_event(type, dev, change, 0, flags);
+}
 EXPORT_SYMBOL(rtmsg_ifinfo);
 
 static int nlmsg_populate_fdb_fill(struct sk_buff *skb,
@@ -4128,7 +4172,7 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi
 	case NETDEV_NOTIFY_PEERS:
 	case NETDEV_RESEND_IGMP:
 	case NETDEV_CHANGEINFODATA:
-		rtmsg_ifinfo(RTM_NEWLINK, dev, 0, GFP_KERNEL);
+		rtmsg_ifinfo_event(RTM_NEWLINK, dev, 0, event, GFP_KERNEL);
 		break;
 	default:
 		break;
-- 
2.7.4

Re: [PATCH net-next V3 2/2] rtnl: Add support for netdev event attribute to link messages

[David Ahern]

On 4/21/17 11:31 AM, Vladislav Yasevich wrote:
> @@ -1276,9 +1277,40 @@ static int rtnl_xdp_fill(struct sk_buff *skb, struct net_device *dev)
>  	return err;
>  }
>  
> +static int rtnl_fill_link_event(struct sk_buff *skb, unsigned long event)
> +{
> +	u32 rtnl_event;
> +
> +	switch (event) {
> +	case NETDEV_REBOOT:
> +		rtnl_event = IFLA_EVENT_REBOOT;
> +		break;
> +	case NETDEV_FEAT_CHANGE:
> +		rtnl_event = IFLA_EVENT_FEAT_CHANGE;
> +		break;
> +	case NETDEV_BONDING_FAILOVER:
> +		rtnl_event = IFLA_EVENT_BONDING_FAILOVER;
> +		break;
> +	case NETDEV_NOTIFY_PEERS:
> +		rtnl_event = IFLA_EVENT_NOTIFY_PEERS;
> +		break;
> +	case NETDEV_RESEND_IGMP:
> +		rtnl_event = IFLA_EVENT_RESEND_IGMP;
> +		break;
> +	case NETDEV_CHANGEINFODATA:
> +		rtnl_event = IFLA_EVENT_CHANGE_INFO_DATA;
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	return nla_put_u32(skb, IFLA_EVENT, rtnl_event);
> +}
> +

I still have doubts about encoding kernel events into a uapi.

For example, NETDEV_CHANGEINFODATA is only for bonds though nothing
about the name suggests it is a bonding notification. This one was added
specifically to notify userspace (d4261e5650004), yet seems to happen
only during a changelink and that already generates a RTM_NEWLINK
message via do_setlink. Since the rtnetlink_event message does not
contain anything "NETDEV_CHANGEINFODATA" related what purpose does it
really serve besides duplicating netlink messages to userspace.

The REBOOT, IGMP, FEAT_CHANGE and BONDING_FAILOVER seem to be unique
messages (code analysis only) which I get for notifying userspace.

NETDEV_NOTIFY_PEERS is not so clear in how often it duplicates other
messages.

Re: [PATCH net-next V3 2/2] rtnl: Add support for netdev event attribute to link messages

[Roopa Prabhu]

On Sun, Apr 23, 2017 at 6:07 PM, David Ahern <dsa@cumulusnetworks.com> wrote:
>
> On 4/21/17 11:31 AM, Vladislav Yasevich wrote:
> > @@ -1276,9 +1277,40 @@ static int rtnl_xdp_fill(struct sk_buff *skb, struct net_device *dev)
> >       return err;
> >  }
> >
> > +static int rtnl_fill_link_event(struct sk_buff *skb, unsigned long event)
> > +{
> > +     u32 rtnl_event;
> > +
> > +     switch (event) {
> > +     case NETDEV_REBOOT:
> > +             rtnl_event = IFLA_EVENT_REBOOT;
> > +             break;
> > +     case NETDEV_FEAT_CHANGE:
> > +             rtnl_event = IFLA_EVENT_FEAT_CHANGE;
> > +             break;
> > +     case NETDEV_BONDING_FAILOVER:
> > +             rtnl_event = IFLA_EVENT_BONDING_FAILOVER;
> > +             break;
> > +     case NETDEV_NOTIFY_PEERS:
> > +             rtnl_event = IFLA_EVENT_NOTIFY_PEERS;
> > +             break;
> > +     case NETDEV_RESEND_IGMP:
> > +             rtnl_event = IFLA_EVENT_RESEND_IGMP;
> > +             break;
> > +     case NETDEV_CHANGEINFODATA:
> > +             rtnl_event = IFLA_EVENT_CHANGE_INFO_DATA;
> > +             break;
> > +     default:
> > +             return 0;
> > +     }
> > +
> > +     return nla_put_u32(skb, IFLA_EVENT, rtnl_event);
> > +}
> > +
>
> I still have doubts about encoding kernel events into a uapi.

agree. I don't see why user-space will need NETDEV_CHANGEINFODATA and
others david listed.

My other concerns are, once we have this exposed to user-space and
user-space starts relying on it, it will need accurate information and
will expect to have this event information all the time.
IIUC, we cannot cover multiple events in a single notification and not
all link notifications will contain an IFLA_EVENT attribute. In other
words, we will be telling user-space to not expect that the kernel
will send IFLA_EVENT every time.



>
> For example, NETDEV_CHANGEINFODATA is only for bonds though nothing
> about the name suggests it is a bonding notification. This one was added
> specifically to notify userspace (d4261e5650004), yet seems to happen
> only during a changelink and that already generates a RTM_NEWLINK
> message via do_setlink. Since the rtnetlink_event message does not
> contain anything "NETDEV_CHANGEINFODATA" related what purpose does it
> really serve besides duplicating netlink messages to userspace.
>
> The REBOOT, IGMP, FEAT_CHANGE and BONDING_FAILOVER seem to be unique
> messages (code analysis only) which I get for notifying userspace.
>
> NETDEV_NOTIFY_PEERS is not so clear in how often it duplicates other
> messages.

Re: [PATCH net-next V3 2/2] rtnl: Add support for netdev event attribute to link messages

[Vlad Yasevich]

On 04/24/2017 11:14 AM, Roopa Prabhu wrote:
> On Sun, Apr 23, 2017 at 6:07 PM, David Ahern <dsa@cumulusnetworks.com> wrote:
>>
>> On 4/21/17 11:31 AM, Vladislav Yasevich wrote:
>>> @@ -1276,9 +1277,40 @@ static int rtnl_xdp_fill(struct sk_buff *skb, struct net_device *dev)
>>>       return err;
>>>  }
>>>
>>> +static int rtnl_fill_link_event(struct sk_buff *skb, unsigned long event)
>>> +{
>>> +     u32 rtnl_event;
>>> +
>>> +     switch (event) {
>>> +     case NETDEV_REBOOT:
>>> +             rtnl_event = IFLA_EVENT_REBOOT;
>>> +             break;
>>> +     case NETDEV_FEAT_CHANGE:
>>> +             rtnl_event = IFLA_EVENT_FEAT_CHANGE;
>>> +             break;
>>> +     case NETDEV_BONDING_FAILOVER:
>>> +             rtnl_event = IFLA_EVENT_BONDING_FAILOVER;
>>> +             break;
>>> +     case NETDEV_NOTIFY_PEERS:
>>> +             rtnl_event = IFLA_EVENT_NOTIFY_PEERS;
>>> +             break;
>>> +     case NETDEV_RESEND_IGMP:
>>> +             rtnl_event = IFLA_EVENT_RESEND_IGMP;
>>> +             break;
>>> +     case NETDEV_CHANGEINFODATA:
>>> +             rtnl_event = IFLA_EVENT_CHANGE_INFO_DATA;
>>> +             break;
>>> +     default:
>>> +             return 0;
>>> +     }
>>> +
>>> +     return nla_put_u32(skb, IFLA_EVENT, rtnl_event);
>>> +}
>>> +
>>
>> I still have doubts about encoding kernel events into a uapi.
> 
> agree. I don't see why user-space will need NETDEV_CHANGEINFODATA and
> others david listed.
> 

Well, I am not sure about CHANGEINFODATA as well, but I can see use
cases for others.

> My other concerns are, once we have this exposed to user-space and
> user-space starts relying on it, it will need accurate information and
> will expect to have this event information all the time.
> IIUC, we cannot cover multiple events in a single notification and not
> all link notifications will contain an IFLA_EVENT attribute.

Uhm...  If the rtnetlink message was a result of an event, it will have
an IFLA_EVENT.  If a message is something else, then it will not have
an event.  That's the point.  Not all netlink attributes are in every
netlink message.

> In other
> words, we will be telling user-space to not expect that the kernel
> will send IFLA_EVENT every time.
> 

No, we are telling the user that if it is interested in a specific event
(let's say NOTIFY_PEERS or RESEND_IGMP), then it now can monitor netlink
traffic for those events.
As things stand right now, that's not possible.

I've done this specifically for all events for which we currently generate
a netlink message.

The only concern I have is that if in the future we remove a certain netdev
event, it may impact applications.  But we may be doing it right now as well,
only silently, and the apps may have to find some ways to work around it.

There is also a potential to improve libnl caching and not invalidate the
cached data for certain events.

-vlad
> 
> 
>>
>> For example, NETDEV_CHANGEINFODATA is only for bonds though nothing
>> about the name suggests it is a bonding notification. This one was added
>> specifically to notify userspace (d4261e5650004), yet seems to happen
>> only during a changelink and that already generates a RTM_NEWLINK
>> message via do_setlink. Since the rtnetlink_event message does not
>> contain anything "NETDEV_CHANGEINFODATA" related what purpose does it
>> really serve besides duplicating netlink messages to userspace.
>>
>> The REBOOT, IGMP, FEAT_CHANGE and BONDING_FAILOVER seem to be unique
>> messages (code analysis only) which I get for notifying userspace.
>>
>> NETDEV_NOTIFY_PEERS is not so clear in how often it duplicates other
>> messages.

Re: [PATCH net-next V3 2/2] rtnl: Add support for netdev event attribute to link messages

[Roopa Prabhu]

On Thu, Apr 27, 2017 at 12:51 PM, Vlad Yasevich <vyasevic@redhat.com> wrote:
> On 04/24/2017 11:14 AM, Roopa Prabhu wrote:
>> On Sun, Apr 23, 2017 at 6:07 PM, David Ahern <dsa@cumulusnetworks.com> wrote:
>>>
>>> On 4/21/17 11:31 AM, Vladislav Yasevich wrote:
>>>> @@ -1276,9 +1277,40 @@ static int rtnl_xdp_fill(struct sk_buff *skb, struct net_device *dev)
>>>>       return err;
>>>>  }
>>>>
>>>> +static int rtnl_fill_link_event(struct sk_buff *skb, unsigned long event)
>>>> +{
>>>> +     u32 rtnl_event;
>>>> +
>>>> +     switch (event) {
>>>> +     case NETDEV_REBOOT:
>>>> +             rtnl_event = IFLA_EVENT_REBOOT;
>>>> +             break;
>>>> +     case NETDEV_FEAT_CHANGE:
>>>> +             rtnl_event = IFLA_EVENT_FEAT_CHANGE;
>>>> +             break;
>>>> +     case NETDEV_BONDING_FAILOVER:
>>>> +             rtnl_event = IFLA_EVENT_BONDING_FAILOVER;
>>>> +             break;
>>>> +     case NETDEV_NOTIFY_PEERS:
>>>> +             rtnl_event = IFLA_EVENT_NOTIFY_PEERS;
>>>> +             break;
>>>> +     case NETDEV_RESEND_IGMP:
>>>> +             rtnl_event = IFLA_EVENT_RESEND_IGMP;
>>>> +             break;
>>>> +     case NETDEV_CHANGEINFODATA:
>>>> +             rtnl_event = IFLA_EVENT_CHANGE_INFO_DATA;
>>>> +             break;
>>>> +     default:
>>>> +             return 0;
>>>> +     }
>>>> +
>>>> +     return nla_put_u32(skb, IFLA_EVENT, rtnl_event);
>>>> +}
>>>> +
>>>
>>> I still have doubts about encoding kernel events into a uapi.
>>
>> agree. I don't see why user-space will need NETDEV_CHANGEINFODATA and
>> others david listed.
>>
>
> Well, I am not sure about CHANGEINFODATA as well, but I can see use
> cases for others.
>
>> My other concerns are, once we have this exposed to user-space and
>> user-space starts relying on it, it will need accurate information and
>> will expect to have this event information all the time.
>> IIUC, we cannot cover multiple events in a single notification and not
>> all link notifications will contain an IFLA_EVENT attribute.
>
> Uhm...  If the rtnetlink message was a result of an event, it will have
> an IFLA_EVENT.  If a message is something else, then it will not have
> an event.  That's the point.  Not all netlink attributes are in every
> netlink message.
>
>> In other
>> words, we will be telling user-space to not expect that the kernel
>> will send IFLA_EVENT every time.
>>
>
> No, we are telling the user that if it is interested in a specific event
> (let's say NOTIFY_PEERS or RESEND_IGMP), then it now can monitor netlink
> traffic for those events.
> As things stand right now, that's not possible.
>
> I've done this specifically for all events for which we currently generate
> a netlink message.
>
> The only concern I have is that if in the future we remove a certain netdev
> event, it may impact applications.  But we may be doing it right now as well,
> only silently, and the apps may have to find some ways to work around it.
>

ok, fair enough. it might be ok then....except for the specific
attributes that user-space may not be interested like CHANGEINFODATA.

Re: [PATCH net-next V3 2/2] rtnl: Add support for netdev event attribute to link messages

[Vlad Yasevich]

On 04/23/2017 09:07 PM, David Ahern wrote:
> On 4/21/17 11:31 AM, Vladislav Yasevich wrote:
>> @@ -1276,9 +1277,40 @@ static int rtnl_xdp_fill(struct sk_buff *skb, struct net_device *dev)
>>  	return err;
>>  }
>>  
>> +static int rtnl_fill_link_event(struct sk_buff *skb, unsigned long event)
>> +{
>> +	u32 rtnl_event;
>> +
>> +	switch (event) {
>> +	case NETDEV_REBOOT:
>> +		rtnl_event = IFLA_EVENT_REBOOT;
>> +		break;
>> +	case NETDEV_FEAT_CHANGE:
>> +		rtnl_event = IFLA_EVENT_FEAT_CHANGE;
>> +		break;
>> +	case NETDEV_BONDING_FAILOVER:
>> +		rtnl_event = IFLA_EVENT_BONDING_FAILOVER;
>> +		break;
>> +	case NETDEV_NOTIFY_PEERS:
>> +		rtnl_event = IFLA_EVENT_NOTIFY_PEERS;
>> +		break;
>> +	case NETDEV_RESEND_IGMP:
>> +		rtnl_event = IFLA_EVENT_RESEND_IGMP;
>> +		break;
>> +	case NETDEV_CHANGEINFODATA:
>> +		rtnl_event = IFLA_EVENT_CHANGE_INFO_DATA;
>> +		break;
>> +	default:
>> +		return 0;
>> +	}
>> +
>> +	return nla_put_u32(skb, IFLA_EVENT, rtnl_event);
>> +}
>> +
> 
> I still have doubts about encoding kernel events into a uapi.
> 
> For example, NETDEV_CHANGEINFODATA is only for bonds though nothing
> about the name suggests it is a bonding notification. This one was added
> specifically to notify userspace (d4261e5650004), yet seems to happen
> only during a changelink and that already generates a RTM_NEWLINK
> message via do_setlink. Since the rtnetlink_event message does not
> contain anything "NETDEV_CHANGEINFODATA" related what purpose does it
> really serve besides duplicating netlink messages to userspace.
> 

I am not sure about this one, but if you have an app trying to monitor
for this event, it can't really since there is no info in the netlink message.

> The REBOOT, IGMP, FEAT_CHANGE and BONDING_FAILOVER seem to be unique
> messages (code analysis only) which I get for notifying userspace.
> 
> NETDEV_NOTIFY_PEERS is not so clear in how often it duplicates other
> messages.
> 

This one sometimes happens in addition to bonding failover, but not always
(it depends on bonding mode).
For me, having access to this particular event is important as it will
used to trigger a guest announcements.

-vlad

Re: [PATCH net-next V3 2/2] rtnl: Add support for netdev event attribute to link messages

[David Ahern]

On 4/27/17 1:43 PM, Vlad Yasevich wrote:
>> For example, NETDEV_CHANGEINFODATA is only for bonds though nothing
>> about the name suggests it is a bonding notification. This one was added
>> specifically to notify userspace (d4261e5650004), yet seems to happen
>> only during a changelink and that already generates a RTM_NEWLINK
>> message via do_setlink. Since the rtnetlink_event message does not
>> contain anything "NETDEV_CHANGEINFODATA" related what purpose does it
>> really serve besides duplicating netlink messages to userspace.
>>
> 
> I am not sure about this one, but if you have an app trying to monitor
> for this event, it can't really since there is no info in the netlink message.

I cc'ed Jiri on this thread hoping he would explain the intent.

I propose it gets removed.

Re: [PATCH net-next V3 2/2] rtnl: Add support for netdev event attribute to link messages

[Jiri Pirko]

Thu, Apr 27, 2017 at 09:59:38PM CEST, dsa@cumulusnetworks.com wrote:
>On 4/27/17 1:43 PM, Vlad Yasevich wrote:
>>> For example, NETDEV_CHANGEINFODATA is only for bonds though nothing
>>> about the name suggests it is a bonding notification. This one was added
>>> specifically to notify userspace (d4261e5650004), yet seems to happen
>>> only during a changelink and that already generates a RTM_NEWLINK
>>> message via do_setlink. Since the rtnetlink_event message does not
>>> contain anything "NETDEV_CHANGEINFODATA" related what purpose does it
>>> really serve besides duplicating netlink messages to userspace.
>>>
>> 
>> I am not sure about this one, but if you have an app trying to monitor
>> for this event, it can't really since there is no info in the netlink message.
>
>I cc'ed Jiri on this thread hoping he would explain the intent.
>
>I propose it gets removed.

Hmm, I don't really recall. But looking at it now, I agree it is
redundant.

Re: [PATCH net-next V3 2/2] rtnl: Add support for netdev event attribute to link messages

[Vlad Yasevich]

On 04/28/2017 12:38 PM, Jiri Pirko wrote:
> Thu, Apr 27, 2017 at 09:59:38PM CEST, dsa@cumulusnetworks.com wrote:
>> On 4/27/17 1:43 PM, Vlad Yasevich wrote:
>>>> For example, NETDEV_CHANGEINFODATA is only for bonds though nothing
>>>> about the name suggests it is a bonding notification. This one was added
>>>> specifically to notify userspace (d4261e5650004), yet seems to happen
>>>> only during a changelink and that already generates a RTM_NEWLINK
>>>> message via do_setlink. Since the rtnetlink_event message does not
>>>> contain anything "NETDEV_CHANGEINFODATA" related what purpose does it
>>>> really serve besides duplicating netlink messages to userspace.
>>>>
>>>
>>> I am not sure about this one, but if you have an app trying to monitor
>>> for this event, it can't really since there is no info in the netlink message.
>>
>> I cc'ed Jiri on this thread hoping he would explain the intent.
>>
>> I propose it gets removed.
> 
> Hmm, I don't really recall. But looking at it now, I agree it is
> redundant.
> 

So, it looks like the notifier might be there to account for the ioctl/sysfs
interfaces.

Additionally, the message is not generated from do_setlink() if the devices is
down, so notifier accounts for that as well.

I guess, basic question is whether data carried in NETDEV_CHANGEINFODATA is useful
to user space?  If yes (I can possibly see some managements apps that might be interested
in it), then we shouldn't just remove it.  Possible solutions to eliminate duplicates
would be to move the notifier call into non-rtnl code paths...

Also, may be netdev_state_change() should call rtmsg_ifinfo() unconditionally?

-vlad

Re: [PATCH net-next V3 2/2] rtnl: Add support for netdev event attribute to link messages

[David Ahern]

On 5/1/17 7:35 AM, Vlad Yasevich wrote:
> So, it looks like the notifier might be there to account for the ioctl/sysfs
> interfaces.

hmmm... I guess we have to leave it then.

> 
> Additionally, the message is not generated from do_setlink() if the devices is
> down, so notifier accounts for that as well.
> 
> I guess, basic question is whether data carried in NETDEV_CHANGEINFODATA is useful
> to user space?  If yes (I can possibly see some managements apps that might be interested
> in it), then we shouldn't just remove it.  Possible solutions to eliminate duplicates
> would be to move the notifier call into non-rtnl code paths...
> 
> Also, may be netdev_state_change() should call rtmsg_ifinfo() unconditionally?

Link up will generate a message, so userspace gets notified at some point.