Date:   Wed, 21 Sep 2022 18:14:09 +0200
From:   Guillaume Nault <gnault@...hat.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Hangbin Liu <liuhangbin@...il.com>, 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>
Subject: Re: [PATCH net-next] rtnetlink: Honour NLM_F_ECHO flag in rtnl_{new,
 set}link

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@...hat.com>
> > Signed-off-by: Hangbin Liu <liuhangbin@...il.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.

Powered by blists - more mailing lists