Date:   Thu, 29 Sep 2022 16:32:42 +0200
From:   Guillaume Nault <gnault@...hat.com>
To:     Hangbin Liu <liuhangbin@...il.com>
Cc:     Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Ido Schimmel <idosch@...dia.com>,
        Petr Machata <petrm@...dia.com>,
        Florent Fourcot <florent.fourcot@...irst.fr>,
        Nikolay Aleksandrov <razor@...ckwall.org>,
        Nicolas Dichtel <nicolas.dichtel@...nd.com>,
        David Ahern <dsahern@...nel.org>
Subject: Re: [PATCHv3 net-next] rtnetlink: Honour NLM_F_ECHO flag in
 rtnl_{new, set, del}link

On Thu, Sep 29, 2022 at 11:10:36AM +0800, Hangbin Liu wrote:
> On Wed, Sep 28, 2022 at 11:47:57AM +0200, Guillaume Nault wrote:
> > On Wed, Sep 28, 2022 at 10:39:49AM +0800, Hangbin Liu wrote:
> > > On Tue, Sep 27, 2022 at 07:21:30AM -0700, Jakub Kicinski wrote:
> > > > On Tue, 27 Sep 2022 12:13:03 +0800 Hangbin Liu wrote:
> > > > > @@ -3382,6 +3401,12 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
> > > > >  		if (err)
> > > > >  			goto out_unregister;
> > > > >  	}
> > > > > +
> > > > > +	nskb = rtmsg_ifinfo_build_skb(RTM_NEWLINK, dev, 0, 0, GFP_KERNEL, NULL,
> > > > > +				      0, pid, nlh->nlmsg_seq);
> > > > > +	if (nskb)
> > > > > +		rtnl_notify(nskb, dev_net(dev), pid, RTNLGRP_LINK, nlh, GFP_KERNEL);
> > > > > +
> > > > >  out:
> > > > >  	if (link_net)
> > > > >  		put_net(link_net);
> > > > 
> > > > I'm surprised you're adding new notifications. Does the kernel not
> > > > already notify about new links? I thought rtnl_newlink_create() ->
> > > > rtnl_configure_link() -> __dev_notify_flags() sends a notification,
> > > > already.
> > > 
> > > I think __dev_notify_flags() only sends notification when dev flag changed.
> > > On the other hand, the notification is sent via multicast, while this patch
> > > is intend to unicast the notification to the user space.
> > 
> > In rntl_configure_link(), dev->rtnl_link_state is RTNL_LINK_INITIALIZING
> > on device cretation, so __dev_notify_flags() is called with gchanges=~0
> > and notification should be always sent. It's just a matter of passing the
> > portid and the nlmsghdr down the call chain to make rtnl_notify() send
> > the unicast message together with the multicast ones.
> 
> To update __dev_notify_flags() with nlmsghdr, we also need to update
> rtnl_configure_link(), which is called by some drivers.

There's just a handful of virtual net devices that call
rtnl_configure_link(). It should be easy to modify its prototype
and adjust these external callers.

> > Now for device modification, I'm not sure there's a use case for
> > unicast notifications. The caller already knows which values it asked
> > to modify, so ECHO doesn't bring much value compared to a simple ACK.
> 
> And the __dev_notify_flags() is only used when the dev flag changed.
> 
> It looks no much change if we call it when create new link:
> rtnl_newlink_create() -> rtnl_configure_link() -> __dev_notify_flags()
> 
> But when set link, it is only called when flag changed
> do_setlink() -> dev_change_flags() -> __dev_notify_flags().
> 
> Unless you want to omit the ECHO message when setting link.

For SET operations, we may send one, many or even zero notifications.
I agree with Jackub that we shouldn't add specific notifications just
for NLM_F_ECHO. That means, either we echo all the existing
notifications (or none if the device hasn't been modified), or we
leave NLM_F_ECHO unimplemented for SET operations.

As I said in my previous reply, I can't see any use case for ECHO on
SET operations, so I don't mind if we skip it.

> At latest, when call rtnl_delete_link(), there is no way to call
> __dev_notify_flags(). So we still need to use the current way.

Not everything has to be done with __dev_notify_flags(). For DEL
operations notifications are sent in netdev_unregister_many().
Given the overwhelming number of callers, modifying its prototype isn't
going to be practical, but you can use a wrapper.

Something like (very rough idea):

-void unregister_netdevice_many(struct list_head *head)
+void unregister_netdevice_many_notify(struct list_head *head, u32 portid,
+                                      const struct nlmsghdr *nlh)
 {
[...]
                 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);
+                        skb = rtmsg_ifinfo_build_skb(RTM_DELLINK, portid,
+                                                     nlh->nlmsg_seq, dev, ~0U,
+                                                     0, GFP_KERNEL, NULL, 0);
 
[...]
                if (skb)
-                        rtmsg_ifinfo_send(skb, dev, GFP_KERNEL);
+                        rtmsg_ifinfo_send(skb, portid, dev, nlh, GFP_KERNEL);
 
[...]
 }
 
+void unregister_netdevice_many(struct list_head *head)
+{
+        unregister_netdevice_many_notify(head, 0, NULL);
+}

> As a summarize, we need to change a lot of code if we use __dev_notify_flags()
> to notify user, while we can only use it in one place. This looks not worth.
> 
> WDYT?

Using __dev_notify_flags() is only for NEW operations. SET and DEL have
to be handled differently.

To summarise, the objective is to reuse the existing notifications.
In practice that means just doing the required plumbing to pass the
correct parameters to the existing nlmsg_notify() calls (and ensure we
set the correct portid and sequence number in the netlink message
header).

> Thanks
> Hangbin
> 

Powered by blists - more mailing lists