Re: [PATCH v5] kni: fix device address set

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 6/2/22 09:54, Min Hu (Connor) wrote:
> Currently, run KNI APP When Kernel version is 5.17. When quit the APP,
> dmesg buffer get calltrace, info like:
> [ 5965.847401] rte_kni: Creating kni...
> [ 6225.627205] vEth0 (unregistered): Current addr:  70 fd 45 d0 72 a7 00..
> [ 6225.640113] vEth0 (unregistered): Expected addr: 00 00 00 00 00 00 00..
> [ 6225.653010] ------------[ cut here ]------------
> [ 6225.657609] netdevice: vEth0 (unregistered): Incorrect netdev->dev_addr
> [ 6225.832647] Call trace:
> [ 6225.835083]  dev_addr_check+0xa0/0x144
> [ 6225.838816]  dev_addr_flush+0x30/0x9c
> [ 6225.842462]  free_netdev+0x8c/0x1e0
> [ 6225.845939]  kni_release+0xc0/0x1d0 [rte_kni]
> [ 6225.850281]  __fput+0x78/0x220
> [ 6225.853327]  ____fput+0x1c/0x30
> [ 6225.856455]  task_work_run+0x88/0xc0
> [ 6225.860017]  do_exit+0x2fc/0x940
> [ 6225.863232]  do_group_exit+0x40/0xac
> [ 6225.866791]  get_signal+0x190/0x960
> [ 6225.870265]  do_notify_resume+0x26c/0x1360
> [ 6225.874346]  el0_interrupt+0x60/0xe0
> [ 6225.877910]  __el0_irq_handler_common+0x18/0x24
> [ 6225.882420]  el0t_64_irq_handler+0x14/0x20
> [ 6225.886499]  el0t_64_irq+0x1a0/0x1a4
> [ 6225.890059] ---[ end trace 0000000000000000 ]---
> [ 6245.598157] rte_kni: Creating kni...
> 
> The reason is that 5.17 kernel introduce 'dev_addr_shadow' in function
> 'dev_addr_set' to ensure that netdev->dev_addr should only be modified
> via helpers('dev_addr_set'). 'dev_addr_check' will check if
> netdev->dev_addr is modified by other ways, like 'memcpy'.
> 
> More info could get by referring to kernel patch:
> https://patchwork.kernel.org/project/netdevbpf/patch/
> 20211118041501.3102861-8-kuba@kernel.org/
> https://www.spinics.net/lists/netdev/msg764992.html
> 
> Fixes: ea6b39b5b847 ("kni: remove ethtool support")
> 
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
> v5:
> * define 'HAVE_DEV_ADDR_SHADOW'.
> 
> v3,v4:
> * fix compiling errors.
> 
> v2:
> * change 'memcpy' to 'ether_addr_copy' to copy device addr.
> ---
>   kernel/linux/kni/compat.h   |  4 ++++
>   kernel/linux/kni/kni_misc.c | 10 ++++++++--
>   2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/linux/kni/compat.h b/kernel/linux/kni/compat.h
> index 664785674f..7ffca63bd4 100644
> --- a/kernel/linux/kni/compat.h
> +++ b/kernel/linux/kni/compat.h
> @@ -141,3 +141,7 @@
>   #if KERNEL_VERSION(5, 9, 0) > LINUX_VERSION_CODE
>   #define HAVE_TSK_IN_GUP
>   #endif
> +
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(5, 17, 0)
> +#define HAVE_DEV_ADDR_SHADOW
> +#endif
> diff --git a/kernel/linux/kni/kni_misc.c b/kernel/linux/kni/kni_misc.c
> index 780187d8bf..0470d349c5 100644
> --- a/kernel/linux/kni/kni_misc.c
> +++ b/kernel/linux/kni/kni_misc.c
> @@ -299,6 +299,7 @@ kni_ioctl_create(struct net *net, uint32_t ioctl_num,
>   	struct kni_net *knet = net_generic(net, kni_net_id);
>   	int ret;
>   	struct rte_kni_device_info dev_info;
> +	unsigned char mac_addr[ETH_ALEN];
>   	struct net_device *net_dev = NULL;
>   	struct kni_dev *kni, *dev, *n;
>   
> @@ -403,10 +404,15 @@ kni_ioctl_create(struct net *net, uint32_t ioctl_num,
>   
>   	/* if user has provided a valid mac address */
>   	if (is_valid_ether_addr(dev_info.mac_addr))
> -		memcpy(net_dev->dev_addr, dev_info.mac_addr, ETH_ALEN);
> +		ether_addr_copy(mac_addr, dev_info.mac_addr);
>   	else
>   		/* Generate random MAC address. */
> -		eth_random_addr(net_dev->dev_addr);
> +		eth_random_addr(mac_addr);

The following point from Stephen is still not processed:

<quote>
This is still not the best way to handle this.
If you use eth_hw_addr_random() it will set the address assign type. 
Right now KNI is not doing this correctly.
</quote>

> +#ifdef HAVE_DEV_ADDR_SHADOW
> +	dev_addr_set(net_dev, mac_addr);
> +#else
> +	ether_addr_copy(net_dev->dev_addr, mac_addr);
> +#endif
>   
>   	if (dev_info.mtu)
>   		net_dev->mtu = dev_info.mtu;