[PATCH net] rtnetlink: Restore RTM_NEW/DELLINK notification behavior

[PATCH net] rtnetlink: Restore RTM_NEW/DELLINK notification behavior

[Martin Willi]

The commits referenced below allows userspace to use the NLM_F_ECHO flag
for RTM_NEW/DELLINK operations to receive unicast notifications for the
affected link. Prior to these changes, applications may have relied on
multicast notifications to learn the same information without specifying
the NLM_F_ECHO flag.

For such applications, the mentioned commits changed the behavior for
requests not using NLM_F_ECHO. Multicast notifications are still received,
but now use the portid of the requester and the sequence number of the
request instead of zero values used previously. For the application, this
message may be unexpected and likely handled as a response to the
NLM_F_ACKed request, especially if it uses the same socket to handle
requests and notifications.

To fix existing applications relying on the old notification behavior,
set the portid and sequence number in the notification only if the
request included the NLM_F_ECHO flag. This restores the old behavior
for applications not using it, but allows unicasted notifications for
others.

Fixes: f3a63cce1b4f ("rtnetlink: Honour NLM_F_ECHO flag in rtnl_delete_link")
Fixes: d88e136cab37 ("rtnetlink: Honour NLM_F_ECHO flag in rtnl_newlink_create")
Signed-off-by: Martin Willi <martin@strongswan.org>
---
 include/linux/rtnetlink.h |  3 ++-
 net/core/dev.c            |  2 +-
 net/core/rtnetlink.c      | 11 +++++++++--
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
index 92ad75549e9c..b6e6378dcbbd 100644
--- a/include/linux/rtnetlink.h
+++ b/include/linux/rtnetlink.h
@@ -25,7 +25,8 @@ 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, u32 portid, u32 seq);
+				       int new_ifindex, u32 portid,
+				       const struct nlmsghdr *nlh);
 void rtmsg_ifinfo_send(struct sk_buff *skb, struct net_device *dev,
 		       gfp_t flags, u32 portid, const struct nlmsghdr *nlh);
 
diff --git a/net/core/dev.c b/net/core/dev.c
index 48067321c0db..1488f700bf81 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10847,7 +10847,7 @@ void unregister_netdevice_many_notify(struct list_head *head,
 		    dev->rtnl_link_state == RTNL_LINK_INITIALIZED)
 			skb = rtmsg_ifinfo_build_skb(RTM_DELLINK, dev, ~0U, 0,
 						     GFP_KERNEL, NULL, 0,
-						     portid, nlmsg_seq(nlh));
+						     portid, nlh);
 
 		/*
 		 *	Flush the unicast and multicast chains
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 5d8eb57867a9..6e44e92ebdf5 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3972,16 +3972,23 @@ 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, u32 portid, u32 seq)
+				       int new_ifindex, u32 portid,
+				       const struct nlmsghdr *nlh)
 {
 	struct net *net = dev_net(dev);
 	struct sk_buff *skb;
 	int err = -ENOBUFS;
+	u32 seq = 0;
 
 	skb = nlmsg_new(if_nlmsg_size(dev, 0), flags);
 	if (skb == NULL)
 		goto errout;
 
+	if (nlmsg_report(nlh))
+		seq = nlmsg_seq(nlh);
+	else
+		portid = 0;
+
 	err = rtnl_fill_ifinfo(skb, dev, dev_net(dev),
 			       type, portid, seq, change, 0, 0, event,
 			       new_nsid, new_ifindex, -1, flags);
@@ -4017,7 +4024,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, portid, nlmsg_seq(nlh));
+				     new_ifindex, portid, nlh);
 	if (skb)
 		rtmsg_ifinfo_send(skb, dev, flags, portid, nlh);
 }
-- 
2.34.1

Re: [PATCH net] rtnetlink: Restore RTM_NEW/DELLINK notification behavior

[Hangbin Liu]

On Tue, Apr 11, 2023 at 09:43:19AM +0200, Martin Willi wrote:
> The commits referenced below allows userspace to use the NLM_F_ECHO flag
> for RTM_NEW/DELLINK operations to receive unicast notifications for the
> affected link. Prior to these changes, applications may have relied on
> multicast notifications to learn the same information without specifying
> the NLM_F_ECHO flag.
> 
> For such applications, the mentioned commits changed the behavior for
> requests not using NLM_F_ECHO. Multicast notifications are still received,
> but now use the portid of the requester and the sequence number of the
> request instead of zero values used previously. For the application, this
> message may be unexpected and likely handled as a response to the
> NLM_F_ACKed request, especially if it uses the same socket to handle
> requests and notifications.
> 
> To fix existing applications relying on the old notification behavior,
> set the portid and sequence number in the notification only if the
> request included the NLM_F_ECHO flag. This restores the old behavior
> for applications not using it, but allows unicasted notifications for
> others.
> 
> Fixes: f3a63cce1b4f ("rtnetlink: Honour NLM_F_ECHO flag in rtnl_delete_link")
> Fixes: d88e136cab37 ("rtnetlink: Honour NLM_F_ECHO flag in rtnl_newlink_create")
> Signed-off-by: Martin Willi <martin@strongswan.org>

Not sure if the Fixes tag should be
1d997f101307 ("rtnetlink: pass netlink message header and portid to rtnl_configure_link()")

Others looks good to me.

Thanks
Hangbin

Re: [PATCH net] rtnetlink: Restore RTM_NEW/DELLINK notification behavior

[Martin Willi]

Hi,

> > Fixes: f3a63cce1b4f ("rtnetlink: Honour NLM_F_ECHO flag in rtnl_delete_link")
> > Fixes: d88e136cab37 ("rtnetlink: Honour NLM_F_ECHO flag in rtnl_newlink_create")
> > Signed-off-by: Martin Willi <martin@strongswan.org>
> 
> Not sure if the Fixes tag should be
> 1d997f101307 ("rtnetlink: pass netlink message header and portid to rtnl_configure_link()")

While this one adds the infrastructure, the discussed issue manifests
only with the two commits above. Anyway, I'm fine with either, let me
know if I shall change it.

Thanks,
Martin

Re: [PATCH net] rtnetlink: Restore RTM_NEW/DELLINK notification behavior

[Hangbin Liu]

On Wed, Apr 12, 2023 at 11:21:33AM +0200, Martin Willi wrote:
> Hi,
> 
> > > Fixes: f3a63cce1b4f ("rtnetlink: Honour NLM_F_ECHO flag in rtnl_delete_link")
> > > Fixes: d88e136cab37 ("rtnetlink: Honour NLM_F_ECHO flag in rtnl_newlink_create")
> > > Signed-off-by: Martin Willi <martin@strongswan.org>
> > 
> > Not sure if the Fixes tag should be
> > 1d997f101307 ("rtnetlink: pass netlink message header and portid to rtnl_configure_link()")
> 
> While this one adds the infrastructure, the discussed issue manifests

Yes

> only with the two commits above. Anyway, I'm fine with either, let me
> know if I shall change it.

In my understanding the above 2 commits only pass netlink header to
rtnl_configure_link. The question code in 1d997f101307 didn't check if
NLM_F_ECHO is honoured, as your commit pointed.

Thanks
Hangbin

Re: [PATCH net] rtnetlink: Restore RTM_NEW/DELLINK notification behavior

[Guillaume Nault]

On Wed, Apr 12, 2023 at 09:16:20PM +0800, Hangbin Liu wrote:
> On Wed, Apr 12, 2023 at 11:21:33AM +0200, Martin Willi wrote:
> > Hi,
> > 
> > > > Fixes: f3a63cce1b4f ("rtnetlink: Honour NLM_F_ECHO flag in rtnl_delete_link")
> > > > Fixes: d88e136cab37 ("rtnetlink: Honour NLM_F_ECHO flag in rtnl_newlink_create")
> > > > Signed-off-by: Martin Willi <martin@strongswan.org>
> > > 
> > > Not sure if the Fixes tag should be
> > > 1d997f101307 ("rtnetlink: pass netlink message header and portid to rtnl_configure_link()")
> > 
> > While this one adds the infrastructure, the discussed issue manifests
> 
> Yes
> 
> > only with the two commits above. Anyway, I'm fine with either, let me
> > know if I shall change it.
> 
> In my understanding the above 2 commits only pass netlink header to
> rtnl_configure_link. The question code in 1d997f101307 didn't check if
> NLM_F_ECHO is honoured, as your commit pointed.

That's right, but personally I find it clearer to cite the commits that
brought the actual behaviour change.

> Thanks
> Hangbin
> 

Re: [PATCH net] rtnetlink: Restore RTM_NEW/DELLINK notification behavior

[Hangbin Liu]

On Wed, Apr 12, 2023 at 11:42:14PM +0200, Guillaume Nault wrote:
> On Wed, Apr 12, 2023 at 09:16:20PM +0800, Hangbin Liu wrote:
> > On Wed, Apr 12, 2023 at 11:21:33AM +0200, Martin Willi wrote:
> > > Hi,
> > > 
> > > > > Fixes: f3a63cce1b4f ("rtnetlink: Honour NLM_F_ECHO flag in rtnl_delete_link")
> > > > > Fixes: d88e136cab37 ("rtnetlink: Honour NLM_F_ECHO flag in rtnl_newlink_create")
> > > > > Signed-off-by: Martin Willi <martin@strongswan.org>
> > > > 
> > > > Not sure if the Fixes tag should be
> > > > 1d997f101307 ("rtnetlink: pass netlink message header and portid to rtnl_configure_link()")
> > > 
> > > While this one adds the infrastructure, the discussed issue manifests
> > 
> > Yes
> > 
> > > only with the two commits above. Anyway, I'm fine with either, let me
> > > know if I shall change it.
> > 
> > In my understanding the above 2 commits only pass netlink header to
> > rtnl_configure_link. The question code in 1d997f101307 didn't check if
> > NLM_F_ECHO is honoured, as your commit pointed.
> 
> That's right, but personally I find it clearer to cite the commits that
> brought the actual behaviour change.

OK, fine by me.

Hangbin

Re: [PATCH net] rtnetlink: Restore RTM_NEW/DELLINK notification behavior

[Guillaume Nault]

On Tue, Apr 11, 2023 at 09:43:19AM +0200, Martin Willi wrote:
> The commits referenced below allows userspace to use the NLM_F_ECHO flag
> for RTM_NEW/DELLINK operations to receive unicast notifications for the
> affected link. Prior to these changes, applications may have relied on
> multicast notifications to learn the same information without specifying
> the NLM_F_ECHO flag.
> 
> For such applications, the mentioned commits changed the behavior for
> requests not using NLM_F_ECHO. Multicast notifications are still received,
> but now use the portid of the requester and the sequence number of the
> request instead of zero values used previously. For the application, this
> message may be unexpected and likely handled as a response to the
> NLM_F_ACKed request, especially if it uses the same socket to handle
> requests and notifications.
> 
> To fix existing applications relying on the old notification behavior,
> set the portid and sequence number in the notification only if the
> request included the NLM_F_ECHO flag. This restores the old behavior
> for applications not using it, but allows unicasted notifications for
> others.
> 
> Fixes: f3a63cce1b4f ("rtnetlink: Honour NLM_F_ECHO flag in rtnl_delete_link")
> Fixes: d88e136cab37 ("rtnetlink: Honour NLM_F_ECHO flag in rtnl_newlink_create")
> Signed-off-by: Martin Willi <martin@strongswan.org>
> ---
>  include/linux/rtnetlink.h |  3 ++-
>  net/core/dev.c            |  2 +-
>  net/core/rtnetlink.c      | 11 +++++++++--
>  3 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 5d8eb57867a9..6e44e92ebdf5 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -3972,16 +3972,23 @@ 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, u32 portid, u32 seq)
> +				       int new_ifindex, u32 portid,
> +				       const struct nlmsghdr *nlh)
>  {
>  	struct net *net = dev_net(dev);
>  	struct sk_buff *skb;
>  	int err = -ENOBUFS;
> +	u32 seq = 0;
>  
>  	skb = nlmsg_new(if_nlmsg_size(dev, 0), flags);
>  	if (skb == NULL)
>  		goto errout;
>  
> +	if (nlmsg_report(nlh))
> +		seq = nlmsg_seq(nlh);
> +	else
> +		portid = 0;
> +
>  	err = rtnl_fill_ifinfo(skb, dev, dev_net(dev),
>  			       type, portid, seq, change, 0, 0, event,
>  			       new_nsid, new_ifindex, -1, flags);

This special case makes rtnetlink a bit more confusing (I don't think
other netlink handlers do that, but I haven't looked very far).

I mean, ideally, broadcast netlink messages would never have portid and
seq, so we wouldn't need this special case at all.

But for that, we'd need a way for nlmsg_notify() to use a different
nlmsghdr for the unicast and broadcast messages (or let it rewrite it).

Also the value of seq and portid on broadcast notifications seems to
have always been inconsistent. Some rtnetlink commands seem to have
always set seq and portid on broadcast notifications (like RTM_NEWROUTE
and RTM_NEWADDR) while other commands didn't (RTM_NEWLINK). With
the commits cited in the Fixes tags, RTM_NEWLINK also started to set
portid and seq, and this patch now sets them only if the original
command had NLM_F_ECHO. I guess it's too late make broadcast
notifications consistent anyway :/.

For lack of a better idea,

Acked-by: Guillaume Nault <gnault@redhat.com>

Re: [PATCH net] rtnetlink: Restore RTM_NEW/DELLINK notification behavior

[Hangbin Liu]

On Tue, Apr 11, 2023 at 09:43:19AM +0200, Martin Willi wrote:
> The commits referenced below allows userspace to use the NLM_F_ECHO flag
> for RTM_NEW/DELLINK operations to receive unicast notifications for the
> affected link. Prior to these changes, applications may have relied on
> multicast notifications to learn the same information without specifying
> the NLM_F_ECHO flag.
> 
> For such applications, the mentioned commits changed the behavior for
> requests not using NLM_F_ECHO. Multicast notifications are still received,
> but now use the portid of the requester and the sequence number of the
> request instead of zero values used previously. For the application, this
> message may be unexpected and likely handled as a response to the
> NLM_F_ACKed request, especially if it uses the same socket to handle
> requests and notifications.
> 
> To fix existing applications relying on the old notification behavior,
> set the portid and sequence number in the notification only if the
> request included the NLM_F_ECHO flag. This restores the old behavior
> for applications not using it, but allows unicasted notifications for
> others.
> 
> Fixes: f3a63cce1b4f ("rtnetlink: Honour NLM_F_ECHO flag in rtnl_delete_link")
> Fixes: d88e136cab37 ("rtnetlink: Honour NLM_F_ECHO flag in rtnl_newlink_create")
> Signed-off-by: Martin Willi <martin@strongswan.org>

Acked-by: Hangbin Liu <liuhangbin@gmail.com>

Re: [PATCH net] rtnetlink: Restore RTM_NEW/DELLINK notification behavior

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 11 Apr 2023 09:43:19 +0200 you wrote:
> The commits referenced below allows userspace to use the NLM_F_ECHO flag
> for RTM_NEW/DELLINK operations to receive unicast notifications for the
> affected link. Prior to these changes, applications may have relied on
> multicast notifications to learn the same information without specifying
> the NLM_F_ECHO flag.
> 
> For such applications, the mentioned commits changed the behavior for
> requests not using NLM_F_ECHO. Multicast notifications are still received,
> but now use the portid of the requester and the sequence number of the
> request instead of zero values used previously. For the application, this
> message may be unexpected and likely handled as a response to the
> NLM_F_ACKed request, especially if it uses the same socket to handle
> requests and notifications.
> 
> [...]

Here is the summary with links:
  - [net] rtnetlink: Restore RTM_NEW/DELLINK notification behavior
    https://git.kernel.org/netdev/net/c/59d3efd27c11

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html