[PATCHv5 net-next 0/4] rtnetlink: Honour NLM_F_ECHO flag in rtnl_{new, del}link

[PATCHv5 net-next 0/4] rtnetlink: Honour NLM_F_ECHO flag in rtnl_{new, 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 set handles NLM_F_ECHO flag and send link
info back after rtnl_{new, del}link.

v5:
1) make rtnl_configure_link_notify static, reported by kernel test robot

v4:
1) Add rtnl_configure_link_notify() helper so rtnl_newlink_create could
   use it instead of creating new notify function.
2) Add unregister_netdevice_many_notify() helper so rtnl_delete_link()
   could use it instead of creating new notify function
3) Split the previous patch to 4 small patches for easier reviewing.

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

Hangbin Liu (4):
  rtnetlink: add new helper rtnl_configure_link_notify()
  net: add new helper unregister_netdevice_many_notify
  rtnetlink: Honour NLM_F_ECHO flag in rtnl_newlink_create
  rtnetlink: Honour NLM_F_ECHO flag in rtnl_delete_link

 include/linux/netdevice.h      |  4 ++-
 include/linux/rtnetlink.h      |  6 ++--
 include/net/rtnetlink.h        |  2 +-
 net/core/dev.c                 | 26 +++++++++++-----
 net/core/rtnetlink.c           | 56 ++++++++++++++++++++++------------
 net/openvswitch/vport-geneve.c |  2 +-
 net/openvswitch/vport-gre.c    |  2 +-
 net/openvswitch/vport-netdev.c |  2 +-
 net/openvswitch/vport-vxlan.c  |  2 +-
 9 files changed, 67 insertions(+), 35 deletions(-)

-- 
2.37.2

[PATCHv5 net-next 1/4] rtnetlink: add new helper rtnl_configure_link_notify()

[Hangbin Liu]

This patch update rtnl_configure_link() to rtnl_configure_link_nlh() by
adding parameter netlink message header and port id so we can notify the
userspace about the new link info if NLM_F_ECHO flag is set.

The rtnl_configure_link() will be a wrapper of the new function. The new
call chain looks like:

- rtnl_configure_link_notify()
  - __dev_notify_flags()
    - rtmsg_ifinfo_nlh()
      - rtmsg_ifinfo_event()
        - rtmsg_ifinfo_build_skb()
        - rtmsg_ifinfo_send()

All the functions in this call chain will add parameter nlh and pid, so
we can use them in the last call rtnl_notify().

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 include/linux/netdevice.h |  2 +-
 include/linux/rtnetlink.h |  6 ++++--
 net/core/dev.c            | 14 ++++++-------
 net/core/rtnetlink.c      | 41 ++++++++++++++++++++++++++-------------
 4 files changed, 40 insertions(+), 23 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index eddf8ee270e7..a71d378945e3 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3856,7 +3856,7 @@ int __dev_change_flags(struct net_device *dev, unsigned int flags,
 int dev_change_flags(struct net_device *dev, unsigned int flags,
 		     struct netlink_ext_ack *extack);
 void __dev_notify_flags(struct net_device *, unsigned int old_flags,
-			unsigned int gchanges);
+			unsigned int gchanges, u32 pid, struct nlmsghdr *nlh);
 int dev_set_alias(struct net_device *, const char *, size_t);
 int dev_get_alias(const struct net_device *, char *, size_t);
 int __dev_change_net_namespace(struct net_device *dev, struct net *net,
diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
index ae2c6a3cec5d..ef703b4484a7 100644
--- a/include/linux/rtnetlink.h
+++ b/include/linux/rtnetlink.h
@@ -21,12 +21,14 @@ 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);
 void rtmsg_ifinfo_newnet(int type, struct net_device *dev, unsigned int change,
 			 gfp_t flags, int *new_nsid, int new_ifindex);
+void rtmsg_ifinfo_nlh(int type, struct net_device *dev, unsigned int change,
+		      gfp_t flags, u32 pid, struct nlmsghdr *nlh);
 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);
+		       gfp_t flags, u32 pid, struct nlmsghdr *nlh);
 
 
 /* RTNL is used as a global lock for all changes to network configuration  */
diff --git a/net/core/dev.c b/net/core/dev.c
index fa53830d0683..89cf082317dd 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8347,7 +8347,7 @@ static int __dev_set_promiscuity(struct net_device *dev, int inc, bool notify)
 		dev_change_rx_flags(dev, IFF_PROMISC);
 	}
 	if (notify)
-		__dev_notify_flags(dev, old_flags, IFF_PROMISC);
+		__dev_notify_flags(dev, old_flags, IFF_PROMISC, 0, NULL);
 	return 0;
 }
 
@@ -8402,7 +8402,7 @@ static int __dev_set_allmulti(struct net_device *dev, int inc, bool notify)
 		dev_set_rx_mode(dev);
 		if (notify)
 			__dev_notify_flags(dev, old_flags,
-					   dev->gflags ^ old_gflags);
+					   dev->gflags ^ old_gflags, 0, NULL);
 	}
 	return 0;
 }
@@ -8565,12 +8565,12 @@ int __dev_change_flags(struct net_device *dev, unsigned int flags,
 }
 
 void __dev_notify_flags(struct net_device *dev, unsigned int old_flags,
-			unsigned int gchanges)
+			unsigned int gchanges, u32 pid, struct nlmsghdr *nlh)
 {
 	unsigned int changes = dev->flags ^ old_flags;
 
 	if (gchanges)
-		rtmsg_ifinfo(RTM_NEWLINK, dev, gchanges, GFP_ATOMIC);
+		rtmsg_ifinfo_nlh(RTM_NEWLINK, dev, gchanges, GFP_ATOMIC, pid, nlh);
 
 	if (changes & IFF_UP) {
 		if (dev->flags & IFF_UP)
@@ -8612,7 +8612,7 @@ int dev_change_flags(struct net_device *dev, unsigned int flags,
 		return ret;
 
 	changes = (old_flags ^ dev->flags) | (old_gflags ^ dev->gflags);
-	__dev_notify_flags(dev, old_flags, changes);
+	__dev_notify_flags(dev, old_flags, changes, 0, NULL);
 	return ret;
 }
 EXPORT_SYMBOL(dev_change_flags);
@@ -10845,7 +10845,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
@@ -10860,7 +10860,7 @@ void unregister_netdevice_many(struct list_head *head)
 			dev->netdev_ops->ndo_uninit(dev);
 
 		if (skb)
-			rtmsg_ifinfo_send(skb, dev, GFP_KERNEL);
+			rtmsg_ifinfo_send(skb, dev, GFP_KERNEL, 0, NULL);
 
 		/* Notifier chain MUST detach us all upper devices. */
 		WARN_ON(netdev_has_any_upper_dev(dev));
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 74864dc46a7e..1558921bd4da 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3180,7 +3180,8 @@ static int rtnl_dellink(struct sk_buff *skb, struct nlmsghdr *nlh,
 	return err;
 }
 
-int rtnl_configure_link(struct net_device *dev, const struct ifinfomsg *ifm)
+static int rtnl_configure_link_notify(struct net_device *dev, const struct ifinfomsg *ifm,
+				      struct nlmsghdr *nlh, u32 pid)
 {
 	unsigned int old_flags;
 	int err;
@@ -3194,13 +3195,18 @@ int rtnl_configure_link(struct net_device *dev, const struct ifinfomsg *ifm)
 	}
 
 	if (dev->rtnl_link_state == RTNL_LINK_INITIALIZED) {
-		__dev_notify_flags(dev, old_flags, (old_flags ^ dev->flags));
+		__dev_notify_flags(dev, old_flags, (old_flags ^ dev->flags), pid, nlh);
 	} else {
 		dev->rtnl_link_state = RTNL_LINK_INITIALIZED;
-		__dev_notify_flags(dev, old_flags, ~0U);
+		__dev_notify_flags(dev, old_flags, ~0U, pid, nlh);
 	}
 	return 0;
 }
+
+int rtnl_configure_link(struct net_device *dev, const struct ifinfomsg *ifm)
+{
+	return rtnl_configure_link_notify(dev, ifm, NULL, 0);
+}
 EXPORT_SYMBOL(rtnl_configure_link);
 
 struct net_device *rtnl_create_link(struct net *net, const char *ifname,
@@ -3896,7 +3902,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 +3913,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() */
@@ -3922,16 +3928,18 @@ struct sk_buff *rtmsg_ifinfo_build_skb(int type, struct net_device *dev,
 	return NULL;
 }
 
-void rtmsg_ifinfo_send(struct sk_buff *skb, struct net_device *dev, gfp_t flags)
+void rtmsg_ifinfo_send(struct sk_buff *skb, struct net_device *dev, gfp_t flags,
+		       u32 pid, struct nlmsghdr *nlh)
 {
 	struct net *net = dev_net(dev);
 
-	rtnl_notify(skb, net, 0, RTNLGRP_LINK, NULL, flags);
+	rtnl_notify(skb, net, pid, RTNLGRP_LINK, nlh, flags);
 }
 
 static void rtmsg_ifinfo_event(int type, struct net_device *dev,
 			       unsigned int change, u32 event,
-			       gfp_t flags, int *new_nsid, int new_ifindex)
+			       gfp_t flags, int *new_nsid, int new_ifindex,
+			       u32 pid, struct nlmsghdr *nlh)
 {
 	struct sk_buff *skb;
 
@@ -3939,23 +3947,30 @@ 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, pid, nlh ? nlh->nlmsg_seq : 0);
 	if (skb)
-		rtmsg_ifinfo_send(skb, dev, flags);
+		rtmsg_ifinfo_send(skb, dev, flags, pid, nlh);
 }
 
 void rtmsg_ifinfo(int type, struct net_device *dev, unsigned int change,
 		  gfp_t flags)
 {
 	rtmsg_ifinfo_event(type, dev, change, rtnl_get_event(0), flags,
-			   NULL, 0);
+			   NULL, 0, 0, NULL);
 }
 
 void rtmsg_ifinfo_newnet(int type, struct net_device *dev, unsigned int change,
 			 gfp_t flags, int *new_nsid, int new_ifindex)
 {
 	rtmsg_ifinfo_event(type, dev, change, rtnl_get_event(0), flags,
-			   new_nsid, new_ifindex);
+			   new_nsid, new_ifindex, 0, NULL);
+}
+
+void rtmsg_ifinfo_nlh(int type, struct net_device *dev, unsigned int change,
+		      gfp_t flags, u32 pid, struct nlmsghdr *nlh)
+{
+	rtmsg_ifinfo_event(type, dev, change, rtnl_get_event(0), flags,
+			   NULL, 0, pid, nlh);
 }
 
 static int nlmsg_populate_fdb_fill(struct sk_buff *skb,
@@ -6140,7 +6155,7 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi
 	case NETDEV_CHANGELOWERSTATE:
 	case NETDEV_CHANGE_TX_QUEUE_LEN:
 		rtmsg_ifinfo_event(RTM_NEWLINK, dev, 0, rtnl_get_event(event),
-				   GFP_KERNEL, NULL, 0);
+				   GFP_KERNEL, NULL, 0, 0, NULL);
 		break;
 	default:
 		break;
-- 
2.37.2

[PATCHv5 net-next 2/4] net: add new helper unregister_netdevice_many_notify

[Hangbin Liu]

Add new helper unregister_netdevice_many_notify(), pass netlink message
header and port id, which could be used to notify userspace when flag
NLM_F_ECHO is set.

Make the unregister_netdevice_many() as a wrapper of new function
unregister_netdevice_many_notify().

Suggested-by: Guillaume Nault <gnault@redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 include/linux/netdevice.h |  2 ++
 net/core/dev.c            | 16 +++++++++++++---
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index a71d378945e3..150d7e90b2fc 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3026,6 +3026,8 @@ static inline int dev_direct_xmit(struct sk_buff *skb, u16 queue_id)
 
 int register_netdevice(struct net_device *dev);
 void unregister_netdevice_queue(struct net_device *dev, struct list_head *head);
+void unregister_netdevice_many_notify(struct list_head *head,
+				      struct nlmsghdr *nlh, u32 pid);
 void unregister_netdevice_many(struct list_head *head);
 static inline void unregister_netdevice(struct net_device *dev)
 {
diff --git a/net/core/dev.c b/net/core/dev.c
index 89cf082317dd..7e625b37880f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10779,11 +10779,14 @@ EXPORT_SYMBOL(unregister_netdevice_queue);
 /**
  *	unregister_netdevice_many - unregister many devices
  *	@head: list of devices
+ *	@nlh: netlink message header
+ *	@pid: destination netlink portid for reports
  *
  *  Note: As most callers use a stack allocated list_head,
  *  we force a list_del() to make sure stack wont be corrupted later.
  */
-void unregister_netdevice_many(struct list_head *head)
+void unregister_netdevice_many_notify(struct list_head *head,
+				      struct nlmsghdr *nlh, u32 pid)
 {
 	struct net_device *dev, *tmp;
 	LIST_HEAD(close_head);
@@ -10845,7 +10848,8 @@ 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, 0, 0);
+						     GFP_KERNEL, NULL, 0,
+						     pid, nlh ? nlh->nlmsg_seq : 0);
 
 		/*
 		 *	Flush the unicast and multicast chains
@@ -10860,7 +10864,7 @@ void unregister_netdevice_many(struct list_head *head)
 			dev->netdev_ops->ndo_uninit(dev);
 
 		if (skb)
-			rtmsg_ifinfo_send(skb, dev, GFP_KERNEL, 0, NULL);
+			rtmsg_ifinfo_send(skb, dev, GFP_KERNEL, pid, nlh);
 
 		/* Notifier chain MUST detach us all upper devices. */
 		WARN_ON(netdev_has_any_upper_dev(dev));
@@ -10883,6 +10887,12 @@ void unregister_netdevice_many(struct list_head *head)
 
 	list_del(head);
 }
+EXPORT_SYMBOL(unregister_netdevice_many_notify);
+
+void unregister_netdevice_many(struct list_head *head)
+{
+	unregister_netdevice_many_notify(head, NULL, 0);
+}
 EXPORT_SYMBOL(unregister_netdevice_many);
 
 /**
-- 
2.37.2

[PATCHv5 net-next 3/4] rtnetlink: Honour NLM_F_ECHO flag in rtnl_newlink_create

[Hangbin Liu]

This patch use the new helper rtnl_configure_link_notify() for
rtnl_newlink_create(), so that the kernel could reply unicast
when userspace set NLM_F_ECHO flag to request the new created
interface info.

Suggested-by: Guillaume Nault <gnault@redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 net/core/rtnetlink.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 1558921bd4da..da9a6fd156d8 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3318,10 +3318,12 @@ 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];
@@ -3375,7 +3377,7 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
 		goto out;
 	}
 
-	err = rtnl_configure_link(dev, ifm);
+	err = rtnl_configure_link_notify(dev, ifm, nlh, pid);
 	if (err < 0)
 		goto out_unregister;
 	if (link_net) {
@@ -3584,7 +3586,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,
-- 
2.37.2

[PATCHv5 net-next 4/4] rtnetlink: Honour NLM_F_ECHO flag in rtnl_delete_link

[Hangbin Liu]

This patch use the new helper unregister_netdevice_many_notify() for
rtnl_delete_link(), so that the kernel could reply unicast when userspace
 set NLM_F_ECHO flag to request the new created interface info.

At the same time, the parameters of rtnl_delete_link() need to be updated
since we need nlmsghdr and pid info.

Suggested-by: Guillaume Nault <gnault@redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 include/net/rtnetlink.h        | 2 +-
 net/core/rtnetlink.c           | 7 ++++---
 net/openvswitch/vport-geneve.c | 2 +-
 net/openvswitch/vport-gre.c    | 2 +-
 net/openvswitch/vport-netdev.c | 2 +-
 net/openvswitch/vport-vxlan.c  | 2 +-
 6 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index bf8bb3357825..1a152993caef 100644
--- a/include/net/rtnetlink.h
+++ b/include/net/rtnetlink.h
@@ -186,7 +186,7 @@ struct net_device *rtnl_create_link(struct net *net, const char *ifname,
 				    const struct rtnl_link_ops *ops,
 				    struct nlattr *tb[],
 				    struct netlink_ext_ack *extack);
-int rtnl_delete_link(struct net_device *dev);
+int rtnl_delete_link(struct net_device *dev, struct nlmsghdr *nlh, u32 pid);
 int rtnl_configure_link(struct net_device *dev, const struct ifinfomsg *ifm);
 
 int rtnl_nla_parse_ifla(struct nlattr **tb, const struct nlattr *head, int len,
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index da9a6fd156d8..3144ec7324b9 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3110,7 +3110,7 @@ static int rtnl_group_dellink(const struct net *net, int group)
 	return 0;
 }
 
-int rtnl_delete_link(struct net_device *dev)
+int rtnl_delete_link(struct net_device *dev, struct nlmsghdr *nlh, u32 pid)
 {
 	const struct rtnl_link_ops *ops;
 	LIST_HEAD(list_kill);
@@ -3120,7 +3120,7 @@ int rtnl_delete_link(struct net_device *dev)
 		return -EOPNOTSUPP;
 
 	ops->dellink(dev, &list_kill);
-	unregister_netdevice_many(&list_kill);
+	unregister_netdevice_many_notify(&list_kill, nlh, pid);
 
 	return 0;
 }
@@ -3130,6 +3130,7 @@ 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;
@@ -3171,7 +3172,7 @@ static int rtnl_dellink(struct sk_buff *skb, struct nlmsghdr *nlh,
 		goto out;
 	}
 
-	err = rtnl_delete_link(dev);
+	err = rtnl_delete_link(dev, nlh, pid);
 
 out:
 	if (netnsid >= 0)
diff --git a/net/openvswitch/vport-geneve.c b/net/openvswitch/vport-geneve.c
index 89a8e1501809..0e11ff8ee5ce 100644
--- a/net/openvswitch/vport-geneve.c
+++ b/net/openvswitch/vport-geneve.c
@@ -91,7 +91,7 @@ static struct vport *geneve_tnl_create(const struct vport_parms *parms)
 
 	err = dev_change_flags(dev, dev->flags | IFF_UP, NULL);
 	if (err < 0) {
-		rtnl_delete_link(dev);
+		rtnl_delete_link(dev, NULL, 0);
 		rtnl_unlock();
 		ovs_vport_free(vport);
 		goto error;
diff --git a/net/openvswitch/vport-gre.c b/net/openvswitch/vport-gre.c
index e6b5e76a962a..3a299383fca0 100644
--- a/net/openvswitch/vport-gre.c
+++ b/net/openvswitch/vport-gre.c
@@ -57,7 +57,7 @@ static struct vport *gre_tnl_create(const struct vport_parms *parms)
 
 	err = dev_change_flags(dev, dev->flags | IFF_UP, NULL);
 	if (err < 0) {
-		rtnl_delete_link(dev);
+		rtnl_delete_link(dev, NULL, 0);
 		rtnl_unlock();
 		ovs_vport_free(vport);
 		return ERR_PTR(err);
diff --git a/net/openvswitch/vport-netdev.c b/net/openvswitch/vport-netdev.c
index 2f61d5bdce1a..1bead7854593 100644
--- a/net/openvswitch/vport-netdev.c
+++ b/net/openvswitch/vport-netdev.c
@@ -172,7 +172,7 @@ void ovs_netdev_tunnel_destroy(struct vport *vport)
 	 * if it's not already shutting down.
 	 */
 	if (vport->dev->reg_state == NETREG_REGISTERED)
-		rtnl_delete_link(vport->dev);
+		rtnl_delete_link(vport->dev, NULL, 0);
 	netdev_put(vport->dev, &vport->dev_tracker);
 	vport->dev = NULL;
 	rtnl_unlock();
diff --git a/net/openvswitch/vport-vxlan.c b/net/openvswitch/vport-vxlan.c
index 188e9c1360a1..dae8eb1a6e7a 100644
--- a/net/openvswitch/vport-vxlan.c
+++ b/net/openvswitch/vport-vxlan.c
@@ -120,7 +120,7 @@ static struct vport *vxlan_tnl_create(const struct vport_parms *parms)
 
 	err = dev_change_flags(dev, dev->flags | IFF_UP, NULL);
 	if (err < 0) {
-		rtnl_delete_link(dev);
+		rtnl_delete_link(dev, NULL, 0);
 		rtnl_unlock();
 		ovs_vport_free(vport);
 		goto error;
-- 
2.37.2

Re: [PATCHv5 net-next 1/4] rtnetlink: add new helper rtnl_configure_link_notify()

[Nicolas Dichtel]

Le 30/09/2022 à 11:45, Hangbin Liu a écrit :
> This patch update rtnl_configure_link() to rtnl_configure_link_nlh() by
> adding parameter netlink message header and port id so we can notify the
> userspace about the new link info if NLM_F_ECHO flag is set.
> 
> The rtnl_configure_link() will be a wrapper of the new function. The new
> call chain looks like:
> 
> - rtnl_configure_link_notify()
>   - __dev_notify_flags()
>     - rtmsg_ifinfo_nlh()
>       - rtmsg_ifinfo_event()
>         - rtmsg_ifinfo_build_skb()
>         - rtmsg_ifinfo_send()
> 
> All the functions in this call chain will add parameter nlh and pid, so
> we can use them in the last call rtnl_notify().
> 
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  include/linux/netdevice.h |  2 +-
>  include/linux/rtnetlink.h |  6 ++++--
>  net/core/dev.c            | 14 ++++++-------
>  net/core/rtnetlink.c      | 41 ++++++++++++++++++++++++++-------------
>  4 files changed, 40 insertions(+), 23 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index eddf8ee270e7..a71d378945e3 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -3856,7 +3856,7 @@ int __dev_change_flags(struct net_device *dev, unsigned int flags,
>  int dev_change_flags(struct net_device *dev, unsigned int flags,
>  		     struct netlink_ext_ack *extack);
>  void __dev_notify_flags(struct net_device *, unsigned int old_flags,
> -			unsigned int gchanges);
> +			unsigned int gchanges, u32 pid, struct nlmsghdr *nlh);
Order is pid/nlh.

[snip]

> +void rtmsg_ifinfo_nlh(int type, struct net_device *dev, unsigned int change,
> +		      gfp_t flags, u32 pid, struct nlmsghdr *nlh);
Same here.

>  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);
Same here.

>  void rtmsg_ifinfo_send(struct sk_buff *skb, struct net_device *dev,
> -		       gfp_t flags);
> +		       gfp_t flags, u32 pid, struct nlmsghdr *nlh);
Same here.

[snip]


> -int rtnl_configure_link(struct net_device *dev, const struct ifinfomsg *ifm)
> +static int rtnl_configure_link_notify(struct net_device *dev, const struct ifinfomsg *ifm,
> +				      struct nlmsghdr *nlh, u32 pid)
But not here. Following patches also use this order instead of the previous one.
For consistency, it could be good to keep the same order everywhere.


Regards,
Nicolas

Re: [PATCHv5 net-next 2/4] net: add new helper unregister_netdevice_many_notify

[Nicolas Dichtel]

Le 30/09/2022 à 11:45, Hangbin Liu a écrit :
> Add new helper unregister_netdevice_many_notify(), pass netlink message
> header and port id, which could be used to notify userspace when flag
> NLM_F_ECHO is set.
> 
> Make the unregister_netdevice_many() as a wrapper of new function
> unregister_netdevice_many_notify().
> 
> Suggested-by: Guillaume Nault <gnault@redhat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

[snip]

> @@ -10860,7 +10864,7 @@ void unregister_netdevice_many(struct list_head *head)
>  			dev->netdev_ops->ndo_uninit(dev);
>  
>  		if (skb)
> -			rtmsg_ifinfo_send(skb, dev, GFP_KERNEL, 0, NULL);
> +			rtmsg_ifinfo_send(skb, dev, GFP_KERNEL, pid, nlh);
>  
>  		/* Notifier chain MUST detach us all upper devices. */
>  		WARN_ON(netdev_has_any_upper_dev(dev));
> @@ -10883,6 +10887,12 @@ void unregister_netdevice_many(struct list_head *head)
>  
>  	list_del(head);
>  }
> +EXPORT_SYMBOL(unregister_netdevice_many_notify);
Is this export really needed?

> +
> +void unregister_netdevice_many(struct list_head *head)
> +{
> +	unregister_netdevice_many_notify(head, NULL, 0);
> +}
>  EXPORT_SYMBOL(unregister_netdevice_many);
>  
>  /**

Re: [PATCHv5 net-next 2/4] net: add new helper unregister_netdevice_many_notify

[Guillaume Nault]

On Fri, Sep 30, 2022 at 04:23:07PM +0200, Nicolas Dichtel wrote:
> Le 30/09/2022 à 11:45, Hangbin Liu a écrit :
> > Add new helper unregister_netdevice_many_notify(), pass netlink message
> > header and port id, which could be used to notify userspace when flag
> > NLM_F_ECHO is set.
> > 
> > Make the unregister_netdevice_many() as a wrapper of new function
> > unregister_netdevice_many_notify().
> > 
> > Suggested-by: Guillaume Nault <gnault@redhat.com>
> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> 
> [snip]
> 
> > @@ -10860,7 +10864,7 @@ void unregister_netdevice_many(struct list_head *head)
> >  			dev->netdev_ops->ndo_uninit(dev);
> >  
> >  		if (skb)
> > -			rtmsg_ifinfo_send(skb, dev, GFP_KERNEL, 0, NULL);
> > +			rtmsg_ifinfo_send(skb, dev, GFP_KERNEL, pid, nlh);
> >  
> >  		/* Notifier chain MUST detach us all upper devices. */
> >  		WARN_ON(netdev_has_any_upper_dev(dev));
> > @@ -10883,6 +10887,12 @@ void unregister_netdevice_many(struct list_head *head)
> >  
> >  	list_del(head);
> >  }
> > +EXPORT_SYMBOL(unregister_netdevice_many_notify);
> Is this export really needed?

I was about to make the same comment :). I see no reason to export this
function. Declaring it in net/core/dev.h should be enough.

> > +void unregister_netdevice_many(struct list_head *head)
> > +{
> > +	unregister_netdevice_many_notify(head, NULL, 0);
> > +}
> >  EXPORT_SYMBOL(unregister_netdevice_many);
> >  
> >  /**
> 

Re: [PATCHv5 net-next 2/4] net: add new helper unregister_netdevice_many_notify

[Jakub Kicinski]

On Fri, 30 Sep 2022 16:29:10 +0200 Guillaume Nault wrote:
> Declaring it in net/core/dev.h should be enough.

Yes, please. And while you're touching __dev_notify_flags()
its declaration can be moved to dev.h, too.

Re: [PATCHv5 net-next 1/4] rtnetlink: add new helper rtnl_configure_link_notify()

[Guillaume Nault]

On Fri, Sep 30, 2022 at 04:22:19PM +0200, Nicolas Dichtel wrote:
> Le 30/09/2022 à 11:45, Hangbin Liu a écrit :
> > -int rtnl_configure_link(struct net_device *dev, const struct ifinfomsg *ifm)
> > +static int rtnl_configure_link_notify(struct net_device *dev, const struct ifinfomsg *ifm,
> > +				      struct nlmsghdr *nlh, u32 pid)
> But not here. Following patches also use this order instead of the previous one.
> For consistency, it could be good to keep the same order everywhere.

Yes, since a v6 will be necessary anyway, let's be consistent about the
order of parameters. That helps reading the code.

While there, I'd prefer to use 'portid' instead of 'pid'. I know
rtnetlink.c uses both, but 'portid' is more explicit and is what
af_netlink.c generally uses.

Re: [PATCHv5 net-next 1/4] rtnetlink: add new helper rtnl_configure_link_notify()

[Guillaume Nault]

> @@ -3856,7 +3856,7 @@ int __dev_change_flags(struct net_device *dev, unsigned int flags,
>  int dev_change_flags(struct net_device *dev, unsigned int flags,
>  		     struct netlink_ext_ack *extack);
>  void __dev_notify_flags(struct net_device *, unsigned int old_flags,
> -			unsigned int gchanges);
> +			unsigned int gchanges, u32 pid, struct nlmsghdr *nlh);

In all the modified functions, you could make struct nlmsghdr * a const
pointer. You just need to also update rtnl_notify() to make its nlh
parameter const too.

> +void rtmsg_ifinfo_nlh(int type, struct net_device *dev, unsigned int change,
> +		      gfp_t flags, u32 pid, struct nlmsghdr *nlh)
> +{
> +	rtmsg_ifinfo_event(type, dev, change, rtnl_get_event(0), flags,
> +			   NULL, 0, pid, nlh);
>  }

Can't we just add the extra parameters to rtmsg_ifinfo() and trivially
adapt the few users to the new prototype? Maybe that's a personal
taste, but I find such wrapper unnecessary.

Re: [PATCHv5 net-next 2/4] net: add new helper unregister_netdevice_many_notify

[Guillaume Nault]

On Fri, Sep 30, 2022 at 05:45:04PM +0800, Hangbin Liu wrote:
> @@ -10779,11 +10779,14 @@ EXPORT_SYMBOL(unregister_netdevice_queue);
>  /**
>   *	unregister_netdevice_many - unregister many devices
>   *	@head: list of devices
> + *	@nlh: netlink message header
> + *	@pid: destination netlink portid for reports
>   *
>   *  Note: As most callers use a stack allocated list_head,
>   *  we force a list_del() to make sure stack wont be corrupted later.
>   */
> -void unregister_netdevice_many(struct list_head *head)
> +void unregister_netdevice_many_notify(struct list_head *head,
> +				      struct nlmsghdr *nlh, u32 pid)

Let's use portid (not pid) here too.

Re: [PATCHv5 net-next 3/4] rtnetlink: Honour NLM_F_ECHO flag in rtnl_newlink_create

[Guillaume Nault]

On Fri, Sep 30, 2022 at 05:45:05PM +0800, Hangbin Liu wrote:
> This patch use the new helper rtnl_configure_link_notify() for
> rtnl_newlink_create(), so that the kernel could reply unicast
> when userspace set NLM_F_ECHO flag to request the new created
> interface info.
> 
> Suggested-by: Guillaume Nault <gnault@redhat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  net/core/rtnetlink.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 1558921bd4da..da9a6fd156d8 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -3318,10 +3318,12 @@ 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)

'nlh' could be const here too. Also, since we've started being picky
about the order of parameters, let's put 'nlh' right before 'tb' to
follow what other functions generally do.

>  {
>  	unsigned char name_assign_type = NET_NAME_USER;
>  	struct net *net = sock_net(skb->sk);
> +	u32 pid = NETLINK_CB(skb).portid;

s/pid/portid/

Re: [PATCHv5 net-next 2/4] net: add new helper unregister_netdevice_many_notify

[Guillaume Nault]

On Fri, Sep 30, 2022 at 06:31:42PM +0200, Guillaume Nault wrote:
> On Fri, Sep 30, 2022 at 05:45:04PM +0800, Hangbin Liu wrote:
> > @@ -10779,11 +10779,14 @@ EXPORT_SYMBOL(unregister_netdevice_queue);
> >  /**
> >   *	unregister_netdevice_many - unregister many devices
> >   *	@head: list of devices
> > + *	@nlh: netlink message header
> > + *	@pid: destination netlink portid for reports
> >   *
> >   *  Note: As most callers use a stack allocated list_head,
> >   *  we force a list_del() to make sure stack wont be corrupted later.
> >   */
> > -void unregister_netdevice_many(struct list_head *head)
> > +void unregister_netdevice_many_notify(struct list_head *head,
> > +				      struct nlmsghdr *nlh, u32 pid)
> 
> Let's use portid (not pid) here too.

...and make 'nlh' const.

Re: [PATCHv5 net-next 4/4] rtnetlink: Honour NLM_F_ECHO flag in rtnl_delete_link

[Guillaume Nault]

On Fri, Sep 30, 2022 at 05:45:06PM +0800, Hangbin Liu wrote:
> This patch use the new helper unregister_netdevice_many_notify() for
> rtnl_delete_link(), so that the kernel could reply unicast when userspace
>  set NLM_F_ECHO flag to request the new created interface info.
> 
> At the same time, the parameters of rtnl_delete_link() need to be updated
> since we need nlmsghdr and pid info.
> 
> Suggested-by: Guillaume Nault <gnault@redhat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  include/net/rtnetlink.h        | 2 +-
>  net/core/rtnetlink.c           | 7 ++++---
>  net/openvswitch/vport-geneve.c | 2 +-
>  net/openvswitch/vport-gre.c    | 2 +-
>  net/openvswitch/vport-netdev.c | 2 +-
>  net/openvswitch/vport-vxlan.c  | 2 +-
>  6 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
> index bf8bb3357825..1a152993caef 100644
> --- a/include/net/rtnetlink.h
> +++ b/include/net/rtnetlink.h
> @@ -186,7 +186,7 @@ struct net_device *rtnl_create_link(struct net *net, const char *ifname,
>  				    const struct rtnl_link_ops *ops,
>  				    struct nlattr *tb[],
>  				    struct netlink_ext_ack *extack);
> -int rtnl_delete_link(struct net_device *dev);
> +int rtnl_delete_link(struct net_device *dev, struct nlmsghdr *nlh, u32 pid);
>  int rtnl_configure_link(struct net_device *dev, const struct ifinfomsg *ifm);

You didn't add a wrapper for rtnl_delete_link() here and modified the
callers instead, which makes me think you could just also have modified
rtnl_configure_link() directly and avoided the creation of
rtnl_configure_link_notify() in patch 1.

Re: [PATCHv5 net-next 1/4] rtnetlink: add new helper rtnl_configure_link_notify()

[Nicolas Dichtel]

Le 30/09/2022 à 18:01, Guillaume Nault a écrit :
> On Fri, Sep 30, 2022 at 04:22:19PM +0200, Nicolas Dichtel wrote:
>> Le 30/09/2022 à 11:45, Hangbin Liu a écrit :
>>> -int rtnl_configure_link(struct net_device *dev, const struct ifinfomsg *ifm)
>>> +static int rtnl_configure_link_notify(struct net_device *dev, const struct ifinfomsg *ifm,
>>> +				      struct nlmsghdr *nlh, u32 pid)
>> But not here. Following patches also use this order instead of the previous one.
>> For consistency, it could be good to keep the same order everywhere.
> 
> Yes, since a v6 will be necessary anyway, let's be consistent about the
> order of parameters. That helps reading the code.
> 
> While there, I'd prefer to use 'portid' instead of 'pid'. I know
> rtnetlink.c uses both, but 'portid' is more explicit and is what
> af_netlink.c generally uses.
> 
+1

pid is historical but too confusing.

Re: [PATCHv5 net-next 1/4] rtnetlink: add new helper rtnl_configure_link_notify()

[Hangbin Liu]

On Fri, Sep 30, 2022 at 11:40:21PM +0200, Nicolas Dichtel wrote:
> Le 30/09/2022 à 18:01, Guillaume Nault a écrit :
> > On Fri, Sep 30, 2022 at 04:22:19PM +0200, Nicolas Dichtel wrote:
> >> Le 30/09/2022 à 11:45, Hangbin Liu a écrit :
> >>> -int rtnl_configure_link(struct net_device *dev, const struct ifinfomsg *ifm)
> >>> +static int rtnl_configure_link_notify(struct net_device *dev, const struct ifinfomsg *ifm,
> >>> +				      struct nlmsghdr *nlh, u32 pid)
> >> But not here. Following patches also use this order instead of the previous one.
> >> For consistency, it could be good to keep the same order everywhere.
> > 
> > Yes, since a v6 will be necessary anyway, let's be consistent about the
> > order of parameters. That helps reading the code.
> > 
> > While there, I'd prefer to use 'portid' instead of 'pid'. I know
> > rtnetlink.c uses both, but 'portid' is more explicit and is what
> > af_netlink.c generally uses.
> > 
> +1
> 
> pid is historical but too confusing.

Thanks for all the comments. I will post the new patch when net-next
re-open.

Hangbin