| Message ID | 20240709063039.2909536-12-maxime.chevallier@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce PHY listing and link_topology tracking | expand |
Le 09/07/2024 à 08:30, Maxime Chevallier a écrit : > Cable testing is a PHY-specific command. Instead of targeting the command > towards dev->phydev, use the request to pick the targeted PHY. > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu> Tested-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > net/ethtool/cabletest.c | 35 ++++++++++++++++++++++------------- > 1 file changed, 22 insertions(+), 13 deletions(-) > > diff --git a/net/ethtool/cabletest.c b/net/ethtool/cabletest.c > index f6f136ec7ddf..01db8f394869 100644 > --- a/net/ethtool/cabletest.c > +++ b/net/ethtool/cabletest.c > @@ -13,7 +13,7 @@ > > const struct nla_policy ethnl_cable_test_act_policy[] = { > [ETHTOOL_A_CABLE_TEST_HEADER] = > - NLA_POLICY_NESTED(ethnl_header_policy), > + NLA_POLICY_NESTED(ethnl_header_policy_phy), > }; > > static int ethnl_cable_test_started(struct phy_device *phydev, u8 cmd) > @@ -58,6 +58,7 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info) > struct ethnl_req_info req_info = {}; > const struct ethtool_phy_ops *ops; > struct nlattr **tb = info->attrs; > + struct phy_device *phydev; > struct net_device *dev; > int ret; > > @@ -69,12 +70,16 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info) > return ret; > > dev = req_info.dev; > - if (!dev->phydev) { > + > + rtnl_lock(); > + phydev = ethnl_req_get_phydev(&req_info, > + tb[ETHTOOL_A_CABLE_TEST_HEADER], > + info->extack); > + if (IS_ERR_OR_NULL(phydev)) { > ret = -EOPNOTSUPP; > goto out_dev_put; > } > > - rtnl_lock(); > ops = ethtool_phy_ops; > if (!ops || !ops->start_cable_test) { > ret = -EOPNOTSUPP; > @@ -85,13 +90,12 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info) > if (ret < 0) > goto out_rtnl; > > - ret = ops->start_cable_test(dev->phydev, info->extack); > + ret = ops->start_cable_test(phydev, info->extack); > > ethnl_ops_complete(dev); > > if (!ret) > - ethnl_cable_test_started(dev->phydev, > - ETHTOOL_MSG_CABLE_TEST_NTF); > + ethnl_cable_test_started(phydev, ETHTOOL_MSG_CABLE_TEST_NTF); > > out_rtnl: > rtnl_unlock(); > @@ -216,7 +220,7 @@ static const struct nla_policy cable_test_tdr_act_cfg_policy[] = { > > const struct nla_policy ethnl_cable_test_tdr_act_policy[] = { > [ETHTOOL_A_CABLE_TEST_TDR_HEADER] = > - NLA_POLICY_NESTED(ethnl_header_policy), > + NLA_POLICY_NESTED(ethnl_header_policy_phy), > [ETHTOOL_A_CABLE_TEST_TDR_CFG] = { .type = NLA_NESTED }, > }; > > @@ -305,6 +309,7 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info) > struct ethnl_req_info req_info = {}; > const struct ethtool_phy_ops *ops; > struct nlattr **tb = info->attrs; > + struct phy_device *phydev; > struct phy_tdr_config cfg; > struct net_device *dev; > int ret; > @@ -317,10 +322,6 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info) > return ret; > > dev = req_info.dev; > - if (!dev->phydev) { > - ret = -EOPNOTSUPP; > - goto out_dev_put; > - } > > ret = ethnl_act_cable_test_tdr_cfg(tb[ETHTOOL_A_CABLE_TEST_TDR_CFG], > info, &cfg); > @@ -328,6 +329,14 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info) > goto out_dev_put; > > rtnl_lock(); > + phydev = ethnl_req_get_phydev(&req_info, > + tb[ETHTOOL_A_CABLE_TEST_TDR_HEADER], > + info->extack); > + if (!IS_ERR_OR_NULL(phydev)) { > + ret = -EOPNOTSUPP; > + goto out_dev_put; > + } > + > ops = ethtool_phy_ops; > if (!ops || !ops->start_cable_test_tdr) { > ret = -EOPNOTSUPP; > @@ -338,12 +347,12 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info) > if (ret < 0) > goto out_rtnl; > > - ret = ops->start_cable_test_tdr(dev->phydev, info->extack, &cfg); > + ret = ops->start_cable_test_tdr(phydev, info->extack, &cfg); > > ethnl_ops_complete(dev); > > if (!ret) > - ethnl_cable_test_started(dev->phydev, > + ethnl_cable_test_started(phydev, > ETHTOOL_MSG_CABLE_TEST_TDR_NTF); > > out_rtnl:
Hi Maxime Chevallier, I used syzkaller and found that: there was general protection fault in phy_start_cable_test_tdr in Linux next:next-20240826. Bisected and found first bad commit: " 3688ff3077d3 net: ethtool: cable-test: Target the command to the requested PHY " After reverted below commit on top of next-20240826, this issue was gone. All detailed info: https://github.com/xupengfe/syzkaller_logs/tree/main/240826_202302_phy_start_cable_test_tdr Syzkaller repro code: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/repro.c Syzkaller repro syscall steps: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/repro.prog Syzkaller report: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/repro.report Kconfig(make olddefconfig): https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/kconfig_origin Bisect info: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/bisect_info.log bzImage: https://github.com/xupengfe/syzkaller_logs/raw/main/240826_202302_phy_start_cable_test_tdr/bzImage_1ca4237ad9ce29b0c66fe87862f1da54ac56a1e8.tar.gz Issue dmesg: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/1ca4237ad9ce29b0c66fe87862f1da54ac56a1e8_dmesg.log " [ 27.323262] Oops: general protection fault, probably for non-canonical address 0xdffffc00000000fe: 0000 [#1] PREEMPT SMP KASAN NOPTI [ 27.324087] KASAN: null-ptr-deref in range [0x00000000000007f0-0x00000000000007f7] [ 27.324587] CPU: 0 UID: 0 PID: 729 Comm: repro Not tainted 6.11.0-rc5-next-20240826-1ca4237ad9ce-dirty #1 [ 27.325203] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014 [ 27.325931] RIP: 0010:phy_start_cable_test_tdr+0x43/0x690 [ 27.326320] Code: 48 83 ec 20 48 89 55 c0 48 89 75 c8 e8 b6 a6 1e fd 49 8d bc 24 f0 07 00 00 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 9c 05 00 00 4d 8d bc 24 e8 04 00 00 49 8b 9c 24 [ 27.327485] RSP: 0018:ffff888022397370 EFLAGS: 00010202 [ 27.327828] RAX: dffffc0000000000 RBX: ffff888022397560 RCX: 1ffffffff0c6423b [ 27.328291] RDX: 00000000000000fe RSI: ffffffff84482e0a RDI: 00000000000007f0 [ 27.328763] RBP: ffff8880223973b8 R08: 0000000000000000 R09: ffffed1002715815 [ 27.329232] R10: 0000000000000000 R11: 0000000000000005 R12: 0000000000000000 [ 27.329691] R13: 0000000000000000 R14: 0000000000000000 R15: ffffffff84482de0 [ 27.330155] FS: 00007ff6a8b4e740(0000) GS:ffff88806c400000(0000) knlGS:0000000000000000 [ 27.330678] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 27.331060] CR2: 00007ffc26df3eb8 CR3: 0000000020c62001 CR4: 0000000000770ef0 [ 27.331529] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 27.331991] DR3: 0000000000000000 DR6: 00000000ffff07f0 DR7: 0000000000000400 [ 27.332452] PKRU: 55555554 [ 27.332639] Call Trace: [ 27.332808] <TASK> [ 27.332960] ? show_regs+0x6d/0x80 [ 27.333211] ? die_addr+0x45/0xb0 [ 27.333445] ? exc_general_protection+0x1ae/0x340 [ 27.333780] ? asm_exc_general_protection+0x2b/0x30 [ 27.334120] ? __pfx_phy_start_cable_test_tdr+0x10/0x10 [ 27.334473] ? phy_start_cable_test_tdr+0x2a/0x690 [ 27.334797] ? phy_start_cable_test_tdr+0x43/0x690 [ 27.335120] ? __sanitizer_cov_trace_const_cmp1+0x1e/0x30 [ 27.335489] ? __pfx_phy_start_cable_test_tdr+0x10/0x10 [ 27.335834] ethnl_act_cable_test_tdr+0x718/0xe70 [ 27.336161] ? __pfx_ethnl_act_cable_test_tdr+0x10/0x10 [ 27.336513] ? debug_smp_processor_id+0x20/0x30 [ 27.336822] ? __nla_parse+0x49/0x60 [ 27.337087] ? __sanitizer_cov_trace_const_cmp4+0x1a/0x20 [ 27.337446] ? genl_family_rcv_msg_attrs_parse.constprop.0+0xbc/0x2a0 [ 27.337877] genl_family_rcv_msg_doit+0x22f/0x330 [ 27.338192] ? __pfx_genl_family_rcv_msg_doit+0x10/0x10 [ 27.338543] ? cap_capable+0x1d3/0x240 [ 27.338813] ? __sanitizer_cov_trace_const_cmp4+0x1a/0x20 [ 27.339173] ? ns_capable+0xec/0x130 [ 27.339425] genl_rcv_msg+0x582/0x850 [ 27.339683] ? __pfx_genl_rcv_msg+0x10/0x10 [ 27.339964] ? __pfx_ethnl_act_cable_test_tdr+0x10/0x10 [ 27.340313] ? __this_cpu_preempt_check+0x21/0x30 [ 27.340632] ? lock_acquire.part.0+0x152/0x390 [ 27.340943] netlink_rcv_skb+0x187/0x470 [ 27.341216] ? __pfx_genl_rcv_msg+0x10/0x10 [ 27.341498] ? __pfx_netlink_rcv_skb+0x10/0x10 [ 27.341809] ? __pfx_down_read+0x10/0x10 [ 27.342079] ? netlink_deliver_tap+0x1b9/0xca0 [ 27.342388] genl_rcv+0x32/0x50 [ 27.342607] netlink_unicast+0x5a3/0x870 [ 27.342878] ? __pfx_netlink_unicast+0x10/0x10 [ 27.343184] ? __sanitizer_cov_trace_cmp8+0x1c/0x30 [ 27.343514] ? __check_object_size+0x43/0x8e0 [ 27.343821] netlink_sendmsg+0x956/0xe80 [ 27.344096] ? __pfx_netlink_sendmsg+0x10/0x10 [ 27.344401] ? __import_iovec+0x1f5/0x6c0 [ 27.344686] ? __might_fault+0xf1/0x1b0 [ 27.344961] ? __pfx_netlink_sendmsg+0x10/0x10 [ 27.345268] ____sys_sendmsg+0xaba/0xc90 [ 27.345544] ? __pfx_____sys_sendmsg+0x10/0x10 [ 27.345848] ? lock_release+0x441/0x870 [ 27.346115] ___sys_sendmsg+0x122/0x1c0 [ 27.346386] ? __pfx____sys_sendmsg+0x10/0x10 [ 27.346689] ? __pfx___lock_acquire+0x10/0x10 [ 27.346987] ? __sanitizer_cov_trace_const_cmp8+0x1c/0x30 [ 27.347351] ? put_user_ifreq+0xa5/0xc0 [ 27.347614] ? __this_cpu_preempt_check+0x21/0x30 [ 27.347933] ? __sanitizer_cov_trace_const_cmp4+0x1a/0x20 [ 27.348294] ? fdget+0x188/0x230 [ 27.348531] ? __sanitizer_cov_trace_const_cmp8+0x1c/0x30 [ 27.348895] __sys_sendmsg+0x11f/0x200 [ 27.349157] ? __pfx___sys_sendmsg+0x10/0x10 [ 27.349453] ? __this_cpu_preempt_check+0x21/0x30 [ 27.349777] ? __audit_syscall_entry+0x39c/0x500 [ 27.350096] __x64_sys_sendmsg+0x80/0xc0 [ 27.350371] ? syscall_trace_enter+0x14a/0x230 [ 27.350678] x64_sys_call+0x932/0x2140 [ 27.350941] do_syscall_64+0x6d/0x140 [ 27.351199] entry_SYSCALL_64_after_hwframe+0x76/0x7e [ 27.351543] RIP: 0033:0x7ff6a883ee5d [ 27.351791] Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 93 af 1b 00 f7 d8 64 89 01 48 [ 27.352965] RSP: 002b:00007ffc26df4f28 EFLAGS: 00000282 ORIG_RAX: 000000000000002e [ 27.353457] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007ff6a883ee5d [ 27.353916] RDX: 0000000000000000 RSI: 00000000200003c0 RDI: 0000000000000003 [ 27.354429] RBP: 00007ffc26df4f40 R08: 000000000000000c R09: 000000000000000c [ 27.354874] R10: 000000000000000c R11: 0000000000000282 R12: 00007ffc26df5088 [ 27.355321] R13: 000000000040257c R14: 0000000000404e08 R15: 00007ff6a8b99000 [ 27.355775] </TASK> [ 27.355923] Modules linked in: [ 27.356255] ---[ end trace 0000000000000000 ]--- " I hope it's helpful. --- If you don't need the following environment to reproduce the problem or if you already have one reproduced environment, please ignore the following information. How to reproduce: git clone https://gitlab.com/xupengfe/repro_vm_env.git cd repro_vm_env tar -xvf repro_vm_env.tar.gz cd repro_vm_env; ./start3.sh // it needs qemu-system-x86_64 and I used v7.1.0 // start3.sh will load bzImage_2241ab53cbb5cdb08a6b2d4688feb13971058f65 v6.2-rc5 kernel // You could change the bzImage_xxx as you want // Maybe you need to remove line "-drive if=pflash,format=raw,readonly=on,file=./OVMF_CODE.fd \" for different qemu version You could use below command to log in, there is no password for root. ssh -p 10023 root@localhost After login vm(virtual machine) successfully, you could transfer reproduced binary to the vm by below way, and reproduce the problem in vm: gcc -pthread -o repro repro.c scp -P 10023 repro root@localhost:/root/ Get the bzImage for target kernel: Please use target kconfig and copy it to kernel_src/.config make olddefconfig make -jx bzImage //x should equal or less than cpu num your pc has Fill the bzImage file into above start3.sh to load the target kernel in vm. Tips: If you already have qemu-system-x86_64, please ignore below info. If you want to install qemu v7.1.0 version: git clone https://github.com/qemu/qemu.git cd qemu git checkout -f v7.1.0 mkdir build cd build yum install -y ninja-build.x86_64 yum -y install libslirp-devel.x86_64 ../configure --target-list=x86_64-softmmu --enable-kvm --enable-vnc --enable-gtk --enable-sdl --enable-usb-redir --enable-slirp make make install Best Regards, Thanks! On 2024-07-09 at 08:30:34 +0200, Maxime Chevallier wrote: > Cable testing is a PHY-specific command. Instead of targeting the command > towards dev->phydev, use the request to pick the targeted PHY. > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > --- > net/ethtool/cabletest.c | 35 ++++++++++++++++++++++------------- > 1 file changed, 22 insertions(+), 13 deletions(-) > > diff --git a/net/ethtool/cabletest.c b/net/ethtool/cabletest.c > index f6f136ec7ddf..01db8f394869 100644 > --- a/net/ethtool/cabletest.c > +++ b/net/ethtool/cabletest.c > @@ -13,7 +13,7 @@ > > const struct nla_policy ethnl_cable_test_act_policy[] = { > [ETHTOOL_A_CABLE_TEST_HEADER] = > - NLA_POLICY_NESTED(ethnl_header_policy), > + NLA_POLICY_NESTED(ethnl_header_policy_phy), > }; > > static int ethnl_cable_test_started(struct phy_device *phydev, u8 cmd) > @@ -58,6 +58,7 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info) > struct ethnl_req_info req_info = {}; > const struct ethtool_phy_ops *ops; > struct nlattr **tb = info->attrs; > + struct phy_device *phydev; > struct net_device *dev; > int ret; > > @@ -69,12 +70,16 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info) > return ret; > > dev = req_info.dev; > - if (!dev->phydev) { > + > + rtnl_lock(); > + phydev = ethnl_req_get_phydev(&req_info, > + tb[ETHTOOL_A_CABLE_TEST_HEADER], > + info->extack); > + if (IS_ERR_OR_NULL(phydev)) { > ret = -EOPNOTSUPP; > goto out_dev_put; > } > > - rtnl_lock(); > ops = ethtool_phy_ops; > if (!ops || !ops->start_cable_test) { > ret = -EOPNOTSUPP; > @@ -85,13 +90,12 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info) > if (ret < 0) > goto out_rtnl; > > - ret = ops->start_cable_test(dev->phydev, info->extack); > + ret = ops->start_cable_test(phydev, info->extack); > > ethnl_ops_complete(dev); > > if (!ret) > - ethnl_cable_test_started(dev->phydev, > - ETHTOOL_MSG_CABLE_TEST_NTF); > + ethnl_cable_test_started(phydev, ETHTOOL_MSG_CABLE_TEST_NTF); > > out_rtnl: > rtnl_unlock(); > @@ -216,7 +220,7 @@ static const struct nla_policy cable_test_tdr_act_cfg_policy[] = { > > const struct nla_policy ethnl_cable_test_tdr_act_policy[] = { > [ETHTOOL_A_CABLE_TEST_TDR_HEADER] = > - NLA_POLICY_NESTED(ethnl_header_policy), > + NLA_POLICY_NESTED(ethnl_header_policy_phy), > [ETHTOOL_A_CABLE_TEST_TDR_CFG] = { .type = NLA_NESTED }, > }; > > @@ -305,6 +309,7 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info) > struct ethnl_req_info req_info = {}; > const struct ethtool_phy_ops *ops; > struct nlattr **tb = info->attrs; > + struct phy_device *phydev; > struct phy_tdr_config cfg; > struct net_device *dev; > int ret; > @@ -317,10 +322,6 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info) > return ret; > > dev = req_info.dev; > - if (!dev->phydev) { > - ret = -EOPNOTSUPP; > - goto out_dev_put; > - } > > ret = ethnl_act_cable_test_tdr_cfg(tb[ETHTOOL_A_CABLE_TEST_TDR_CFG], > info, &cfg); > @@ -328,6 +329,14 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info) > goto out_dev_put; > > rtnl_lock(); > + phydev = ethnl_req_get_phydev(&req_info, > + tb[ETHTOOL_A_CABLE_TEST_TDR_HEADER], > + info->extack); > + if (!IS_ERR_OR_NULL(phydev)) { > + ret = -EOPNOTSUPP; > + goto out_dev_put; > + } > + > ops = ethtool_phy_ops; > if (!ops || !ops->start_cable_test_tdr) { > ret = -EOPNOTSUPP; > @@ -338,12 +347,12 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info) > if (ret < 0) > goto out_rtnl; > > - ret = ops->start_cable_test_tdr(dev->phydev, info->extack, &cfg); > + ret = ops->start_cable_test_tdr(phydev, info->extack, &cfg); > > ethnl_ops_complete(dev); > > if (!ret) > - ethnl_cable_test_started(dev->phydev, > + ethnl_cable_test_started(phydev, > ETHTOOL_MSG_CABLE_TEST_TDR_NTF); > > out_rtnl: > -- > 2.45.1 >
Hi, On Tue, 27 Aug 2024 13:25:52 +0800 Pengfei Xu <pengfei.xu@intel.com> wrote: > Hi Maxime Chevallier, > > I used syzkaller and found that: there was general protection fault in > phy_start_cable_test_tdr in Linux next:next-20240826. > > Bisected and found first bad commit: > " > 3688ff3077d3 net: ethtool: cable-test: Target the command to the requested PHY > " > After reverted below commit on top of next-20240826, this issue was gone. > > All detailed info: https://github.com/xupengfe/syzkaller_logs/tree/main/240826_202302_phy_start_cable_test_tdr > Syzkaller repro code: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/repro.c > Syzkaller repro syscall steps: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/repro.prog > Syzkaller report: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/repro.report > Kconfig(make olddefconfig): https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/kconfig_origin > Bisect info: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/bisect_info.log > bzImage: https://github.com/xupengfe/syzkaller_logs/raw/main/240826_202302_phy_start_cable_test_tdr/bzImage_1ca4237ad9ce29b0c66fe87862f1da54ac56a1e8.tar.gz > Issue dmesg: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/1ca4237ad9ce29b0c66fe87862f1da54ac56a1e8_dmesg.log Thanks for the report ! This issue has indeed been detected, and is being addressed, see : https://lore.kernel.org/netdev/20240826134656.94892-1-djahchankoike@gmail.com/ Thanks, Maxime
Hi Maxime Chevallier, On 2024-08-27 at 07:33:59 +0200, Maxime Chevallier wrote: > Hi, > > On Tue, 27 Aug 2024 13:25:52 +0800 > Pengfei Xu <pengfei.xu@intel.com> wrote: > > > Hi Maxime Chevallier, > > > > I used syzkaller and found that: there was general protection fault in > > phy_start_cable_test_tdr in Linux next:next-20240826. > > > > Bisected and found first bad commit: > > " > > 3688ff3077d3 net: ethtool: cable-test: Target the command to the requested PHY > > " > > After reverted below commit on top of next-20240826, this issue was gone. > > > > All detailed info: https://github.com/xupengfe/syzkaller_logs/tree/main/240826_202302_phy_start_cable_test_tdr > > Syzkaller repro code: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/repro.c > > Syzkaller repro syscall steps: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/repro.prog > > Syzkaller report: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/repro.report > > Kconfig(make olddefconfig): https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/kconfig_origin > > Bisect info: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/bisect_info.log > > bzImage: https://github.com/xupengfe/syzkaller_logs/raw/main/240826_202302_phy_start_cable_test_tdr/bzImage_1ca4237ad9ce29b0c66fe87862f1da54ac56a1e8.tar.gz > > Issue dmesg: https://github.com/xupengfe/syzkaller_logs/blob/main/240826_202302_phy_start_cable_test_tdr/1ca4237ad9ce29b0c66fe87862f1da54ac56a1e8_dmesg.log > > Thanks for the report ! > > This issue has indeed been detected, and is being addressed, see : > > https://lore.kernel.org/netdev/20240826134656.94892-1-djahchankoike@gmail.com/ Thanks for your sharing of the solution! Best Regards, Thanks! > > Thanks, > > Maxime
On Tue, Aug 27, 2024 at 07:33:59AM +0200, Maxime Chevallier wrote: > > This issue has indeed been detected, and is being addressed, see : > > https://lore.kernel.org/netdev/20240826134656.94892-1-djahchankoike@gmail.com/ > There is a similar bug in ethnl_act_cable_test_tdr() that needs to be fixed as well. net/ethtool/cabletest.c 307 int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info) 308 { 309 struct ethnl_req_info req_info = {}; 310 const struct ethtool_phy_ops *ops; 311 struct nlattr **tb = info->attrs; 312 struct phy_device *phydev; 313 struct phy_tdr_config cfg; 314 struct net_device *dev; 315 int ret; 316 317 ret = ethnl_parse_header_dev_get(&req_info, 318 tb[ETHTOOL_A_CABLE_TEST_TDR_HEADER], 319 genl_info_net(info), info->extack, 320 true); 321 if (ret < 0) 322 return ret; 323 324 dev = req_info.dev; 325 326 ret = ethnl_act_cable_test_tdr_cfg(tb[ETHTOOL_A_CABLE_TEST_TDR_CFG], 327 info, &cfg); 328 if (ret) 329 goto out_dev_put; 330 331 rtnl_lock(); ^^^^^^^^^^^^ 332 phydev = ethnl_req_get_phydev(&req_info, 333 tb[ETHTOOL_A_CABLE_TEST_TDR_HEADER], 334 info->extack); 335 if (!IS_ERR_OR_NULL(phydev)) { ^ This test is reversed so it will lead to a crash. Could you add some comments to ethnl_req_get_phydev() what the NULL return means vs the error pointers? I figured it out because the callers have comments but it should be next to ethnl_req_get_phydev() as well. 336 ret = -EOPNOTSUPP; 337 goto out_dev_put; 338 } 339 340 ops = ethtool_phy_ops; 341 if (!ops || !ops->start_cable_test_tdr) { 342 ret = -EOPNOTSUPP; 343 goto out_rtnl; 344 } 345 346 ret = ethnl_ops_begin(dev); 347 if (ret < 0) 348 goto out_rtnl; 349 350 ret = ops->start_cable_test_tdr(phydev, info->extack, &cfg); 351 352 ethnl_ops_complete(dev); 353 354 if (!ret) 355 ethnl_cable_test_started(phydev, 356 ETHTOOL_MSG_CABLE_TEST_TDR_NTF); 357 358 out_rtnl: 359 rtnl_unlock(); 360 out_dev_put: 361 ethnl_parse_header_dev_put(&req_info); 362 return ret; 363 } regards, dan carpenter
Hello Dan, On Tue, 27 Aug 2024 11:27:48 +0300 Dan Carpenter <dan.carpenter@linaro.org> wrote: > On Tue, Aug 27, 2024 at 07:33:59AM +0200, Maxime Chevallier wrote: > > > > This issue has indeed been detected, and is being addressed, see : > > > > https://lore.kernel.org/netdev/20240826134656.94892-1-djahchankoike@gmail.com/ > > > > There is a similar bug in ethnl_act_cable_test_tdr() that needs to be fixed > as well. > > net/ethtool/cabletest.c > 307 int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info) > 308 { > 309 struct ethnl_req_info req_info = {}; > 310 const struct ethtool_phy_ops *ops; > 311 struct nlattr **tb = info->attrs; > 312 struct phy_device *phydev; > 313 struct phy_tdr_config cfg; > 314 struct net_device *dev; > 315 int ret; > 316 > 317 ret = ethnl_parse_header_dev_get(&req_info, > 318 tb[ETHTOOL_A_CABLE_TEST_TDR_HEADER], > 319 genl_info_net(info), info->extack, > 320 true); > 321 if (ret < 0) > 322 return ret; > 323 > 324 dev = req_info.dev; > 325 > 326 ret = ethnl_act_cable_test_tdr_cfg(tb[ETHTOOL_A_CABLE_TEST_TDR_CFG], > 327 info, &cfg); > 328 if (ret) > 329 goto out_dev_put; > 330 > 331 rtnl_lock(); > ^^^^^^^^^^^^ > > 332 phydev = ethnl_req_get_phydev(&req_info, > 333 tb[ETHTOOL_A_CABLE_TEST_TDR_HEADER], > 334 info->extack); > 335 if (!IS_ERR_OR_NULL(phydev)) { > ^ > This test is reversed so it will lead to a crash. > > Could you add some comments to ethnl_req_get_phydev() what the NULL return > means vs the error pointers? I figured it out because the callers have comments > but it should be next to ethnl_req_get_phydev() as well. Good catch ! I'll send some followup to address this report as well as update the doc. Thanks, Maxime
Hi again Dan, On Tue, 27 Aug 2024 11:27:48 +0300 Dan Carpenter <dan.carpenter@linaro.org> wrote: > Could you add some comments to ethnl_req_get_phydev() what the NULL return > means vs the error pointers? I figured it out because the callers have comments > but it should be next to ethnl_req_get_phydev() as well. Actually I replied a bit too fast, this is already documented : https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/net/ethtool/netlink.h#n284 Is this doc clear enough or should I still add some more explicit comments ? Thanks, Maxime
On Tue, Aug 27, 2024 at 10:48:25AM +0200, Maxime Chevallier wrote: > Hi again Dan, > > On Tue, 27 Aug 2024 11:27:48 +0300 > Dan Carpenter <dan.carpenter@linaro.org> wrote: > > > > Could you add some comments to ethnl_req_get_phydev() what the NULL return > > means vs the error pointers? I figured it out because the callers have comments > > but it should be next to ethnl_req_get_phydev() as well. > > Actually I replied a bit too fast, this is already documented : > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/net/ethtool/netlink.h#n284 > > Is this doc clear enough or should I still add some more explicit > comments ? > Ah, I didn't see that. Thanks. That comment is fine but we normally would have put it next to the function implementation instead of the header file. There are lots of comments in the header file, sure, but those are for inline functions so it's the same rule of thumb that the comments are next to the implementation. regards, dan carpenter
diff --git a/net/ethtool/cabletest.c b/net/ethtool/cabletest.c index f6f136ec7ddf..01db8f394869 100644 --- a/net/ethtool/cabletest.c +++ b/net/ethtool/cabletest.c @@ -13,7 +13,7 @@ const struct nla_policy ethnl_cable_test_act_policy[] = { [ETHTOOL_A_CABLE_TEST_HEADER] = - NLA_POLICY_NESTED(ethnl_header_policy), + NLA_POLICY_NESTED(ethnl_header_policy_phy), }; static int ethnl_cable_test_started(struct phy_device *phydev, u8 cmd) @@ -58,6 +58,7 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info) struct ethnl_req_info req_info = {}; const struct ethtool_phy_ops *ops; struct nlattr **tb = info->attrs; + struct phy_device *phydev; struct net_device *dev; int ret; @@ -69,12 +70,16 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info) return ret; dev = req_info.dev; - if (!dev->phydev) { + + rtnl_lock(); + phydev = ethnl_req_get_phydev(&req_info, + tb[ETHTOOL_A_CABLE_TEST_HEADER], + info->extack); + if (IS_ERR_OR_NULL(phydev)) { ret = -EOPNOTSUPP; goto out_dev_put; } - rtnl_lock(); ops = ethtool_phy_ops; if (!ops || !ops->start_cable_test) { ret = -EOPNOTSUPP; @@ -85,13 +90,12 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info) if (ret < 0) goto out_rtnl; - ret = ops->start_cable_test(dev->phydev, info->extack); + ret = ops->start_cable_test(phydev, info->extack); ethnl_ops_complete(dev); if (!ret) - ethnl_cable_test_started(dev->phydev, - ETHTOOL_MSG_CABLE_TEST_NTF); + ethnl_cable_test_started(phydev, ETHTOOL_MSG_CABLE_TEST_NTF); out_rtnl: rtnl_unlock(); @@ -216,7 +220,7 @@ static const struct nla_policy cable_test_tdr_act_cfg_policy[] = { const struct nla_policy ethnl_cable_test_tdr_act_policy[] = { [ETHTOOL_A_CABLE_TEST_TDR_HEADER] = - NLA_POLICY_NESTED(ethnl_header_policy), + NLA_POLICY_NESTED(ethnl_header_policy_phy), [ETHTOOL_A_CABLE_TEST_TDR_CFG] = { .type = NLA_NESTED }, }; @@ -305,6 +309,7 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info) struct ethnl_req_info req_info = {}; const struct ethtool_phy_ops *ops; struct nlattr **tb = info->attrs; + struct phy_device *phydev; struct phy_tdr_config cfg; struct net_device *dev; int ret; @@ -317,10 +322,6 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info) return ret; dev = req_info.dev; - if (!dev->phydev) { - ret = -EOPNOTSUPP; - goto out_dev_put; - } ret = ethnl_act_cable_test_tdr_cfg(tb[ETHTOOL_A_CABLE_TEST_TDR_CFG], info, &cfg); @@ -328,6 +329,14 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info) goto out_dev_put; rtnl_lock(); + phydev = ethnl_req_get_phydev(&req_info, + tb[ETHTOOL_A_CABLE_TEST_TDR_HEADER], + info->extack); + if (!IS_ERR_OR_NULL(phydev)) { + ret = -EOPNOTSUPP; + goto out_dev_put; + } + ops = ethtool_phy_ops; if (!ops || !ops->start_cable_test_tdr) { ret = -EOPNOTSUPP; @@ -338,12 +347,12 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info) if (ret < 0) goto out_rtnl; - ret = ops->start_cable_test_tdr(dev->phydev, info->extack, &cfg); + ret = ops->start_cable_test_tdr(phydev, info->extack, &cfg); ethnl_ops_complete(dev); if (!ret) - ethnl_cable_test_started(dev->phydev, + ethnl_cable_test_started(phydev, ETHTOOL_MSG_CABLE_TEST_TDR_NTF); out_rtnl:
Cable testing is a PHY-specific command. Instead of targeting the command towards dev->phydev, use the request to pick the targeted PHY. Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> --- net/ethtool/cabletest.c | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-)