[Devel] [PATCH RH8] ve/net: fix crash in ve_dev_can_rename

[Pavel Tikhomirov]

On 05.10.2021 15:31, Pavel Tikhomirov wrote:
> Add ve_get_net_ns helper to get network namespace owned by ve. Using
> rcu_read_lock to protect ve->ve_ns nsproxy from being freed. Referencing
> netns also protects from any kind of netns reuse when comparing netns
> pointers.
> 
> Previousely if we rename network device in netns owned by ve which is
> not in started state we get crash on dereferencing ve->ve_ns.
> 
> https://jira.sw.ru/browse/PSBM-133993

Fixes: 0460650a21fa ("ve/net: allow to rename devices in non-ve namespaces")

> Signed-off-by: Pavel Tikhomirov <ptikhomirov at virtuozzo.com>
> ---
>   include/linux/ve.h |  2 ++
>   kernel/ve/ve.c     | 15 +++++++++++++++
>   net/core/dev.c     | 15 +++++++++------
>   3 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/ve.h b/include/linux/ve.h
> index 46b18e1535b6..c75718017d9d 100644
> --- a/include/linux/ve.h
> +++ b/include/linux/ve.h
> @@ -221,6 +221,8 @@ extern int vz_security_protocol_check(struct net *net, int protocol);
>   
>   int ve_net_hide_sysctl(struct net *net);
>   
> +extern struct net *ve_get_net_ns(struct ve_struct* ve);
> +
>   #else	/* CONFIG_VE */
>   #define get_ve(ve)	(NULL)
>   #define put_ve(ve)	do { } while (0)
> diff --git a/kernel/ve/ve.c b/kernel/ve/ve.c
> index 9bccdd90037d..56a535d840c0 100644
> --- a/kernel/ve/ve.c
> +++ b/kernel/ve/ve.c
> @@ -30,6 +30,7 @@
>   #include <linux/task_work.h>
>   #include <linux/ctype.h>
>   #include <linux/tty.h>
> +#include <net/net_namespace.h>
>   #include <linux/genhd.h>
>   #include <linux/device.h>
>   
> @@ -276,6 +277,20 @@ int ve_net_hide_sysctl(struct net *net)
>   }
>   EXPORT_SYMBOL(ve_net_hide_sysctl);
>   
> +struct net *ve_get_net_ns(struct ve_struct* ve)
> +{
> +	struct nsproxy *ve_ns;
> +	struct net *net_ns;
> +
> +	rcu_read_lock();
> +	ve_ns = rcu_dereference(ve->ve_ns);
> +	net_ns = ve_ns ? get_net(ve_ns->net_ns) : NULL;
> +	rcu_read_unlock();
> +
> +	return net_ns;
> +}
> +EXPORT_SYMBOL(ve_get_net_ns);
> +
>   int nr_threads_ve(struct ve_struct *ve)
>   {
>           return cgroup_task_count(ve->css.cgroup);
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 01e773ff57c1..4f9849bac212 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1167,19 +1167,19 @@ int dev_get_valid_name(struct net *net, struct net_device *dev,
>   }
>   EXPORT_SYMBOL(dev_get_valid_name);
>   
> +#ifdef CONFIG_VE
>   static bool ve_dev_can_rename(struct net_device *dev)
>   {
>   	struct net *net;
>   	bool can;
>   
> -	/*
> -	 * rcu_read_lock()/unlock won't help here anyway:
> -	 * "can" value can change right after rcu lock is dropped.
> -	 */
> -	net = rcu_dereference_check(dev_net(dev)->owner_ve->ve_ns, 1)->net_ns;
> +	net = ve_get_net_ns(dev_net(dev)->owner_ve);
>   	can = !net || net == dev_net(dev);
> +	if (net)
> +		put_net(net);
>   	return can;
>   }
> +#endif
>   
>   /**
>    *	dev_change_name - change name of a device
> @@ -1226,9 +1226,10 @@ int dev_change_name(struct net_device *dev, const char *newname)
>   	dev->name_assign_type = NET_NAME_RENAMED;
>   
>   rollback:
> -
> +#ifdef CONFIG_VE
>   	if (!ve_dev_can_rename(dev))
>   		goto skip_rename;
> +#endif
>   
>   	ret = device_rename(&dev->dev, dev->name);
>   	if (ret) {
> @@ -1238,7 +1239,9 @@ int dev_change_name(struct net_device *dev, const char *newname)
>   		return ret;
>   	}
>   
> +#ifdef CONFIG_VE
>   skip_rename:
> +#endif
>   	up_write(&devnet_rename_sem);
>   
>   	netdev_adjacent_rename_links(dev, oldname);
> 

-- 
Best regards, Tikhomirov Pavel
Software Developer, Virtuozzo.