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

[Guillaume Nault <gnault@redhat.com>]

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>