[PATCHv3 net-next] rtnetlink: Honour NLM_F_ECHO flag in rtnl_{new, set, del}link

[PATCHv3 net-next] rtnetlink: Honour NLM_F_ECHO flag in rtnl_{new, set, del}link

[Hangbin Liu]

Netlink messages are used for communicating between user and kernel space.
When user space configures the kernel with netlink messages, it can set the
NLM_F_ECHO flag to request the kernel to send the applied configuration back
to the caller. This allows user space to retrieve configuration information
that are filled by the kernel (either because these parameters can only be
set by the kernel or because user space let the kernel choose a default
value).

The kernel has support this feature in some places like RTM_{NEW, DEL}ADDR,
RTM_{NEW, DEL}ROUTE. This patch handles NLM_F_ECHO flag and send link info
back after rtnl_{new, set, del}link.

Suggested-by: Guillaume Nault <gnault@redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
v3:
1) Fix group parameter in rtnl_notify.
2) Use helper rtmsg_ifinfo_build_skb() instead re-write a new one.

v2:
1) Rename rtnl_echo_link_info() to rtnl_link_notify().
2) Remove IFLA_LINK_NETNSID and IFLA_EXT_MASK, which do not fit here.
3) Add NLM_F_ECHO in rtnl_dellink. But we can't re-use the rtnl_link_notify()
   helper as we need to get the link info before rtnl_delete_link().
---
 include/linux/rtnetlink.h |  2 +-
 net/core/dev.c            |  2 +-
 net/core/rtnetlink.c      | 47 ++++++++++++++++++++++++++++++---------
 3 files changed, 38 insertions(+), 13 deletions(-)

diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
index ae2c6a3cec5d..3534701cdcc5 100644
--- a/include/linux/rtnetlink.h
+++ b/include/linux/rtnetlink.h
@@ -24,7 +24,7 @@ void rtmsg_ifinfo_newnet(int type, struct net_device *dev, unsigned int change,
 struct sk_buff *rtmsg_ifinfo_build_skb(int type, struct net_device *dev,
 				       unsigned change, u32 event,
 				       gfp_t flags, int *new_nsid,
-				       int new_ifindex);
+				       int new_ifindex, u32 pid, u32 seq);
 void rtmsg_ifinfo_send(struct sk_buff *skb, struct net_device *dev,
 		       gfp_t flags);
 
diff --git a/net/core/dev.c b/net/core/dev.c
index d66c73c1c734..fb2603bd07a9 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10862,7 +10862,7 @@ void unregister_netdevice_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, 0,
-						     GFP_KERNEL, NULL, 0);
+						     GFP_KERNEL, NULL, 0, 0, 0);
 
 		/*
 		 *	Flush the unicast and multicast chains
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 74864dc46a7e..a399b623a44f 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2651,10 +2651,13 @@ static int do_set_proto_down(struct net_device *dev,
 static int do_setlink(const struct sk_buff *skb,
 		      struct net_device *dev, struct ifinfomsg *ifm,
 		      struct netlink_ext_ack *extack,
-		      struct nlattr **tb, int status)
+		      struct nlattr **tb, int status,
+		      struct nlmsghdr *nlh)
 {
 	const struct net_device_ops *ops = dev->netdev_ops;
+	u32 pid = NETLINK_CB(skb).portid;
 	char ifname[IFNAMSIZ];
+	struct sk_buff *nskb;
 	int err;
 
 	err = validate_linkmsg(dev, tb, extack);
@@ -3009,6 +3012,11 @@ static int do_setlink(const struct sk_buff *skb,
 		}
 	}
 
+	nskb = rtmsg_ifinfo_build_skb(RTM_NEWLINK, dev, 0, 0, GFP_KERNEL, NULL,
+				      0, pid, nlh->nlmsg_seq);
+	if (nskb)
+		rtnl_notify(nskb, dev_net(dev), pid, RTNLGRP_LINK, nlh, GFP_KERNEL);
+
 errout:
 	if (status & DO_SETLINK_MODIFIED) {
 		if ((status & DO_SETLINK_NOTIFY) == DO_SETLINK_NOTIFY)
@@ -3069,7 +3077,8 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 		goto errout;
 	}
 
-	err = do_setlink(skb, dev, ifm, extack, tb, 0);
+	err = do_setlink(skb, dev, ifm, extack, tb, 0, nlh);
+
 errout:
 	return err;
 }
@@ -3130,10 +3139,12 @@ static int rtnl_dellink(struct sk_buff *skb, struct nlmsghdr *nlh,
 			struct netlink_ext_ack *extack)
 {
 	struct net *net = sock_net(skb->sk);
+	u32 pid = NETLINK_CB(skb).portid;
 	struct net *tgt_net = net;
 	struct net_device *dev = NULL;
 	struct ifinfomsg *ifm;
 	struct nlattr *tb[IFLA_MAX+1];
+	struct sk_buff *nskb;
 	int err;
 	int netnsid = -1;
 
@@ -3171,7 +3182,12 @@ static int rtnl_dellink(struct sk_buff *skb, struct nlmsghdr *nlh,
 		goto out;
 	}
 
+	nskb = rtmsg_ifinfo_build_skb(RTM_DELLINK, dev, 0, 0, GFP_KERNEL, NULL,
+				      0, pid, nlh->nlmsg_seq);
+
 	err = rtnl_delete_link(dev);
+	if (!err && nskb)
+		rtnl_notify(nskb, dev_net(dev), pid, RTNLGRP_LINK, nlh, GFP_KERNEL);
 
 out:
 	if (netnsid >= 0)
@@ -3293,14 +3309,14 @@ static int rtnl_group_changelink(const struct sk_buff *skb,
 		struct net *net, int group,
 		struct ifinfomsg *ifm,
 		struct netlink_ext_ack *extack,
-		struct nlattr **tb)
+		struct nlattr **tb, struct nlmsghdr *nlh)
 {
 	struct net_device *dev, *aux;
 	int err;
 
 	for_each_netdev_safe(net, dev, aux) {
 		if (dev->group == group) {
-			err = do_setlink(skb, dev, ifm, extack, tb, 0);
+			err = do_setlink(skb, dev, ifm, extack, tb, 0, nlh);
 			if (err < 0)
 				return err;
 		}
@@ -3312,13 +3328,16 @@ static int rtnl_group_changelink(const struct sk_buff *skb,
 static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
 			       const struct rtnl_link_ops *ops,
 			       struct nlattr **tb, struct nlattr **data,
-			       struct netlink_ext_ack *extack)
+			       struct netlink_ext_ack *extack,
+			       struct nlmsghdr *nlh)
 {
 	unsigned char name_assign_type = NET_NAME_USER;
 	struct net *net = sock_net(skb->sk);
+	u32 pid = NETLINK_CB(skb).portid;
 	struct net *dest_net, *link_net;
 	struct net_device *dev;
 	char ifname[IFNAMSIZ];
+	struct sk_buff *nskb;
 	int err;
 
 	if (!ops->alloc && !ops->setup)
@@ -3382,6 +3401,12 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
 		if (err)
 			goto out_unregister;
 	}
+
+	nskb = rtmsg_ifinfo_build_skb(RTM_NEWLINK, dev, 0, 0, GFP_KERNEL, NULL,
+				      0, pid, nlh->nlmsg_seq);
+	if (nskb)
+		rtnl_notify(nskb, dev_net(dev), pid, RTNLGRP_LINK, nlh, GFP_KERNEL);
+
 out:
 	if (link_net)
 		put_net(link_net);
@@ -3544,7 +3569,7 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 			status |= DO_SETLINK_NOTIFY;
 		}
 
-		return do_setlink(skb, dev, ifm, extack, tb, status);
+		return do_setlink(skb, dev, ifm, extack, tb, status, nlh);
 	}
 
 	if (!(nlh->nlmsg_flags & NLM_F_CREATE)) {
@@ -3556,7 +3581,7 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 		if (tb[IFLA_GROUP])
 			return rtnl_group_changelink(skb, net,
 						nla_get_u32(tb[IFLA_GROUP]),
-						ifm, extack, tb);
+						ifm, extack, tb, nlh);
 		return -ENODEV;
 	}
 
@@ -3578,7 +3603,7 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 		return -EOPNOTSUPP;
 	}
 
-	return rtnl_newlink_create(skb, ifm, ops, tb, data, extack);
+	return rtnl_newlink_create(skb, ifm, ops, tb, data, extack, nlh);
 }
 
 static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
@@ -3896,7 +3921,7 @@ 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,
 				       u32 event, gfp_t flags, int *new_nsid,
-				       int new_ifindex)
+				       int new_ifindex, u32 pid, u32 seq)
 {
 	struct net *net = dev_net(dev);
 	struct sk_buff *skb;
@@ -3907,7 +3932,7 @@ struct sk_buff *rtmsg_ifinfo_build_skb(int type, struct net_device *dev,
 		goto errout;
 
 	err = rtnl_fill_ifinfo(skb, dev, dev_net(dev),
-			       type, 0, 0, change, 0, 0, event,
+			       type, pid, seq, change, 0, 0, event,
 			       new_nsid, new_ifindex, -1, flags);
 	if (err < 0) {
 		/* -EMSGSIZE implies BUG in if_nlmsg_size() */
@@ -3939,7 +3964,7 @@ static void rtmsg_ifinfo_event(int type, struct net_device *dev,
 		return;
 
 	skb = rtmsg_ifinfo_build_skb(type, dev, change, event, flags, new_nsid,
-				     new_ifindex);
+				     new_ifindex, 0, 0);
 	if (skb)
 		rtmsg_ifinfo_send(skb, dev, flags);
 }
-- 
2.37.2

Re: [PATCHv3 net-next] rtnetlink: Honour NLM_F_ECHO flag in rtnl_{new, set, del}link

[Jakub Kicinski]

On Tue, 27 Sep 2022 12:13:03 +0800 Hangbin Liu wrote:
> @@ -3382,6 +3401,12 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
>  		if (err)
>  			goto out_unregister;
>  	}
> +
> +	nskb = rtmsg_ifinfo_build_skb(RTM_NEWLINK, dev, 0, 0, GFP_KERNEL, NULL,
> +				      0, pid, nlh->nlmsg_seq);
> +	if (nskb)
> +		rtnl_notify(nskb, dev_net(dev), pid, RTNLGRP_LINK, nlh, GFP_KERNEL);
> +
>  out:
>  	if (link_net)
>  		put_net(link_net);

I'm surprised you're adding new notifications. Does the kernel not
already notify about new links? I thought rtnl_newlink_create() ->
rtnl_configure_link() -> __dev_notify_flags() sends a notification,
already.

Re: [PATCHv3 net-next] rtnetlink: Honour NLM_F_ECHO flag in rtnl_{new, set, del}link

[Hangbin Liu]

On Tue, Sep 27, 2022 at 07:21:30AM -0700, Jakub Kicinski wrote:
> On Tue, 27 Sep 2022 12:13:03 +0800 Hangbin Liu wrote:
> > @@ -3382,6 +3401,12 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
> >  		if (err)
> >  			goto out_unregister;
> >  	}
> > +
> > +	nskb = rtmsg_ifinfo_build_skb(RTM_NEWLINK, dev, 0, 0, GFP_KERNEL, NULL,
> > +				      0, pid, nlh->nlmsg_seq);
> > +	if (nskb)
> > +		rtnl_notify(nskb, dev_net(dev), pid, RTNLGRP_LINK, nlh, GFP_KERNEL);
> > +
> >  out:
> >  	if (link_net)
> >  		put_net(link_net);
> 
> I'm surprised you're adding new notifications. Does the kernel not
> already notify about new links? I thought rtnl_newlink_create() ->
> rtnl_configure_link() -> __dev_notify_flags() sends a notification,
> already.

I think __dev_notify_flags() only sends notification when dev flag changed.
On the other hand, the notification is sent via multicast, while this patch
is intend to unicast the notification to the user space.

Thanks
Hangbin

Re: [PATCHv3 net-next] rtnetlink: Honour NLM_F_ECHO flag in rtnl_{new, set, del}link

[Hangbin Liu]

Hi Jakub,
On Tue, Sep 27, 2022 at 12:13:03PM +0800, Hangbin Liu wrote:
> @@ -3009,6 +3012,11 @@ static int do_setlink(const struct sk_buff *skb,
>  		}
>  	}
>  
> +	nskb = rtmsg_ifinfo_build_skb(RTM_NEWLINK, dev, 0, 0, GFP_KERNEL, NULL,
> +				      0, pid, nlh->nlmsg_seq);
> +	if (nskb)
> +		rtnl_notify(nskb, dev_net(dev), pid, RTNLGRP_LINK, nlh, GFP_KERNEL);

BTW, in do_setlink() I planed to use RTM_SETLINK. But I found iproute2 use
RTM_NEWLINK to set links. And I saw an old doc[1] said

"""
- RTM_SETLINK does not follow the usual rtnetlink conventions and ignores
  all netlink flags

The RTM_NEWLINK message type is a superset of RTM_SETLINK, it allows
to change both driver specific and generic attributes of the device.
"""

So I just use RTM_NEWLINK for the notification. Do you think if we should
use RTM_SETLINK?

[1] https://lwn.net/Articles/236919/

Thanks
Hangbin

Re: [PATCHv3 net-next] rtnetlink: Honour NLM_F_ECHO flag in rtnl_{new, set, del}link

[Guillaume Nault]

On Wed, Sep 28, 2022 at 10:39:49AM +0800, Hangbin Liu wrote:
> On Tue, Sep 27, 2022 at 07:21:30AM -0700, Jakub Kicinski wrote:
> > On Tue, 27 Sep 2022 12:13:03 +0800 Hangbin Liu wrote:
> > > @@ -3382,6 +3401,12 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
> > >  		if (err)
> > >  			goto out_unregister;
> > >  	}
> > > +
> > > +	nskb = rtmsg_ifinfo_build_skb(RTM_NEWLINK, dev, 0, 0, GFP_KERNEL, NULL,
> > > +				      0, pid, nlh->nlmsg_seq);
> > > +	if (nskb)
> > > +		rtnl_notify(nskb, dev_net(dev), pid, RTNLGRP_LINK, nlh, GFP_KERNEL);
> > > +
> > >  out:
> > >  	if (link_net)
> > >  		put_net(link_net);
> > 
> > I'm surprised you're adding new notifications. Does the kernel not
> > already notify about new links? I thought rtnl_newlink_create() ->
> > rtnl_configure_link() -> __dev_notify_flags() sends a notification,
> > already.
> 
> I think __dev_notify_flags() only sends notification when dev flag changed.
> On the other hand, the notification is sent via multicast, while this patch
> is intend to unicast the notification to the user space.

In rntl_configure_link(), dev->rtnl_link_state is RTNL_LINK_INITIALIZING
on device cretation, so __dev_notify_flags() is called with gchanges=~0
and notification should be always sent. It's just a matter of passing the
portid and the nlmsghdr down the call chain to make rtnl_notify() send
the unicast message together with the multicast ones.

Now for device modification, I'm not sure there's a use case for
unicast notifications. The caller already knows which values it asked
to modify, so ECHO doesn't bring much value compared to a simple ACK.

> Thanks
> Hangbin
> 

Re: [PATCHv3 net-next] rtnetlink: Honour NLM_F_ECHO flag in rtnl_{new, set, del}link

[Guillaume Nault]

On Wed, Sep 28, 2022 at 11:22:11AM +0800, Hangbin Liu wrote:
> Hi Jakub,
> On Tue, Sep 27, 2022 at 12:13:03PM +0800, Hangbin Liu wrote:
> > @@ -3009,6 +3012,11 @@ static int do_setlink(const struct sk_buff *skb,
> >  		}
> >  	}
> >  
> > +	nskb = rtmsg_ifinfo_build_skb(RTM_NEWLINK, dev, 0, 0, GFP_KERNEL, NULL,
> > +				      0, pid, nlh->nlmsg_seq);
> > +	if (nskb)
> > +		rtnl_notify(nskb, dev_net(dev), pid, RTNLGRP_LINK, nlh, GFP_KERNEL);
> 
> BTW, in do_setlink() I planed to use RTM_SETLINK. But I found iproute2 use
> RTM_NEWLINK to set links. And I saw an old doc[1] said
> 
> """
> - RTM_SETLINK does not follow the usual rtnetlink conventions and ignores
>   all netlink flags
> 
> The RTM_NEWLINK message type is a superset of RTM_SETLINK, it allows
> to change both driver specific and generic attributes of the device.
> """
> 
> So I just use RTM_NEWLINK for the notification. Do you think if we should
> use RTM_SETLINK?

This problem should go away once you switch to reusing the existing
notifications (instead of adding new ones).

> [1] https://lwn.net/Articles/236919/
> 
> Thanks
> Hangbin
> 

Re: [PATCHv3 net-next] rtnetlink: Honour NLM_F_ECHO flag in rtnl_{new, set, del}link

[Jakub Kicinski]

On Wed, 28 Sep 2022 11:22:11 +0800 Hangbin Liu wrote:
> Hi Jakub,
> On Tue, Sep 27, 2022 at 12:13:03PM +0800, Hangbin Liu wrote:
> > @@ -3009,6 +3012,11 @@ static int do_setlink(const struct sk_buff *skb,
> >  		}
> >  	}
> >  
> > +	nskb = rtmsg_ifinfo_build_skb(RTM_NEWLINK, dev, 0, 0, GFP_KERNEL, NULL,
> > +				      0, pid, nlh->nlmsg_seq);
> > +	if (nskb)
> > +		rtnl_notify(nskb, dev_net(dev), pid, RTNLGRP_LINK, nlh, GFP_KERNEL);  
> 
> BTW, in do_setlink() I planed to use RTM_SETLINK. But I found iproute2 use
> RTM_NEWLINK to set links. And I saw an old doc[1] said
> 
> """
> - RTM_SETLINK does not follow the usual rtnetlink conventions and ignores
>   all netlink flags
> 
> The RTM_NEWLINK message type is a superset of RTM_SETLINK, it allows
> to change both driver specific and generic attributes of the device.
> """

Interesting, so we actually do use this "NEW as SET" thing.

> So I just use RTM_NEWLINK for the notification. Do you think if we should
> use RTM_SETLINK?

FWIW I think it's typical for rtnl / classic netlink to generate 
"new object" notification whenever object is changed, rather than
a notification about just the change.

Re: [PATCHv3 net-next] rtnetlink: Honour NLM_F_ECHO flag in rtnl_{new, set, del}link

[Hangbin Liu]

On Wed, Sep 28, 2022 at 11:47:57AM +0200, Guillaume Nault wrote:
> On Wed, Sep 28, 2022 at 10:39:49AM +0800, Hangbin Liu wrote:
> > On Tue, Sep 27, 2022 at 07:21:30AM -0700, Jakub Kicinski wrote:
> > > On Tue, 27 Sep 2022 12:13:03 +0800 Hangbin Liu wrote:
> > > > @@ -3382,6 +3401,12 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
> > > >  		if (err)
> > > >  			goto out_unregister;
> > > >  	}
> > > > +
> > > > +	nskb = rtmsg_ifinfo_build_skb(RTM_NEWLINK, dev, 0, 0, GFP_KERNEL, NULL,
> > > > +				      0, pid, nlh->nlmsg_seq);
> > > > +	if (nskb)
> > > > +		rtnl_notify(nskb, dev_net(dev), pid, RTNLGRP_LINK, nlh, GFP_KERNEL);
> > > > +
> > > >  out:
> > > >  	if (link_net)
> > > >  		put_net(link_net);
> > > 
> > > I'm surprised you're adding new notifications. Does the kernel not
> > > already notify about new links? I thought rtnl_newlink_create() ->
> > > rtnl_configure_link() -> __dev_notify_flags() sends a notification,
> > > already.
> > 
> > I think __dev_notify_flags() only sends notification when dev flag changed.
> > On the other hand, the notification is sent via multicast, while this patch
> > is intend to unicast the notification to the user space.
> 
> In rntl_configure_link(), dev->rtnl_link_state is RTNL_LINK_INITIALIZING
> on device cretation, so __dev_notify_flags() is called with gchanges=~0
> and notification should be always sent. It's just a matter of passing the
> portid and the nlmsghdr down the call chain to make rtnl_notify() send
> the unicast message together with the multicast ones.

To update __dev_notify_flags() with nlmsghdr, we also need to update
rtnl_configure_link(), which is called by some drivers.

> 
> Now for device modification, I'm not sure there's a use case for
> unicast notifications. The caller already knows which values it asked
> to modify, so ECHO doesn't bring much value compared to a simple ACK.

And the __dev_notify_flags() is only used when the dev flag changed.

It looks no much change if we call it when create new link:
rtnl_newlink_create() -> rtnl_configure_link() -> __dev_notify_flags()

But when set link, it is only called when flag changed
do_setlink() -> dev_change_flags() -> __dev_notify_flags().

Unless you want to omit the ECHO message when setting link.

At latest, when call rtnl_delete_link(), there is no way to call
__dev_notify_flags(). So we still need to use the current way.

As a summarize, we need to change a lot of code if we use __dev_notify_flags()
to notify user, while we can only use it in one place. This looks not worth.

WDYT?

Thanks
Hangbin

Re: [PATCHv3 net-next] rtnetlink: Honour NLM_F_ECHO flag in rtnl_{new, set, del}link

[Jakub Kicinski]

On Thu, 29 Sep 2022 11:10:36 +0800 Hangbin Liu wrote:
> > Now for device modification, I'm not sure there's a use case for
> > unicast notifications. The caller already knows which values it asked
> > to modify, so ECHO doesn't bring much value compared to a simple ACK.  
> 
> And the __dev_notify_flags() is only used when the dev flag changed.
> 
> It looks no much change if we call it when create new link:
> rtnl_newlink_create() -> rtnl_configure_link() -> __dev_notify_flags()
> 
> But when set link, it is only called when flag changed
> do_setlink() -> dev_change_flags() -> __dev_notify_flags().
> 
> Unless you want to omit the ECHO message when setting link.
> 
> At latest, when call rtnl_delete_link(), there is no way to call
> __dev_notify_flags(). So we still need to use the current way.
> 
> As a summarize, we need to change a lot of code if we use __dev_notify_flags()
> to notify user, while we can only use it in one place. This looks not worth.
> 
> WDYT?

There needs to be a clear use case if you want to add notifications.
Plumbing ECHO to existing notifications is just good hygiene, if you
want to add new notifications you'd need to provide a real use case.

I don't buy the "a lot code changed" BTW, you can make
dev_change_flags() a wrapper and add dev_change_flags_nlh() or whatnot
which will take the extra argument.

Re: [PATCHv3 net-next] rtnetlink: Honour NLM_F_ECHO flag in rtnl_{new, set, del}link

[Guillaume Nault]

On Thu, Sep 29, 2022 at 11:10:36AM +0800, Hangbin Liu wrote:
> On Wed, Sep 28, 2022 at 11:47:57AM +0200, Guillaume Nault wrote:
> > On Wed, Sep 28, 2022 at 10:39:49AM +0800, Hangbin Liu wrote:
> > > On Tue, Sep 27, 2022 at 07:21:30AM -0700, Jakub Kicinski wrote:
> > > > On Tue, 27 Sep 2022 12:13:03 +0800 Hangbin Liu wrote:
> > > > > @@ -3382,6 +3401,12 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
> > > > >  		if (err)
> > > > >  			goto out_unregister;
> > > > >  	}
> > > > > +
> > > > > +	nskb = rtmsg_ifinfo_build_skb(RTM_NEWLINK, dev, 0, 0, GFP_KERNEL, NULL,
> > > > > +				      0, pid, nlh->nlmsg_seq);
> > > > > +	if (nskb)
> > > > > +		rtnl_notify(nskb, dev_net(dev), pid, RTNLGRP_LINK, nlh, GFP_KERNEL);
> > > > > +
> > > > >  out:
> > > > >  	if (link_net)
> > > > >  		put_net(link_net);
> > > > 
> > > > I'm surprised you're adding new notifications. Does the kernel not
> > > > already notify about new links? I thought rtnl_newlink_create() ->
> > > > rtnl_configure_link() -> __dev_notify_flags() sends a notification,
> > > > already.
> > > 
> > > I think __dev_notify_flags() only sends notification when dev flag changed.
> > > On the other hand, the notification is sent via multicast, while this patch
> > > is intend to unicast the notification to the user space.
> > 
> > In rntl_configure_link(), dev->rtnl_link_state is RTNL_LINK_INITIALIZING
> > on device cretation, so __dev_notify_flags() is called with gchanges=~0
> > and notification should be always sent. It's just a matter of passing the
> > portid and the nlmsghdr down the call chain to make rtnl_notify() send
> > the unicast message together with the multicast ones.
> 
> To update __dev_notify_flags() with nlmsghdr, we also need to update
> rtnl_configure_link(), which is called by some drivers.

There's just a handful of virtual net devices that call
rtnl_configure_link(). It should be easy to modify its prototype
and adjust these external callers.

> > Now for device modification, I'm not sure there's a use case for
> > unicast notifications. The caller already knows which values it asked
> > to modify, so ECHO doesn't bring much value compared to a simple ACK.
> 
> And the __dev_notify_flags() is only used when the dev flag changed.
> 
> It looks no much change if we call it when create new link:
> rtnl_newlink_create() -> rtnl_configure_link() -> __dev_notify_flags()
> 
> But when set link, it is only called when flag changed
> do_setlink() -> dev_change_flags() -> __dev_notify_flags().
> 
> Unless you want to omit the ECHO message when setting link.

For SET operations, we may send one, many or even zero notifications.
I agree with Jackub that we shouldn't add specific notifications just
for NLM_F_ECHO. That means, either we echo all the existing
notifications (or none if the device hasn't been modified), or we
leave NLM_F_ECHO unimplemented for SET operations.

As I said in my previous reply, I can't see any use case for ECHO on
SET operations, so I don't mind if we skip it.

> At latest, when call rtnl_delete_link(), there is no way to call
> __dev_notify_flags(). So we still need to use the current way.

Not everything has to be done with __dev_notify_flags(). For DEL
operations notifications are sent in netdev_unregister_many().
Given the overwhelming number of callers, modifying its prototype isn't
going to be practical, but you can use a wrapper.

Something like (very rough idea):

-void unregister_netdevice_many(struct list_head *head)
+void unregister_netdevice_many_notify(struct list_head *head, u32 portid,
+                                      const struct nlmsghdr *nlh)
 {
[...]
                 if (!dev->rtnl_link_ops ||
                     dev->rtnl_link_state == RTNL_LINK_INITIALIZED)
-                        skb = rtmsg_ifinfo_build_skb(RTM_DELLINK, dev, ~0U, 0,
-                                                     GFP_KERNEL, NULL, 0);
+                        skb = rtmsg_ifinfo_build_skb(RTM_DELLINK, portid,
+                                                     nlh->nlmsg_seq, dev, ~0U,
+                                                     0, GFP_KERNEL, NULL, 0);
 
[...]
                if (skb)
-                        rtmsg_ifinfo_send(skb, dev, GFP_KERNEL);
+                        rtmsg_ifinfo_send(skb, portid, dev, nlh, GFP_KERNEL);
 
[...]
 }
 
+void unregister_netdevice_many(struct list_head *head)
+{
+        unregister_netdevice_many_notify(head, 0, NULL);
+}

> As a summarize, we need to change a lot of code if we use __dev_notify_flags()
> to notify user, while we can only use it in one place. This looks not worth.
> 
> WDYT?

Using __dev_notify_flags() is only for NEW operations. SET and DEL have
to be handled differently.

To summarise, the objective is to reuse the existing notifications.
In practice that means just doing the required plumbing to pass the
correct parameters to the existing nlmsg_notify() calls (and ensure we
set the correct portid and sequence number in the netlink message
header).

> Thanks
> Hangbin
>