Message-ID: <ZjzVM1GXb5tkoqlB@hog>
Date: Thu, 9 May 2024 15:52:51 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Antonio Quartulli <antonio@...nvpn.net>
Cc: netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
	Sergey Ryazanov <ryazanov.s.a@...il.com>,
	Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
	Andrew Lunn <andrew@...n.ch>, Esben Haabendal <esben@...nix.com>
Subject: Re: [PATCH net-next v3 04/24] ovpn: add basic interface
 creation/destruction/management routines

2024-05-09, 15:25:21 +0200, Antonio Quartulli wrote:
> By the way, thank you very much for taking the time to have this
> constructive discussion. I really appreciate it!

Cheers :)


> On 09/05/2024 14:16, Sabrina Dubroca wrote:
> > 2024-05-09, 12:35:32 +0200, Antonio Quartulli wrote:
> > > On 09/05/2024 12:09, Sabrina Dubroca wrote:
> > > Hence I decided to fully trigger the unregistration.
> > 
> > That's the bit that doesn't make sense to me: the device is going
> > away, so you trigger a manual unregister. Cleaning up some additional
> > resources (peers etc), that makes sense. But calling
> > unregister_netdevice (when you're most likely getting called from
> > unregister_netdevice already, because I don't see other spots setting
> > dev->reg_state = NETREG_UNREGISTERING) is what I don't get. And I
> > wonder why you're not hitting the BUG_ON in
> > unregister_netdevice_many_notify:
> > 
> >      BUG_ON(dev->reg_state != NETREG_REGISTERED);
> 
> I think because we have our ovpn->registered check.
>
> It ensures that we don't call ovpn_iface_destruct more than once.

Ah, probably, yes.

> But now, that I implemented the rtnl_link_ops I can confirm I am hitting the
> BUG_ON. And now it makes sense.
> 
> I presume that now I can I simply remove the call to unregister_netdevice()
> from ovpn_iface_destruct() and move it to ovpn_nl_del_iface_doit().

Sounds good.


> > > > Because you create your devices via genl (which I'm not a fan
> > > > of, even if it's a bit nicer for userspace having a single netlink api
> > > > to deal with),
> > > 
> > > Originally I had implemented the rtnl_link_ops, but the (meaningful)
> > > objection was that a user is never supposed to create an ovpn iface by
> > > himself, but there should always be an openvpn process running in userspace.
> > > Hence the restriction to genl only.
> > 
> > Sorry, but how does genl prevent a user from creating the ovpn
> > interface manually? Whatever API you define, anyone who manages to
> > come up with the right netlink message will be able to create an
> > interface. You can't stop people from using your API without your
> > official client.
> 
> I don't want to prevent people from creating ovpn ifaces the way they like.
> I just don't see how the rtnl_link API can be useful, other than allowing
> users to execute 'ip link add/del..'.
>
> And by design that is not a usecase we want to support, because once the
> iface is created, nothing will happen if there is no userspace software
> driving it (no matter if it is openvpn or anything else).
> 
> When explaining this decision, I like to make a comparison to virtual
> 802.11/wifi ifaces.
> They also lack rtnl_link (AFAIR) as they also require some userspace
> software to handle them in order to be useful.
> 
> All this said, having everything in one place looks cleaner too :)

>From an API point of view, maybe. But for the kernel implementation,
using rtnl_link_ops->newlink is easier.

> > > > default_device_exit_batch/default_device_exit_net think
> > > > ovpn devices are real NICs and move them back to init_net instead of
> > > > destroying them.
> > > > 
> > > > Maybe we can extend the condition in default_device_exit_net with a
> > > > new flag so that ovpn devices get destroyed by the core, even without
> > > > rtnl_link_ops?
> > > 
> > > Thanks for pointing out the function responsible for this decision.
> > > How would you extend the check though?
> > > 
> > > Alternatively, what if ovpn simply registers an empty rtnl_link_ops with
> > > netns_fund set to false? That should make the condition happy, while keeping
> > > ovpn genl-only
> > 
> > Yes. I was thinking about adding a flag to the device, because I
> > wasn't sure an almost empty rtnl_link_ops could be handled safely, but
> > it seems ok. ovs does it, see commit 5b9e7e160795 ("openvswitch:
> > introduce rtnl ops stub"). And, as that commit message says, "ip -d
> > link show" would also show that the device is of type openvpn (or
> > ovpn, whatever you put in ops->kind), which would be nice.
> 
> I just coded something along those lines.

Great, thanks.

> It seems pretty clean and we don't need to touch core (+ the bonus of having
> the name in "ip -d link")....and the iface does get destroyed upon netns
> exit! :-)
> 
> I am grasping much better how all these APIs work together now.

Nice :)

-- 
Sabrina

Powered by blists - more mailing lists