Re: [PATCH net-next v3 04/24] ovpn: add basic interface creation/destruction/management routines

[Sabrina Dubroca <sd@queasysnail.net>]

2024-05-06, 03:16:17 +0200, Antonio Quartulli wrote:
> diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
> index ad3813419c33..338e99dfe886 100644
> --- a/drivers/net/ovpn/io.c
> +++ b/drivers/net/ovpn/io.c
> @@ -11,6 +11,26 @@
>  #include <linux/skbuff.h>
>  
>  #include "io.h"
> +#include "ovpnstruct.h"
> +#include "netlink.h"
> +
> +int ovpn_struct_init(struct net_device *dev)

nit: Should this be in main.c? It's only used there, and I think it
would make more sense to drop it next to ovpn_struct_free.

> +{
> +	struct ovpn_struct *ovpn = netdev_priv(dev);
> +	int err;
> +

[...]
> diff --git a/drivers/net/ovpn/main.c b/drivers/net/ovpn/main.c
> index 33c0b004ce16..584cd7286aff 100644
> --- a/drivers/net/ovpn/main.c
> +++ b/drivers/net/ovpn/main.c
[...]
> +static void ovpn_struct_free(struct net_device *net)
> +{
> +	struct ovpn_struct *ovpn = netdev_priv(net);
> +
> +	rtnl_lock();

 ->priv_destructor can run from register_netdevice (already under
RTNL), this doesn't look right.

> +	list_del(&ovpn->dev_list);

And if this gets called from register_netdevice, the list_add from
ovpn_iface_create hasn't run yet, so this will probably do strange
things?

> +	rtnl_unlock();
> +
> +	free_percpu(net->tstats);
> +}
> +
> +static int ovpn_net_open(struct net_device *dev)
> +{
> +	struct in_device *dev_v4 = __in_dev_get_rtnl(dev);
> +
> +	if (dev_v4) {
> +		/* disable redirects as Linux gets confused by ovpn handling
> +		 * same-LAN routing
> +		 */
> +		IN_DEV_CONF_SET(dev_v4, SEND_REDIRECTS, false);
> +		IPV4_DEVCONF_ALL(dev_net(dev), SEND_REDIRECTS) = false;

Jakub, are you ok with that? This feels a bit weird to have in the
middle of a driver.

> +	}
> +
> +	netif_tx_start_all_queues(dev);
> +	return 0;
> +}

[...]
> +void ovpn_iface_destruct(struct ovpn_struct *ovpn)
> +{
> +	ASSERT_RTNL();
> +
> +	netif_carrier_off(ovpn->dev);
> +
> +	ovpn->registered = false;
> +
> +	unregister_netdevice(ovpn->dev);
> +	synchronize_net();

If this gets called from the loop in ovpn_netns_pre_exit, one
synchronize_net per ovpn device would seem quite expensive.

> +}
> +
>  static int ovpn_netdev_notifier_call(struct notifier_block *nb,
>  				     unsigned long state, void *ptr)
>  {
>  	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
> +	struct ovpn_struct *ovpn;
>  
>  	if (!ovpn_dev_is_valid(dev))
>  		return NOTIFY_DONE;
>  
> +	ovpn = netdev_priv(dev);
> +
>  	switch (state) {
>  	case NETDEV_REGISTER:
> -		/* add device to internal list for later destruction upon
> -		 * unregistration
> -		 */
> +		ovpn->registered = true;
>  		break;
>  	case NETDEV_UNREGISTER:
> +		/* twiddle thumbs on netns device moves */
> +		if (dev->reg_state != NETREG_UNREGISTERING)
> +			break;
> +
>  		/* can be delivered multiple times, so check registered flag,
>  		 * then destroy the interface
>  		 */
> +		if (!ovpn->registered)
> +			return NOTIFY_DONE;
> +
> +		ovpn_iface_destruct(ovpn);

Maybe I'm misunderstanding this code. Why do you want to manually
destroy a device that is already going away?

>  		break;
>  	case NETDEV_POST_INIT:
>  	case NETDEV_GOING_DOWN:
>  	case NETDEV_DOWN:
>  	case NETDEV_UP:
>  	case NETDEV_PRE_UP:
> +		break;
>  	default:
>  		return NOTIFY_DONE;
>  	}
> @@ -62,6 +210,24 @@ static struct notifier_block ovpn_netdev_notifier = {
>  	.notifier_call = ovpn_netdev_notifier_call,
>  };
>  
> +static void ovpn_netns_pre_exit(struct net *net)
> +{
> +	struct ovpn_struct *ovpn;
> +
> +	rtnl_lock();
> +	list_for_each_entry(ovpn, &dev_list, dev_list) {
> +		if (dev_net(ovpn->dev) != net)
> +			continue;
> +
> +		ovpn_iface_destruct(ovpn);

Is this needed? On netns destruction all devices within the ns will be
destroyed by the networking core.

> +	}
> +	rtnl_unlock();
> +}

-- 
Sabrina