[v4] kni: fix device address set
Checks
Commit Message
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>
---
v3,v4:
* fix compiling errors.
v2:
* change 'memcpy' to 'ether_addr_copy' to copy device addr.
---
kernel/linux/kni/kni_misc.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
Comments
On 6/1/22 04:59, 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>
> ---
> v3,v4:
> * fix compiling errors.
>
> v2:
> * change 'memcpy' to 'ether_addr_copy' to copy device addr.
> ---
> 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..9c5d8c5576 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);
Isn't it better to use function provided by the kernel, if available?
I mean eth_hw_addr_random() used in [1]
[1]
https://patches.dpdk.org/project/dpdk/patch/20220601054525.7573-1-ke1x.zhang@intel.com/
> +#if KERNEL_VERSION(5, 17, 0) > LINUX_VERSION_CODE
I think usage of HAVE_* defines like in [1] look much better.
It is easier to read and will limit scope if condition is finally
more complicated.
[1]
https://patches.dpdk.org/project/dpdk/patch/20220525102641.20982-1-jslaby@suse.cz/
> + ether_addr_copy(net_dev->dev_addr, mac_addr);
> +#else
> + dev_addr_set(net_dev, mac_addr);
> +#endif
>
> if (dev_info.mtu)
> net_dev->mtu = dev_info.mtu;
Hi, Andrew,
在 2022/6/1 17:02, Andrew Rybchenko 写道:
> On 6/1/22 04:59, 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>
>> ---
>> v3,v4:
>> * fix compiling errors.
>>
>> v2:
>> * change 'memcpy' to 'ether_addr_copy' to copy device addr.
>> ---
>> 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..9c5d8c5576 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);
>
> Isn't it better to use function provided by the kernel, if available?
> I mean eth_hw_addr_random() used in [1]
Well, here 'mac_addr' is my object, this object will be used in
'dev_addr_set' in the next code.
using eth_hw_addr_random will directly operate net-dev->dev_addr.
>
> [1]
> https://patches.dpdk.org/project/dpdk/patch/20220601054525.7573-1-ke1x.zhang@intel.com/
>
>
>> +#if KERNEL_VERSION(5, 17, 0) > LINUX_VERSION_CODE
>
> I think usage of HAVE_* defines like in [1] look much better.
> It is easier to read and will limit scope if condition is finally
> more complicated.
Yes, fixed in v5.
>
> [1]
> https://patches.dpdk.org/project/dpdk/patch/20220525102641.20982-1-jslaby@suse.cz/
>
>
>> + ether_addr_copy(net_dev->dev_addr, mac_addr);
>> +#else
>> + dev_addr_set(net_dev, mac_addr);
>> +#endif
>> if (dev_info.mtu)
>> net_dev->mtu = dev_info.mtu;
>
> .
@@ -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);
+#if KERNEL_VERSION(5, 17, 0) > LINUX_VERSION_CODE
+ ether_addr_copy(net_dev->dev_addr, mac_addr);
+#else
+ dev_addr_set(net_dev, mac_addr);
+#endif
if (dev_info.mtu)
net_dev->mtu = dev_info.mtu;