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

[PATCH net-next] rtnetlink: Honour NLM_F_ECHO flag in rtnl_{new, set}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).

This patch handles NLM_F_ECHO flag and send link info back after
rtnl_{new, set}link.

Suggested-by: Guillaume Nault <gnault@redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---

In this patch I use rtnl_unicast to send the nlmsg directly. But we can
also pass "struct nlmsghdr *nlh" to rtnl_newlink_create() and
do_setlink(), then call rtnl_notify to send the nlmsg. I'm not sure
which way is better, any comments?

For iproute2 patch, please see
https://patchwork.kernel.org/project/netdevbpf/patch/20220916033428.400131-2-liuhangbin@gmail.com/
---
 net/core/rtnetlink.c | 79 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 69 insertions(+), 10 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 74864dc46a7e..b65bd9ed8b0d 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2645,13 +2645,38 @@ static int do_set_proto_down(struct net_device *dev,
 	return 0;
 }
 
+static int rtnl_echo_link_info(struct net_device *dev, u32 pid, u32 seq,
+			       u32 ext_filter_mask, int tgt_netnsid)
+{
+	struct sk_buff *skb;
+	int err;
+
+	skb = nlmsg_new(if_nlmsg_size(dev, ext_filter_mask), GFP_KERNEL);
+	if (!skb)
+		return -ENOBUFS;
+
+	err = rtnl_fill_ifinfo(skb, dev, dev_net(dev), RTM_NEWLINK, pid, seq,
+			       0, 0, ext_filter_mask, 0, NULL, 0,
+			       tgt_netnsid, GFP_KERNEL);
+	if (err < 0) {
+		/* -EMSGSIZE implies BUG in if_nlmsg_size */
+		WARN_ON(err == -EMSGSIZE);
+		kfree_skb(skb);
+	} else {
+		err = rtnl_unicast(skb, dev_net(dev), pid);
+	}
+
+	return err;
+}
+
 #define DO_SETLINK_MODIFIED	0x01
 /* notify flag means notify + modified. */
 #define DO_SETLINK_NOTIFY	0x03
 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,
+		      u16 nlmsg_flags, u32 nlmsg_seq)
 {
 	const struct net_device_ops *ops = dev->netdev_ops;
 	char ifname[IFNAMSIZ];
@@ -3009,6 +3034,21 @@ static int do_setlink(const struct sk_buff *skb,
 		}
 	}
 
+	if (nlmsg_flags & NLM_F_ECHO) {
+		u32 ext_filter_mask = 0;
+		int tgt_netnsid = -1;
+
+		if (tb[IFLA_TARGET_NETNSID])
+			tgt_netnsid = nla_get_s32(tb[IFLA_TARGET_NETNSID]);
+
+		if (tb[IFLA_EXT_MASK])
+			ext_filter_mask = nla_get_u32(tb[IFLA_EXT_MASK]);
+
+		rtnl_echo_link_info(dev, NETLINK_CB(skb).portid,
+				    nlmsg_seq, ext_filter_mask,
+				    tgt_netnsid);
+	}
+
 errout:
 	if (status & DO_SETLINK_MODIFIED) {
 		if ((status & DO_SETLINK_NOTIFY) == DO_SETLINK_NOTIFY)
@@ -3069,7 +3109,9 @@ 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->nlmsg_flags, nlh->nlmsg_seq);
+
 errout:
 	return err;
 }
@@ -3293,14 +3335,15 @@ 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, u16 nlmsg_flags, u32 nlmsg_seq)
 {
 	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,
+					 nlmsg_flags, nlmsg_seq);
 			if (err < 0)
 				return err;
 		}
@@ -3312,13 +3355,15 @@ 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,
+			       u16 nlmsg_flags, u32 nlmsg_seq)
 {
 	unsigned char name_assign_type = NET_NAME_USER;
 	struct net *net = sock_net(skb->sk);
 	struct net *dest_net, *link_net;
 	struct net_device *dev;
 	char ifname[IFNAMSIZ];
+	int netnsid = -1;
 	int err;
 
 	if (!ops->alloc && !ops->setup)
@@ -3336,9 +3381,9 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
 		return PTR_ERR(dest_net);
 
 	if (tb[IFLA_LINK_NETNSID]) {
-		int id = nla_get_s32(tb[IFLA_LINK_NETNSID]);
+		netnsid = nla_get_s32(tb[IFLA_LINK_NETNSID]);
 
-		link_net = get_net_ns_by_id(dest_net, id);
+		link_net = get_net_ns_by_id(dest_net, netnsid);
 		if (!link_net) {
 			NL_SET_ERR_MSG(extack, "Unknown network namespace id");
 			err =  -EINVAL;
@@ -3382,6 +3427,17 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
 		if (err)
 			goto out_unregister;
 	}
+
+	if (nlmsg_flags & NLM_F_ECHO) {
+		u32 ext_filter_mask = 0;
+
+		if (tb[IFLA_EXT_MASK])
+			ext_filter_mask = nla_get_u32(tb[IFLA_EXT_MASK]);
+
+		rtnl_echo_link_info(dev, NETLINK_CB(skb).portid, nlmsg_seq,
+				    ext_filter_mask, netnsid);
+	}
+
 out:
 	if (link_net)
 		put_net(link_net);
@@ -3544,7 +3600,8 @@ 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->nlmsg_flags, nlh->nlmsg_seq);
 	}
 
 	if (!(nlh->nlmsg_flags & NLM_F_CREATE)) {
@@ -3556,7 +3613,8 @@ 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->nlmsg_flags, nlh->nlmsg_seq);
 		return -ENODEV;
 	}
 
@@ -3578,7 +3636,8 @@ 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->nlmsg_flags, nlh->nlmsg_seq);
 }
 
 static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
-- 
2.37.2

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

[Nicolas Dichtel]

Le 21/09/2022 à 05:07, Hangbin Liu a écrit :
> 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).
> 
> This patch handles NLM_F_ECHO flag and send link info back after
> rtnl_{new, set}link.
> 
> Suggested-by: Guillaume Nault <gnault@redhat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---

[snip]

> @@ -3336,9 +3381,9 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
>  		return PTR_ERR(dest_net);
>  
>  	if (tb[IFLA_LINK_NETNSID]) {
> -		int id = nla_get_s32(tb[IFLA_LINK_NETNSID]);
> +		netnsid = nla_get_s32(tb[IFLA_LINK_NETNSID]);
>  
> -		link_net = get_net_ns_by_id(dest_net, id);
> +		link_net = get_net_ns_by_id(dest_net, netnsid);
>  		if (!link_net) {
>  			NL_SET_ERR_MSG(extack, "Unknown network namespace id");
>  			err =  -EINVAL;
> @@ -3382,6 +3427,17 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
>  		if (err)
>  			goto out_unregister;
>  	}
> +
> +	if (nlmsg_flags & NLM_F_ECHO) {
> +		u32 ext_filter_mask = 0;
> +
> +		if (tb[IFLA_EXT_MASK])
> +			ext_filter_mask = nla_get_u32(tb[IFLA_EXT_MASK]);
> +
> +		rtnl_echo_link_info(dev, NETLINK_CB(skb).portid, nlmsg_seq,
> +				    ext_filter_mask, netnsid);
=> netnsid, ie IFLA_LINK_NETNSID has nothing to do with IFLA_TARGET_NETNSID.
Link netns is used for x-netns interface like vlan for example. The vlan iface
could be in a netns while its lower iface could be in another netns.

The target netns is used when a netlink message is sent in a netns but should
act in another netns.

Regards,
Nicolas

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

[Hangbin Liu]

On Wed, Sep 21, 2022 at 11:11:19AM +0200, Nicolas Dichtel wrote:
> > @@ -3336,9 +3381,9 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
> >  		return PTR_ERR(dest_net);
> >  
> >  	if (tb[IFLA_LINK_NETNSID]) {
> > -		int id = nla_get_s32(tb[IFLA_LINK_NETNSID]);
> > +		netnsid = nla_get_s32(tb[IFLA_LINK_NETNSID]);
> >  
> > -		link_net = get_net_ns_by_id(dest_net, id);
> > +		link_net = get_net_ns_by_id(dest_net, netnsid);
> >  		if (!link_net) {
> >  			NL_SET_ERR_MSG(extack, "Unknown network namespace id");
> >  			err =  -EINVAL;
> > @@ -3382,6 +3427,17 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
> >  		if (err)
> >  			goto out_unregister;
> >  	}
> > +
> > +	if (nlmsg_flags & NLM_F_ECHO) {
> > +		u32 ext_filter_mask = 0;
> > +
> > +		if (tb[IFLA_EXT_MASK])
> > +			ext_filter_mask = nla_get_u32(tb[IFLA_EXT_MASK]);
> > +
> > +		rtnl_echo_link_info(dev, NETLINK_CB(skb).portid, nlmsg_seq,
> > +				    ext_filter_mask, netnsid);
> => netnsid, ie IFLA_LINK_NETNSID has nothing to do with IFLA_TARGET_NETNSID.
> Link netns is used for x-netns interface like vlan for example. The vlan iface
> could be in a netns while its lower iface could be in another netns.
> 
> The target netns is used when a netlink message is sent in a netns but should
> act in another netns.

Oh, thanks for the explanation. Then we can remove the netnsid parameter
from rtnl_echo_link_info().

Thanks
Hangbin

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

[Jakub Kicinski]

On Wed, 21 Sep 2022 11:07:21 +0800 Hangbin Liu wrote:
> 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).
> 
> This patch handles NLM_F_ECHO flag and send link info back after
> rtnl_{new, set}link.
> 
> Suggested-by: Guillaume Nault <gnault@redhat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
> 
> In this patch I use rtnl_unicast to send the nlmsg directly. But we can
> also pass "struct nlmsghdr *nlh" to rtnl_newlink_create() and
> do_setlink(), then call rtnl_notify to send the nlmsg. I'm not sure
> which way is better, any comments?
> 
> For iproute2 patch, please see
> https://patchwork.kernel.org/project/netdevbpf/patch/20220916033428.400131-2-liuhangbin@gmail.com/

I feel like the justification for the change is lacking.

I'm biased [and frankly it takes a lot of self-restraint for me not 
to say how I _really_ feel about netlink msg flags ;)] but IMO the
message flags fall squarely into the "this is magic which was never
properly implemented" bucket.

What makes this flag better than just issuing a GET command form user
space?

The flag was never checked on input and is not implemented by 99% of
netlink families and commands.

I'd love to hear what others think. IMO we should declare a moratorium
on any use of netlink flags and fixed fields, push netlink towards
being a simple conduit for TLVs.

> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 74864dc46a7e..b65bd9ed8b0d 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -2645,13 +2645,38 @@ static int do_set_proto_down(struct net_device *dev,
>  	return 0;
>  }
>  
> +static int rtnl_echo_link_info(struct net_device *dev, u32 pid, u32 seq,
> +			       u32 ext_filter_mask, int tgt_netnsid)
> +{
> +	struct sk_buff *skb;
> +	int err;
> +
> +	skb = nlmsg_new(if_nlmsg_size(dev, ext_filter_mask), GFP_KERNEL);
> +	if (!skb)
> +		return -ENOBUFS;
> +
> +	err = rtnl_fill_ifinfo(skb, dev, dev_net(dev), RTM_NEWLINK, pid, seq,
> +			       0, 0, ext_filter_mask, 0, NULL, 0,
> +			       tgt_netnsid, GFP_KERNEL);
> +	if (err < 0) {
> +		/* -EMSGSIZE implies BUG in if_nlmsg_size */
> +		WARN_ON(err == -EMSGSIZE);
> +		kfree_skb(skb);
> +	} else {
> +		err = rtnl_unicast(skb, dev_net(dev), pid);
> +	}
> +
> +	return err;
> +}
> +
>  #define DO_SETLINK_MODIFIED	0x01
>  /* notify flag means notify + modified. */
>  #define DO_SETLINK_NOTIFY	0x03
>  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,
> +		      u16 nlmsg_flags, u32 nlmsg_seq)
>  {
>  	const struct net_device_ops *ops = dev->netdev_ops;
>  	char ifname[IFNAMSIZ];
> @@ -3009,6 +3034,21 @@ static int do_setlink(const struct sk_buff *skb,
>  		}
>  	}
>  
> +	if (nlmsg_flags & NLM_F_ECHO) {
> +		u32 ext_filter_mask = 0;
> +		int tgt_netnsid = -1;
> +
> +		if (tb[IFLA_TARGET_NETNSID])
> +			tgt_netnsid = nla_get_s32(tb[IFLA_TARGET_NETNSID]);
> +
> +		if (tb[IFLA_EXT_MASK])
> +			ext_filter_mask = nla_get_u32(tb[IFLA_EXT_MASK]);
> +
> +		rtnl_echo_link_info(dev, NETLINK_CB(skb).portid,
> +				    nlmsg_seq, ext_filter_mask,
> +				    tgt_netnsid);
> +	}
> +
>  errout:
>  	if (status & DO_SETLINK_MODIFIED) {
>  		if ((status & DO_SETLINK_NOTIFY) == DO_SETLINK_NOTIFY)
> @@ -3069,7 +3109,9 @@ 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->nlmsg_flags, nlh->nlmsg_seq);
> +
>  errout:
>  	return err;
>  }
> @@ -3293,14 +3335,15 @@ 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, u16 nlmsg_flags, u32 nlmsg_seq)
>  {
>  	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,
> +					 nlmsg_flags, nlmsg_seq);
>  			if (err < 0)
>  				return err;
>  		}
> @@ -3312,13 +3355,15 @@ 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,
> +			       u16 nlmsg_flags, u32 nlmsg_seq)
>  {
>  	unsigned char name_assign_type = NET_NAME_USER;
>  	struct net *net = sock_net(skb->sk);
>  	struct net *dest_net, *link_net;
>  	struct net_device *dev;
>  	char ifname[IFNAMSIZ];
> +	int netnsid = -1;
>  	int err;
>  
>  	if (!ops->alloc && !ops->setup)
> @@ -3336,9 +3381,9 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
>  		return PTR_ERR(dest_net);
>  
>  	if (tb[IFLA_LINK_NETNSID]) {
> -		int id = nla_get_s32(tb[IFLA_LINK_NETNSID]);
> +		netnsid = nla_get_s32(tb[IFLA_LINK_NETNSID]);
>  
> -		link_net = get_net_ns_by_id(dest_net, id);
> +		link_net = get_net_ns_by_id(dest_net, netnsid);
>  		if (!link_net) {
>  			NL_SET_ERR_MSG(extack, "Unknown network namespace id");
>  			err =  -EINVAL;
> @@ -3382,6 +3427,17 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
>  		if (err)
>  			goto out_unregister;
>  	}
> +
> +	if (nlmsg_flags & NLM_F_ECHO) {
> +		u32 ext_filter_mask = 0;
> +
> +		if (tb[IFLA_EXT_MASK])
> +			ext_filter_mask = nla_get_u32(tb[IFLA_EXT_MASK]);
> +
> +		rtnl_echo_link_info(dev, NETLINK_CB(skb).portid, nlmsg_seq,
> +				    ext_filter_mask, netnsid);
> +	}
> +
>  out:
>  	if (link_net)
>  		put_net(link_net);
> @@ -3544,7 +3600,8 @@ 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->nlmsg_flags, nlh->nlmsg_seq);
>  	}
>  
>  	if (!(nlh->nlmsg_flags & NLM_F_CREATE)) {
> @@ -3556,7 +3613,8 @@ 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->nlmsg_flags, nlh->nlmsg_seq);
>  		return -ENODEV;
>  	}
>  
> @@ -3578,7 +3636,8 @@ 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->nlmsg_flags, nlh->nlmsg_seq);
>  }
>  
>  static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,

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

[Nikolay Aleksandrov]

On 21/09/2022 16:01, Jakub Kicinski wrote:
> On Wed, 21 Sep 2022 11:07:21 +0800 Hangbin Liu wrote:
>> 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).
>>
>> This patch handles NLM_F_ECHO flag and send link info back after
>> rtnl_{new, set}link.
>>
>> Suggested-by: Guillaume Nault <gnault@redhat.com>
>> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
>> ---
>>
>> In this patch I use rtnl_unicast to send the nlmsg directly. But we can
>> also pass "struct nlmsghdr *nlh" to rtnl_newlink_create() and
>> do_setlink(), then call rtnl_notify to send the nlmsg. I'm not sure
>> which way is better, any comments?
>>
>> For iproute2 patch, please see
>> https://patchwork.kernel.org/project/netdevbpf/patch/20220916033428.400131-2-liuhangbin@gmail.com/
> 
> I feel like the justification for the change is lacking.
> 
> I'm biased [and frankly it takes a lot of self-restraint for me not
> to say how I _really_ feel about netlink msg flags ;)] but IMO the
> message flags fall squarely into the "this is magic which was never
> properly implemented" bucket.
> 
> What makes this flag better than just issuing a GET command form user
> space?
> 
> The flag was never checked on input and is not implemented by 99% of
> netlink families and commands.
> > I'd love to hear what others think. IMO we should declare a moratorium
> on any use of netlink flags and fixed fields, push netlink towards
> being a simple conduit for TLVs.
> 

+1
Just issue a "get" after the change.

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

[Guillaume Nault]

On Wed, Sep 21, 2022 at 06:01:23AM -0700, Jakub Kicinski wrote:
> On Wed, 21 Sep 2022 11:07:21 +0800 Hangbin Liu wrote:
> > 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).
> > 
> > This patch handles NLM_F_ECHO flag and send link info back after
> > rtnl_{new, set}link.
> > 
> > Suggested-by: Guillaume Nault <gnault@redhat.com>
> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> > ---
> > 
> > In this patch I use rtnl_unicast to send the nlmsg directly. But we can
> > also pass "struct nlmsghdr *nlh" to rtnl_newlink_create() and
> > do_setlink(), then call rtnl_notify to send the nlmsg. I'm not sure
> > which way is better, any comments?
> > 
> > For iproute2 patch, please see
> > https://patchwork.kernel.org/project/netdevbpf/patch/20220916033428.400131-2-liuhangbin@gmail.com/
> 
> I feel like the justification for the change is lacking.

To be fair, this is an old idea of mine, that Hangbin just picked it
up. So let me just explain a bit the original reasons behin his work.

> I'm biased [and frankly it takes a lot of self-restraint for me not
> to say how I _really_ feel about netlink msg flags ;)] but IMO the
> message flags fall squarely into the "this is magic which was never
> properly implemented" bucket.

My original idea was indeed to fix NLM_F_ECHO, rather than discarding
the whole idea. It's true that most netlink handlers don't handle it
properly, but some do and it happened to be useful on some projects I
worked on in the past.

> What makes this flag better than just issuing a GET command form user
> space?

I can see three problems with the extra GET command approach:

  * It's racy wrt. external processes. For example when creating a
    network device, the ifname might be changed by an external process
    between the RTM_NEWLINK and the RTM_GETLINK calls.

  * In some cases, you just don't have the required information to
    build a GET message. Reusing the previous example, one could let
    the kernel choose an ifname for the new device, to ensure the
    request isn't going to fail because another device with the same
    ifname already exists. Then there's no ifname or ifindex that can
    be used to query the new device.

  * GET obviously doesn't work after a DEL command. With NLM_F_ECHO,
    one can get the precise informations of the object that was
    deleted.

We can tell developpers to work around these problems by listening at
netlink notifications but that's not very practical. Depending on the
modified object it can be hard or maybe even impossible to accurately
match a notification with the original request. Also, in multi-threaded
programs, the notification handler will likely run in a different
thread. So extra synchronisation will be required between the thread
making the kernel request and the thread reading netlink events. With
NLM_F_ECHO the thread that makes the request can simply read its
netlink socket to get the information.

To summarise, NLM_F_ECHO allows a program to get a reliable feedback
on how the kernel handles its request. I'm not aware of any other
mechanism that provides this reliability.

> The flag was never checked on input and is not implemented by 99% of
> netlink families and commands.

There's at least one case where a netlink message handler later
realised that it needed to handle NLM_F_ECHO:
commit 993e4c929a07 ("netns: fix NLM_F_ECHO mechanism for RTM_NEWNSID")

This commit avoided the need for RTM_NEWNSID to reimplement the same
mechanism in its own way in net/core/net_namespace.c:
https://lore.kernel.org/netdev/20190930160214.4512-1-nicolas.dichtel@6wind.com/#t

> I'd love to hear what others think. IMO we should declare a moratorium
> on any use of netlink flags and fixed fields, push netlink towards
> being a simple conduit for TLVs.

At my previous employer, we had a small program inserting and removing
routes depending on several external events (not a full-fledged routing
daemon). NLM_F_ECHO was used at least to log the real kernel actions (as
opposed to what the program intended to do) and link that to the events
that triggered these actions. That was really helpful for network
administrators. Yes, we were lucky that the RTM_NEWROUTE and
RTM_DELROUTE message handlers supported NLM_F_ECHO. I was surprised when
I later realised that RTM_NEWLINK and many others didn't.

Then, a few years ago, I had questions from another team (maybe Network
Manager but I'm not sure) who asked how to reliably retrieve
informations like the ifindex of newly created devices. That's the use
case NLM_F_ECHO is for, but lacking this feature this team had to rely
on a more convoluted and probably racy way. That was the moment I
decided to expose the problem to our team. Fast-forwarding a couple of
years and Hangbin picked up the task.

Sorry for the long email. I hope the context and use cases are clearer
now.

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

[Jakub Kicinski]

On Wed, 21 Sep 2022 18:14:09 +0200 Guillaume Nault wrote:
> > I'd love to hear what others think. IMO we should declare a moratorium
> > on any use of netlink flags and fixed fields, push netlink towards
> > being a simple conduit for TLVs.  
> 
> At my previous employer, we had a small program inserting and removing
> routes depending on several external events (not a full-fledged routing
> daemon). NLM_F_ECHO was used at least to log the real kernel actions (as
> opposed to what the program intended to do) and link that to the events
> that triggered these actions. That was really helpful for network
> administrators. Yes, we were lucky that the RTM_NEWROUTE and
> RTM_DELROUTE message handlers supported NLM_F_ECHO. I was surprised when
> I later realised that RTM_NEWLINK and many others didn't.
> 
> Then, a few years ago, I had questions from another team (maybe Network
> Manager but I'm not sure) who asked how to reliably retrieve
> informations like the ifindex of newly created devices. That's the use
> case NLM_F_ECHO is for, but lacking this feature this team had to
> rely on a more convoluted and probably racy way. That was the moment
> I decided to expose the problem to our team. Fast-forwarding a couple
> of years and Hangbin picked up the task.

Looking closer at the code it seems like what NLM_F_ECHO does in most
places is to loop notifications resulting from the command back onto
the requesting socket. See nlmsg_notify(), report is usually passed 
as nlmsg_report(req).

I guess that answers Hangbin's question - yes, I'd vote that we just
pass the nlh to rtnl_notify() and let the netlink core do its thing.

In general I still don't think NLM_F_ECHO makes for a reasonable API.
It may seem okay to those who are willing to write manual netlink
parsers but for a normal programmer the ability to receive directly
notifications resulting from a API call they made is going to mean..
nothing they can have prior experience with. NEWLINK should have
reported the allocated handle / ifindex from the start :(

The "give me back the notifications" semantics match well your use
case to log what the command has done, in that case there is no need 
to "return" all the notifications from the API call.

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

[Hangbin Liu]

On Wed, Sep 21, 2022 at 03:56:40PM -0700, Jakub Kicinski wrote:
> Looking closer at the code it seems like what NLM_F_ECHO does in most
> places is to loop notifications resulting from the command back onto
> the requesting socket. See nlmsg_notify(), report is usually passed 
> as nlmsg_report(req).
> 
> I guess that answers Hangbin's question - yes, I'd vote that we just
> pass the nlh to rtnl_notify() and let the netlink core do its thing.

Thanks, I will update the patch by using rtnl_notify().

> 
> In general I still don't think NLM_F_ECHO makes for a reasonable API.
> It may seem okay to those who are willing to write manual netlink
> parsers but for a normal programmer the ability to receive directly
> notifications resulting from a API call they made is going to mean..
> nothing they can have prior experience with. NEWLINK should have
> reported the allocated handle / ifindex from the start :(
> 
> The "give me back the notifications" semantics match well your use
> case to log what the command has done, in that case there is no need 
> to "return" all the notifications from the API call.

I didn't get what you mean about "no need to return all the notifications from
the API call"? Do you ask for some update of the patch, or just talking about
your propose of NEWLINK?

Thanks
Hangbin

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

[Florent Fourcot]

Hello,

On 22/09/2022 00:56, Jakub Kicinski wrote:
> NEWLINK should have
> reported the allocated handle / ifindex from the start


Could we not return ifindex as a COOKIE when extended ACK is enabled?

However, I'm note sure that it will helps to simplify API. COOKIE are 
not well defined at the best of my knowledge (and very rarely used).

-- 
Florent.

-- 
*Ce message et toutes les pièces jointes (ci-après le "message") sont 
établis à l’intention exclusive des destinataires désignés. Il contient des 
informations confidentielles et pouvant être protégé par le secret 
professionnel. Si vous recevez ce message par erreur, merci d'en avertir 
immédiatement l'expéditeur et de détruire le message. Toute utilisation de 
ce message non conforme à sa destination, toute diffusion ou toute 
publication, totale ou partielle, est interdite, sauf autorisation expresse 
de l'émetteur*

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

[Guillaume Nault]

On Wed, Sep 21, 2022 at 03:56:40PM -0700, Jakub Kicinski wrote:
> On Wed, 21 Sep 2022 18:14:09 +0200 Guillaume Nault wrote:
> > > I'd love to hear what others think. IMO we should declare a moratorium
> > > on any use of netlink flags and fixed fields, push netlink towards
> > > being a simple conduit for TLVs.  
> > 
> > At my previous employer, we had a small program inserting and removing
> > routes depending on several external events (not a full-fledged routing
> > daemon). NLM_F_ECHO was used at least to log the real kernel actions (as
> > opposed to what the program intended to do) and link that to the events
> > that triggered these actions. That was really helpful for network
> > administrators. Yes, we were lucky that the RTM_NEWROUTE and
> > RTM_DELROUTE message handlers supported NLM_F_ECHO. I was surprised when
> > I later realised that RTM_NEWLINK and many others didn't.
> > 
> > Then, a few years ago, I had questions from another team (maybe Network
> > Manager but I'm not sure) who asked how to reliably retrieve
> > informations like the ifindex of newly created devices. That's the use
> > case NLM_F_ECHO is for, but lacking this feature this team had to
> > rely on a more convoluted and probably racy way. That was the moment
> > I decided to expose the problem to our team. Fast-forwarding a couple
> > of years and Hangbin picked up the task.
> 
> Looking closer at the code it seems like what NLM_F_ECHO does in most
> places is to loop notifications resulting from the command back onto
> the requesting socket. See nlmsg_notify(), report is usually passed 
> as nlmsg_report(req).

Yes, this is how it's supposed to work. NLM_F_ECHO is already handled
by the core netlink code. Netlink message handlers only have to pass
the right parameters to nlmsg_notify() (or the rtnl_notify() wrapper
for rtnetlink) to make it work.

That's why I complained when RTM_NEWNSID tried to implement its own
notification mechanism:
https://lore.kernel.org/netdev/20191003161940.GA31862@linux.home/

I mean, let's just use the built-in mechanism, rather than reinventing
a new one every time the need comes up.

> I guess that answers Hangbin's question - yes, I'd vote that we just
> pass the nlh to rtnl_notify() and let the netlink core do its thing.

Definitely.

> In general I still don't think NLM_F_ECHO makes for a reasonable API.
> It may seem okay to those who are willing to write manual netlink
> parsers but for a normal programmer the ability to receive directly
> notifications resulting from a API call they made is going to mean..
> nothing they can have prior experience with. NEWLINK should have
> reported the allocated handle / ifindex from the start :(

I also don't know of any other API that works this way. But given the
current situation, I also can't really see that change.

> The "give me back the notifications" semantics match well your use
> case to log what the command has done, in that case there is no need
> to "return" all the notifications from the API call.

It's not really _my_ use case anymore :), but I'm pretty sure this
piece of software is still in use. Anyway I used this example just to
illustrate why a programmer would use this feature. The RTM_NEWNSID
is another practical use case.

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

[Jakub Kicinski]

On Thu, 22 Sep 2022 18:13:33 +0800 Hangbin Liu wrote:
> > In general I still don't think NLM_F_ECHO makes for a reasonable API.
> > It may seem okay to those who are willing to write manual netlink
> > parsers but for a normal programmer the ability to receive directly
> > notifications resulting from a API call they made is going to mean..
> > nothing they can have prior experience with. NEWLINK should have
> > reported the allocated handle / ifindex from the start :(
> > 
> > The "give me back the notifications" semantics match well your use
> > case to log what the command has done, in that case there is no need 
> > to "return" all the notifications from the API call.  
> 
> I didn't get what you mean about "no need to return all the notifications from
> the API call"? Do you ask for some update of the patch, or just talking about
> your propose of NEWLINK?

I'm talking about building "normal programmer" facing libraries on top
of netlink. The concept of ECHO fits very poorly into the normal
request-response semantics.

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

[Jakub Kicinski]

On Thu, 22 Sep 2022 13:09:51 +0200 Guillaume Nault wrote:
> That's why I complained when RTM_NEWNSID tried to implement its own
> notification mechanism:
> https://lore.kernel.org/netdev/20191003161940.GA31862@linux.home/
> 
> I mean, let's just use the built-in mechanism, rather than reinventing
> a new one every time the need comes up.

See, when you say "let's just use the built-in mechanism" you worry 
me again. Let's be clear that no new API should require the use of
ECHO for normal operation, like finding out what the handle of an
allocated object is.

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

[Guillaume Nault]

On Thu, Sep 22, 2022 at 06:03:46AM -0700, Jakub Kicinski wrote:
> On Thu, 22 Sep 2022 13:09:51 +0200 Guillaume Nault wrote:
> > That's why I complained when RTM_NEWNSID tried to implement its own
> > notification mechanism:
> > https://lore.kernel.org/netdev/20191003161940.GA31862@linux.home/
> > 
> > I mean, let's just use the built-in mechanism, rather than reinventing
> > a new one every time the need comes up.
> 
> See, when you say "let's just use the built-in mechanism" you worry 
> me again. Let's be clear that no new API should require the use of
> ECHO for normal operation, like finding out what the handle of an
> allocated object is.

I've always thought the lack of NLM_F_ECHO support in many subsystems
was just an oversight, as it shouldn't take a lot of plumbing to make
it work. But if you prefer to deprecate the feature then okay.

I just don't see any way to pass a handle back to user space at the
moment. The echo mechanism did that and was generic to all netlink
families (as long as nlmsg_notify() was called with the right
parameters).

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

[Nicolas Dichtel]

Le 22/09/2022 à 16:51, Guillaume Nault a écrit :
> I just don't see any way to pass a handle back to user space at the
> moment. The echo mechanism did that and was generic to all netlink
> families (as long as nlmsg_notify() was called with the right
> parameters).
> 
+1

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

[Jakub Kicinski]

Let me clarify one more time in case Hangbin is waiting for 
the discussion to resolve...

On Fri, 23 Sep 2022 10:43:53 +0200 Nicolas Dichtel wrote:
> Le 22/09/2022 à 16:51, Guillaume Nault a écrit :
> > I just don't see any way to pass a handle back to user space at the
> > moment. The echo mechanism did that and was generic to all netlink
> > families (as long as nlmsg_notify() was called with the right
> > parameters).

In NEWLINK, right? In NEWLINK there is no way to pass it back 
at the moment. A newly added command can just respond with the handle
always. The problem with NEWLINK is that it _used to_ not respond so 
we can't make it start responding because it will confuse existing user
space.

At the protocol level NEW is no different than GET, whether it sends 
a response back is decided by whoever implements the command.

So yes, for NEWLINK we need a way to inform the kernel that user space
wants a reply. It can be via ECHO, it could be via a new attr.

What I'm trying to argue about is *not* whether NEWLINK should support
ECHO but whether requiring ECHO to get a response for newly added
CREATE / NEW commands is a good idea. I think it is not, and new
commands should just always respond with the handle.

My main concern with using ECHO is that it breaks the one-to-one
relationship between a request and a response. There may be multiple
notifications generated due to a command, and if we want to retain 
the "ECHO will loop back to you all resulting notifications" semantics,
which I think we should, then there can be multiple "responses".

This also has implications for the command IDs used in notifications.
A lot of modern genl families use different IDs for notifications to
make it easily distinguishable from responses.


I guess tl;dr is Hangbin should go forward with the v2, and I should
document the expectations clearly..

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

[Nicolas Dichtel]

Le 23/09/2022 à 15:48, Jakub Kicinski a écrit :
> Let me clarify one more time in case Hangbin is waiting for 
> the discussion to resolve...
> 
> On Fri, 23 Sep 2022 10:43:53 +0200 Nicolas Dichtel wrote:
>> Le 22/09/2022 à 16:51, Guillaume Nault a écrit :
>>> I just don't see any way to pass a handle back to user space at the
>>> moment. The echo mechanism did that and was generic to all netlink
>>> families (as long as nlmsg_notify() was called with the right
>>> parameters).
> 
> In NEWLINK, right? In NEWLINK there is no way to pass it back 
> at the moment. A newly added command can just respond with the handle
> always. The problem with NEWLINK is that it _used to_ not respond so 
> we can't make it start responding because it will confuse existing user
> space.
> 
> At the protocol level NEW is no different than GET, whether it sends 
> a response back is decided by whoever implements the command.
> 
> So yes, for NEWLINK we need a way to inform the kernel that user space
> wants a reply. It can be via ECHO, it could be via a new attr.
> 
> What I'm trying to argue about is *not* whether NEWLINK should support
> ECHO but whether requiring ECHO to get a response for newly added
> CREATE / NEW commands is a good idea. I think it is not, and new
> commands should just always respond with the handle.
Sure, but I don't see how we can enforce this.

> 
> My main concern with using ECHO is that it breaks the one-to-one
> relationship between a request and a response. There may be multiple
> notifications generated due to a command, and if we want to retain 
> the "ECHO will loop back to you all resulting notifications" semantics,
> which I think we should, then there can be multiple "responses".
Thanks for the detailed explanation.

> 
> This also has implications for the command IDs used in notifications.
> A lot of modern genl families use different IDs for notifications to
> make it easily distinguishable from responses.
I didn't know that. Indeed, it's a good idea.