diff mbox series

kni: fix device address set

Message ID 20220406082213.45750-1-humin29@huawei.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series kni: fix device address set |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/intel-Testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/github-robot: build success github build: passed
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-abi-testing success Testing PASS

Commit Message

humin (Q) April 6, 2022, 8:22 a.m. UTC 
  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>
---
 kernel/linux/kni/kni_misc.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Stephen Hemminger April 6, 2022, 3:17 p.m. UTC | #1 
On Wed, 6 Apr 2022 16:22:13 +0800
"Min Hu (Connor)" <humin29@huawei.com> wrote:

>  	/* 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);
> +		memcpy(mac_addr, dev_info.mac_addr, ETH_ALEN);

Minor enhancement. Could this use ether_addr_copy instead?
		ether_addr_copy(mac_addr, dev_info.mac_addr);
humin (Q) April 7, 2022, 12:44 a.m. UTC | #2 
Hi, Stephen,
  I think this is a good option, but the macro definition is like:
+#if LINUX_VERSION_CODE < KERNEL_VERSION(3, 14, 0)
+#define ether_addr_copy(dst, src) memcpy(dst, src, ETH_ALEN)
+#endif

@Ferry, why is it limited for "LINUX_VERSION_CODE < KERNEL_VERSION(3, 
14, 0)" ?


在 2022/4/6 23:17, Stephen Hemminger 写道:
> On Wed, 6 Apr 2022 16:22:13 +0800
> "Min Hu (Connor)" <humin29@huawei.com> wrote:
> 
>>   	/* 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);
>> +		memcpy(mac_addr, dev_info.mac_addr, ETH_ALEN);
> 
> Minor enhancement. Could this use ether_addr_copy instead?
> 		ether_addr_copy(mac_addr, dev_info.mac_addr);
> .
>
Stephen Hemminger April 7, 2022, 3:18 a.m. UTC | #3 
On Thu, 7 Apr 2022 08:44:23 +0800
"Min Hu (Connor)" <humin29@huawei.com> wrote:

> Hi, Stephen,
>   I think this is a good option, but the macro definition is like:
> +#if LINUX_VERSION_CODE < KERNEL_VERSION(3, 14, 0)
> +#define ether_addr_copy(dst, src) memcpy(dst, src, ETH_ALEN)
> +#endif

Minimal supported DPDK version is 4.4 now. So this is not a problem.

Note: 4.4 kernel reached end of support window in Febrary 2022.
It is supported by the SLTS project but it would be unwise
to use later DPDK on a kernel that is stuck being supported until 2036.

Apparently my patch to update it to current LTS 4.9 is sitting
stuck in patchwork.
humin (Q) April 7, 2022, 6:21 a.m. UTC | #4 
Hi,

在 2022/4/7 11:18, Stephen Hemminger 写道:
> On Thu, 7 Apr 2022 08:44:23 +0800
> "Min Hu (Connor)" <humin29@huawei.com> wrote:
> 
>> Hi, Stephen,
>>    I think this is a good option, but the macro definition is like:
>> +#if LINUX_VERSION_CODE < KERNEL_VERSION(3, 14, 0)
>> +#define ether_addr_copy(dst, src) memcpy(dst, src, ETH_ALEN)
>> +#endif
> 
> Minimal supported DPDK version is 4.4 now. So this is not a problem.
I did not catch your meaning. I think this is a problem.
Acording to current definition, we can only use "ether_addr_copy" on 
linux platform of version 3.13.0(or below).

How about the situaction on other linux version, like 3.14.0(or upper)?


> 
> Note: 4.4 kernel reached end of support window in Febrary 2022.
> It is supported by the SLTS project but it would be unwise
> to use later DPDK on a kernel that is stuck being supported until 2036.
> 
> Apparently my patch to update it to current LTS 4.9 is sitting
> stuck in patchwork.
> .
>
Thomas Monjalon April 7, 2022, 7:42 a.m. UTC | #5 
07/04/2022 02:44, Min Hu (Connor):
> Hi, Stephen,
>   I think this is a good option, but the macro definition is like:
> +#if LINUX_VERSION_CODE < KERNEL_VERSION(3, 14, 0)
> +#define ether_addr_copy(dst, src) memcpy(dst, src, ETH_ALEN)
> +#endif
> 
> @Ferry, why is it limited for "LINUX_VERSION_CODE < KERNEL_VERSION(3, 
> 14, 0)" ?

I guess that's because it is defined in "new kernels" so we need
a definition in DPDK for old kernels.
humin (Q) April 7, 2022, 8:08 a.m. UTC | #6 
OK, I see, thanks Thomas.

在 2022/4/7 15:42, Thomas Monjalon 写道:
> 07/04/2022 02:44, Min Hu (Connor):
>> Hi, Stephen,
>>    I think this is a good option, but the macro definition is like:
>> +#if LINUX_VERSION_CODE < KERNEL_VERSION(3, 14, 0)
>> +#define ether_addr_copy(dst, src) memcpy(dst, src, ETH_ALEN)
>> +#endif
>>
>> @Ferry, why is it limited for "LINUX_VERSION_CODE < KERNEL_VERSION(3,
>> 14, 0)" ?
> 
> I guess that's because it is defined in "new kernels" so we need
> a definition in DPDK for old kernels.
> 
> 
> 
> .
>
Stephen Hemminger June 2, 2022, 3:31 p.m. UTC | #7 
On Wed, 6 Apr 2022 16:22:13 +0800
"Min Hu (Connor)" <humin29@huawei.com> 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>
> ---
>  kernel/linux/kni/kni_misc.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/linux/kni/kni_misc.c b/kernel/linux/kni/kni_misc.c
> index 780187d8bf..046de0311f 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);
> +		memcpy(mac_addr, dev_info.mac_addr, ETH_ALEN);
>  	else
>  		/* Generate random MAC address. */
> -		eth_random_addr(net_dev->dev_addr);
> +		eth_random_addr(mac_addr);
> +#if KERNEL_VERSION(5, 17, 0) > LINUX_VERSION_CODE
> +	memcpy(net_dev->dev_addr, mac_addr, ETH_ALEN);
> +#else
> +	dev_addr_set(net_dev, mac_addr);
> +#endif

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.

Also, the code is more readable if all the kernel compatibility
stuff is handled in compat.h. There should be no kernel version
checks in the functions.
diff mbox series

Patch

diff --git a/kernel/linux/kni/kni_misc.c b/kernel/linux/kni/kni_misc.c
index 780187d8bf..046de0311f 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);
+		memcpy(mac_addr, dev_info.mac_addr, ETH_ALEN);
 	else
 		/* Generate random MAC address. */
-		eth_random_addr(net_dev->dev_addr);
+		eth_random_addr(mac_addr);
+#if KERNEL_VERSION(5, 17, 0) > LINUX_VERSION_CODE
+	memcpy(net_dev->dev_addr, mac_addr, ETH_ALEN);
+#else
+	dev_addr_set(net_dev, mac_addr);
+#endif
 
 	if (dev_info.mtu)
 		net_dev->mtu = dev_info.mtu;