| Message ID | 20230411074319.24133-1-martin@strongswan.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 59d3efd27c11c59b32291e5ebc307bed2edb65ee |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] rtnetlink: Restore RTM_NEW/DELLINK notification behavior | expand |
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
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
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
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>
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 >
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
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>
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!
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); }
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(-)