netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [syzbot] [net?] WARNING: refcount bug in ethnl_phy_done
@ 2024-09-11  8:00 syzbot
  2024-09-11 10:04 ` Maxime Chevallier
  2024-09-13  8:07 ` [PATCH net-next] net: ethtool: phy: Distinguish whether dev is got by phy start or doit Lizhi Xu
  0 siblings, 2 replies; 8+ messages in thread
From: syzbot @ 2024-09-11  8:00 UTC (permalink / raw)
  To: christophe.leroy, davem, edumazet, kuba, linux-kernel,
	maxime.chevallier, netdev, pabeni, syzkaller-bugs

Hello,

syzbot found the following issue on:

HEAD commit:    a9b1fab3b69f Merge branch 'ionic-convert-rx-queue-buffers-..
git tree:       net-next
console+strace: https://syzkaller.appspot.com/x/log.txt?x=1193c49f980000
kernel config:  https://syzkaller.appspot.com/x/.config?x=37742f4fda0d1b09
dashboard link: https://syzkaller.appspot.com/bug?extid=e9ed4e4368d450c8f9db
compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=14bb7bc7980000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=17b0a100580000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/0459f959b12d/disk-a9b1fab3.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/337f1be5353b/vmlinux-a9b1fab3.xz
kernel image: https://storage.googleapis.com/syzbot-assets/0e3701969c4a/bzImage-a9b1fab3.xz

The issue was bisected to:

commit 17194be4c8e1e82d8b484e58cdcb495c0714d1fd
Author: Maxime Chevallier <maxime.chevallier@bootlin.com>
Date:   Wed Aug 21 15:10:01 2024 +0000

    net: ethtool: Introduce a command to list PHYs on an interface

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=1034a49f980000
final oops:     https://syzkaller.appspot.com/x/report.txt?x=1234a49f980000
console output: https://syzkaller.appspot.com/x/log.txt?x=1434a49f980000

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+e9ed4e4368d450c8f9db@syzkaller.appspotmail.com
Fixes: 17194be4c8e1 ("net: ethtool: Introduce a command to list PHYs on an interface")

------------[ cut here ]------------
refcount_t: decrement hit 0; leaking memory.
WARNING: CPU: 0 PID: 5227 at lib/refcount.c:31 refcount_warn_saturate+0xfa/0x1d0 lib/refcount.c:31
Modules linked in:
CPU: 0 UID: 0 PID: 5227 Comm: syz-executor281 Not tainted 6.11.0-rc6-syzkaller-01235-ga9b1fab3b69f #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/06/2024
RIP: 0010:refcount_warn_saturate+0xfa/0x1d0 lib/refcount.c:31
Code: b2 00 00 00 e8 e7 09 e1 fc 5b 5d c3 cc cc cc cc e8 db 09 e1 fc c6 05 56 d4 4f 0b 01 90 48 c7 c7 c0 3b 60 8c e8 f7 2d a3 fc 90 <0f> 0b 90 90 eb d9 e8 bb 09 e1 fc c6 05 33 d4 4f 0b 01 90 48 c7 c7
RSP: 0018:ffffc900033ff048 EFLAGS: 00010246
RAX: 033d767773c83300 RBX: ffff88802f8d0664 RCX: ffff8880506e0000
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
RBP: 0000000000000004 R08: ffffffff8155b372 R09: 1ffff1101710519a
R10: dffffc0000000000 R11: ffffed101710519b R12: ffff88802f8d0620
R13: 0000000000000000 R14: ffff88802f8d0664 R15: dffffc0000000000
FS:  00005555884f4380(0000) GS:ffff8880b8800000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000055c9aad930a8 CR3: 0000000052acc000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <TASK>
 __refcount_dec include/linux/refcount.h:336 [inline]
 refcount_dec include/linux/refcount.h:351 [inline]
 ref_tracker_free+0x6af/0x7e0 lib/ref_tracker.c:236
 netdev_tracker_free include/linux/netdevice.h:4057 [inline]
 netdev_put include/linux/netdevice.h:4074 [inline]
 ethnl_parse_header_dev_put net/ethtool/netlink.h:271 [inline]
 ethnl_phy_done+0x6f/0x100 net/ethtool/phy.c:238
 genl_done+0x136/0x210 net/netlink/genetlink.c:1043
 netlink_dump+0x9b2/0xd80 net/netlink/af_netlink.c:2370
 __netlink_dump_start+0x5a2/0x790 net/netlink/af_netlink.c:2440
 genl_family_rcv_msg_dumpit net/netlink/genetlink.c:1076 [inline]
 genl_family_rcv_msg net/netlink/genetlink.c:1192 [inline]
 genl_rcv_msg+0x88c/0xec0 net/netlink/genetlink.c:1210
 netlink_rcv_skb+0x1e3/0x430 net/netlink/af_netlink.c:2550
 genl_rcv+0x28/0x40 net/netlink/genetlink.c:1219
 netlink_unicast_kernel net/netlink/af_netlink.c:1331 [inline]
 netlink_unicast+0x7f6/0x990 net/netlink/af_netlink.c:1357
 netlink_sendmsg+0x8e4/0xcb0 net/netlink/af_netlink.c:1901
 sock_sendmsg_nosec net/socket.c:730 [inline]
 __sock_sendmsg+0x221/0x270 net/socket.c:745
 ____sys_sendmsg+0x525/0x7d0 net/socket.c:2597
 ___sys_sendmsg net/socket.c:2651 [inline]
 __sys_sendmsg+0x2b0/0x3a0 net/socket.c:2680
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7f7cf874a219
Code: 48 83 c4 28 c3 e8 e7 18 00 00 0f 1f 80 00 00 00 00 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 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffff97db218 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 00007ffff97db3e8 RCX: 00007f7cf874a219
RDX: 0000000000000000 RSI: 0000000020001900 RDI: 0000000000000003
RBP: 00007f7cf87bc610 R08: 00007ffff97db3e8 R09: 00007ffff97db3e8
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000001
R13: 00007ffff97db3d8 R14: 0000000000000001 R15: 0000000000000001
 </TASK>


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection

If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [syzbot] [net?] WARNING: refcount bug in ethnl_phy_done
  2024-09-11  8:00 [syzbot] [net?] WARNING: refcount bug in ethnl_phy_done syzbot
@ 2024-09-11 10:04 ` Maxime Chevallier
  2024-09-11 10:08   ` Eric Dumazet
  2024-09-13  8:07 ` [PATCH net-next] net: ethtool: phy: Distinguish whether dev is got by phy start or doit Lizhi Xu
  1 sibling, 1 reply; 8+ messages in thread
From: Maxime Chevallier @ 2024-09-11 10:04 UTC (permalink / raw)
  To: syzbot
  Cc: christophe.leroy, davem, edumazet, kuba, linux-kernel, netdev,
	pabeni, syzkaller-bugs

Hi,

On Wed, 11 Sep 2024 01:00:23 -0700
syzbot <syzbot+e9ed4e4368d450c8f9db@syzkaller.appspotmail.com> wrote:

> Hello,
> 
> syzbot found the following issue on:
> 
> HEAD commit:    a9b1fab3b69f Merge branch 'ionic-convert-rx-queue-buffers-..
> git tree:       net-next
> console+strace: https://syzkaller.appspot.com/x/log.txt?x=1193c49f980000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=37742f4fda0d1b09
> dashboard link: https://syzkaller.appspot.com/bug?extid=e9ed4e4368d450c8f9db
> compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=14bb7bc7980000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=17b0a100580000
> 
> Downloadable assets:
> disk image: https://storage.googleapis.com/syzbot-assets/0459f959b12d/disk-a9b1fab3.raw.xz
> vmlinux: https://storage.googleapis.com/syzbot-assets/337f1be5353b/vmlinux-a9b1fab3.xz
> kernel image: https://storage.googleapis.com/syzbot-assets/0e3701969c4a/bzImage-a9b1fab3.xz
> 
> The issue was bisected to:
> 
> commit 17194be4c8e1e82d8b484e58cdcb495c0714d1fd
> Author: Maxime Chevallier <maxime.chevallier@bootlin.com>
> Date:   Wed Aug 21 15:10:01 2024 +0000
> 
>     net: ethtool: Introduce a command to list PHYs on an interface
> 
> bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=1034a49f980000
> final oops:     https://syzkaller.appspot.com/x/report.txt?x=1234a49f980000
> console output: https://syzkaller.appspot.com/x/log.txt?x=1434a49f980000
> 
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+e9ed4e4368d450c8f9db@syzkaller.appspotmail.com
> Fixes: 17194be4c8e1 ("net: ethtool: Introduce a command to list PHYs on an interface")

I'm currently investigating this. I couldn't reproduce it though, even
with the C reproducer, although this was on an arm64 box. I'll give it
a try on x86_64 with the provided .config, see if I can figure out
what's going on, as it looks like the ethnl_phy_start() doesn't get
called.

Thanks,

Maxime

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [syzbot] [net?] WARNING: refcount bug in ethnl_phy_done
  2024-09-11 10:04 ` Maxime Chevallier
@ 2024-09-11 10:08   ` Eric Dumazet
  2024-09-11 11:40     ` Maxime Chevallier
  0 siblings, 1 reply; 8+ messages in thread
From: Eric Dumazet @ 2024-09-11 10:08 UTC (permalink / raw)
  To: Maxime Chevallier
  Cc: syzbot, christophe.leroy, davem, kuba, linux-kernel, netdev,
	pabeni, syzkaller-bugs

On Wed, Sep 11, 2024 at 12:04 PM Maxime Chevallier
<maxime.chevallier@bootlin.com> wrote:
>
> Hi,
>
> On Wed, 11 Sep 2024 01:00:23 -0700
> syzbot <syzbot+e9ed4e4368d450c8f9db@syzkaller.appspotmail.com> wrote:
>
> > Hello,
> >
> > syzbot found the following issue on:
> >
> > HEAD commit:    a9b1fab3b69f Merge branch 'ionic-convert-rx-queue-buffers-..
> > git tree:       net-next
> > console+strace: https://syzkaller.appspot.com/x/log.txt?x=1193c49f980000
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=37742f4fda0d1b09
> > dashboard link: https://syzkaller.appspot.com/bug?extid=e9ed4e4368d450c8f9db
> > compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
> > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=14bb7bc7980000
> > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=17b0a100580000
> >
> > Downloadable assets:
> > disk image: https://storage.googleapis.com/syzbot-assets/0459f959b12d/disk-a9b1fab3.raw.xz
> > vmlinux: https://storage.googleapis.com/syzbot-assets/337f1be5353b/vmlinux-a9b1fab3.xz
> > kernel image: https://storage.googleapis.com/syzbot-assets/0e3701969c4a/bzImage-a9b1fab3.xz
> >
> > The issue was bisected to:
> >
> > commit 17194be4c8e1e82d8b484e58cdcb495c0714d1fd
> > Author: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > Date:   Wed Aug 21 15:10:01 2024 +0000
> >
> >     net: ethtool: Introduce a command to list PHYs on an interface
> >
> > bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=1034a49f980000
> > final oops:     https://syzkaller.appspot.com/x/report.txt?x=1234a49f980000
> > console output: https://syzkaller.appspot.com/x/log.txt?x=1434a49f980000
> >
> > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > Reported-by: syzbot+e9ed4e4368d450c8f9db@syzkaller.appspotmail.com
> > Fixes: 17194be4c8e1 ("net: ethtool: Introduce a command to list PHYs on an interface")
>
> I'm currently investigating this. I couldn't reproduce it though, even
> with the C reproducer, although this was on an arm64 box. I'll give it
> a try on x86_64 with the provided .config, see if I can figure out
> what's going on, as it looks like the ethnl_phy_start() doesn't get
> called.

Make sure to have in your .config

CONFIG_REF_TRACKER=y
CONFIG_NET_DEV_REFCNT_TRACKER=y
CONFIG_NET_NS_REFCNT_TRACKER=y

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [syzbot] [net?] WARNING: refcount bug in ethnl_phy_done
  2024-09-11 10:08   ` Eric Dumazet
@ 2024-09-11 11:40     ` Maxime Chevallier
  0 siblings, 0 replies; 8+ messages in thread
From: Maxime Chevallier @ 2024-09-11 11:40 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: syzbot, christophe.leroy, davem, kuba, linux-kernel, netdev,
	pabeni, syzkaller-bugs

Hi Eric,

On Wed, 11 Sep 2024 12:08:36 +0200
Eric Dumazet <edumazet@google.com> wrote:

> > I'm currently investigating this. I couldn't reproduce it though, even
> > with the C reproducer, although this was on an arm64 box. I'll give it
> > a try on x86_64 with the provided .config, see if I can figure out
> > what's going on, as it looks like the ethnl_phy_start() doesn't get
> > called.  
> 
> Make sure to have in your .config
> 
> CONFIG_REF_TRACKER=y
> CONFIG_NET_DEV_REFCNT_TRACKER=y
> CONFIG_NET_NS_REFCNT_TRACKER=y

Good point, I now reproduce the issue indeed. Thanks a lot for the tips,

Maxime

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH net-next] net: ethtool: phy: Distinguish whether dev is got by phy start or doit
  2024-09-11  8:00 [syzbot] [net?] WARNING: refcount bug in ethnl_phy_done syzbot
  2024-09-11 10:04 ` Maxime Chevallier
@ 2024-09-13  8:07 ` Lizhi Xu
  2024-09-13 11:44   ` Simon Horman
                     ` (2 more replies)
  1 sibling, 3 replies; 8+ messages in thread
From: Lizhi Xu @ 2024-09-13  8:07 UTC (permalink / raw)
  To: syzbot+e9ed4e4368d450c8f9db
  Cc: christophe.leroy, davem, edumazet, kuba, linux-kernel,
	maxime.chevallier, netdev, pabeni, syzkaller-bugs

Syzbot reported a refcount bug in ethnl_phy_done.
This is because when executing ethnl_phy_done, it does not know who obtained
the dev(it can be got by ethnl_phy_doit or ethnl_phy_start) and directly
executes ethnl_parse_header_dev_put as long as the dev is not NULL.
Add dev_start_doit to the structure phy_req_info to distinguish who obtains dev.

Fixes: 17194be4c8e1 ("net: ethtool: Introduce a command to list PHYs on an interface")
Reported-and-tested-by: syzbot+e9ed4e4368d450c8f9db@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=e9ed4e4368d450c8f9db
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
 net/ethtool/phy.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/ethtool/phy.c b/net/ethtool/phy.c
index 4ef7c6e32d10..321a7f89803f 100644
--- a/net/ethtool/phy.c
+++ b/net/ethtool/phy.c
@@ -13,6 +13,7 @@
 struct phy_req_info {
 	struct ethnl_req_info		base;
 	struct phy_device_node		*pdn;
+	u8 dev_start_doit;
 };
 
 #define PHY_REQINFO(__req_base) \
@@ -157,6 +158,9 @@ int ethnl_phy_doit(struct sk_buff *skb, struct genl_info *info)
 	if (ret < 0)
 		return ret;
 
+	if (req_info.base.dev)
+		req_info.dev_start_doit = 0;
+
 	rtnl_lock();
 
 	ret = ethnl_phy_parse_request(&req_info.base, tb, info->extack);
@@ -223,10 +227,14 @@ int ethnl_phy_start(struct netlink_callback *cb)
 					 false);
 	ctx->ifindex = 0;
 	ctx->phy_index = 0;
+	ctx->phy_req_info->dev_start_doit = 0;
 
 	if (ret)
 		kfree(ctx->phy_req_info);
 
+	if (ctx->phy_req_info->base.dev)
+		ctx->phy_req_info->dev_start_doit = 1;
+
 	return ret;
 }
 
@@ -234,7 +242,7 @@ int ethnl_phy_done(struct netlink_callback *cb)
 {
 	struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
 
-	if (ctx->phy_req_info->base.dev)
+	if (ctx->phy_req_info->base.dev && ctx->phy_req_info->dev_start_doit)
 		ethnl_parse_header_dev_put(&ctx->phy_req_info->base);
 
 	kfree(ctx->phy_req_info);
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH net-next] net: ethtool: phy: Distinguish whether dev is got by phy start or doit
  2024-09-13  8:07 ` [PATCH net-next] net: ethtool: phy: Distinguish whether dev is got by phy start or doit Lizhi Xu
@ 2024-09-13 11:44   ` Simon Horman
  2024-09-13 11:51   ` Maxime Chevallier
  2024-09-16  7:38   ` Dan Carpenter
  2 siblings, 0 replies; 8+ messages in thread
From: Simon Horman @ 2024-09-13 11:44 UTC (permalink / raw)
  To: Lizhi Xu
  Cc: syzbot+e9ed4e4368d450c8f9db, christophe.leroy, davem, edumazet,
	kuba, linux-kernel, maxime.chevallier, netdev, pabeni,
	syzkaller-bugs

On Fri, Sep 13, 2024 at 04:07:13PM +0800, Lizhi Xu wrote:
> Syzbot reported a refcount bug in ethnl_phy_done.
> This is because when executing ethnl_phy_done, it does not know who obtained
> the dev(it can be got by ethnl_phy_doit or ethnl_phy_start) and directly
> executes ethnl_parse_header_dev_put as long as the dev is not NULL.
> Add dev_start_doit to the structure phy_req_info to distinguish who obtains dev.
> 
> Fixes: 17194be4c8e1 ("net: ethtool: Introduce a command to list PHYs on an interface")
> Reported-and-tested-by: syzbot+e9ed4e4368d450c8f9db@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=e9ed4e4368d450c8f9db
> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>

It seems that Maxime has also posted a patch for this problem.

- [PATCH net-next] net: ethtool: phy: Don't set the context dev pointer for unfiltered DUMP
  https://lore.kernel.org/all/20240913100515.167341-1-maxime.chevallier@bootlin.com/

> ---
>  net/ethtool/phy.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ethtool/phy.c b/net/ethtool/phy.c
> index 4ef7c6e32d10..321a7f89803f 100644
> --- a/net/ethtool/phy.c
> +++ b/net/ethtool/phy.c
> @@ -13,6 +13,7 @@
>  struct phy_req_info {
>  	struct ethnl_req_info		base;
>  	struct phy_device_node		*pdn;
> +	u8 dev_start_doit;

I think bool might be a more suitable type for this field.

>  };
>  
>  #define PHY_REQINFO(__req_base) \
> @@ -157,6 +158,9 @@ int ethnl_phy_doit(struct sk_buff *skb, struct genl_info *info)
>  	if (ret < 0)
>  		return ret;
>  
> +	if (req_info.base.dev)
> +		req_info.dev_start_doit = 0;
> +
>  	rtnl_lock();
>  
>  	ret = ethnl_phy_parse_request(&req_info.base, tb, info->extack);
> @@ -223,10 +227,14 @@ int ethnl_phy_start(struct netlink_callback *cb)
>  					 false);
>  	ctx->ifindex = 0;
>  	ctx->phy_index = 0;
> +	ctx->phy_req_info->dev_start_doit = 0;
>  
>  	if (ret)
>  		kfree(ctx->phy_req_info);
>  
> +	if (ctx->phy_req_info->base.dev)
> +		ctx->phy_req_info->dev_start_doit = 1;

This doesn't seem right, ctx->phy_req_info may have been freed above.

> +
>  	return ret;
>  }
>  
> @@ -234,7 +242,7 @@ int ethnl_phy_done(struct netlink_callback *cb)
>  {
>  	struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
>  
> -	if (ctx->phy_req_info->base.dev)
> +	if (ctx->phy_req_info->base.dev && ctx->phy_req_info->dev_start_doit)
>  		ethnl_parse_header_dev_put(&ctx->phy_req_info->base);
>  
>  	kfree(ctx->phy_req_info);
> -- 
> 2.43.0
> 
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net-next] net: ethtool: phy: Distinguish whether dev is got by phy start or doit
  2024-09-13  8:07 ` [PATCH net-next] net: ethtool: phy: Distinguish whether dev is got by phy start or doit Lizhi Xu
  2024-09-13 11:44   ` Simon Horman
@ 2024-09-13 11:51   ` Maxime Chevallier
  2024-09-16  7:38   ` Dan Carpenter
  2 siblings, 0 replies; 8+ messages in thread
From: Maxime Chevallier @ 2024-09-13 11:51 UTC (permalink / raw)
  To: Lizhi Xu
  Cc: syzbot+e9ed4e4368d450c8f9db, christophe.leroy, davem, edumazet,
	kuba, linux-kernel, netdev, pabeni, syzkaller-bugs

Hi,

On Fri, 13 Sep 2024 16:07:13 +0800
Lizhi Xu <lizhi.xu@windriver.com> wrote:

> Syzbot reported a refcount bug in ethnl_phy_done.
> This is because when executing ethnl_phy_done, it does not know who obtained
> the dev(it can be got by ethnl_phy_doit or ethnl_phy_start) and directly
> executes ethnl_parse_header_dev_put as long as the dev is not NULL.
> Add dev_start_doit to the structure phy_req_info to distinguish who obtains dev.
> 
> Fixes: 17194be4c8e1 ("net: ethtool: Introduce a command to list PHYs on an interface")
> Reported-and-tested-by: syzbot+e9ed4e4368d450c8f9db@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=e9ed4e4368d450c8f9db
> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>

Thanks for addressing this, however I've already sent a first fix for
this [1] yesterday, followed-up by a second one [2] with another
approach following the reviews.

[1] : https://lore.kernel.org/netdev/20240913091404.3d4a9d19@fedora.home/T/#m4777416dbe26bf97b3a0a323fc71a93b40e0f7fb
[2] : https://lore.kernel.org/netdev/20240913100515.167341-1-maxime.chevallier@bootlin.com/T/#u

Best regards,

Maxime

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net-next] net: ethtool: phy: Distinguish whether dev is got by phy start or doit
  2024-09-13  8:07 ` [PATCH net-next] net: ethtool: phy: Distinguish whether dev is got by phy start or doit Lizhi Xu
  2024-09-13 11:44   ` Simon Horman
  2024-09-13 11:51   ` Maxime Chevallier
@ 2024-09-16  7:38   ` Dan Carpenter
  2 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2024-09-16  7:38 UTC (permalink / raw)
  To: oe-kbuild, Lizhi Xu, syzbot+e9ed4e4368d450c8f9db
  Cc: lkp, oe-kbuild-all, christophe.leroy, davem, edumazet, kuba,
	linux-kernel, maxime.chevallier, netdev, pabeni, syzkaller-bugs

Hi Lizhi,

kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/Lizhi-Xu/net-ethtool-phy-Distinguish-whether-dev-is-got-by-phy-start-or-doit/20240913-160835
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240913080714.1809254-1-lizhi.xu%40windriver.com
patch subject: [PATCH net-next] net: ethtool: phy: Distinguish whether dev is got by phy start or doit
config: x86_64-randconfig-r072-20240914 (https://download.01.org/0day-ci/archive/20240916/202409161017.tjjHpXGT-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202409161017.tjjHpXGT-lkp@intel.com/

smatch warnings:
net/ethtool/phy.c:235 ethnl_phy_start() error: dereferencing freed memory 'ctx->phy_req_info'

vim +235 net/ethtool/phy.c

17194be4c8e1e8 Maxime Chevallier 2024-08-21  212  int ethnl_phy_start(struct netlink_callback *cb)
17194be4c8e1e8 Maxime Chevallier 2024-08-21  213  {
17194be4c8e1e8 Maxime Chevallier 2024-08-21  214  	const struct genl_info *info = genl_info_dump(cb);
17194be4c8e1e8 Maxime Chevallier 2024-08-21  215  	struct ethnl_phy_dump_ctx *ctx = (void *)cb->ctx;
17194be4c8e1e8 Maxime Chevallier 2024-08-21  216  	int ret;
17194be4c8e1e8 Maxime Chevallier 2024-08-21  217  
17194be4c8e1e8 Maxime Chevallier 2024-08-21  218  	BUILD_BUG_ON(sizeof(*ctx) > sizeof(cb->ctx));
17194be4c8e1e8 Maxime Chevallier 2024-08-21  219  
17194be4c8e1e8 Maxime Chevallier 2024-08-21  220  	ctx->phy_req_info = kzalloc(sizeof(*ctx->phy_req_info), GFP_KERNEL);
17194be4c8e1e8 Maxime Chevallier 2024-08-21  221  	if (!ctx->phy_req_info)
17194be4c8e1e8 Maxime Chevallier 2024-08-21  222  		return -ENOMEM;
17194be4c8e1e8 Maxime Chevallier 2024-08-21  223  
17194be4c8e1e8 Maxime Chevallier 2024-08-21  224  	ret = ethnl_parse_header_dev_get(&ctx->phy_req_info->base,
17194be4c8e1e8 Maxime Chevallier 2024-08-21  225  					 info->attrs[ETHTOOL_A_PHY_HEADER],
17194be4c8e1e8 Maxime Chevallier 2024-08-21  226  					 sock_net(cb->skb->sk), cb->extack,
17194be4c8e1e8 Maxime Chevallier 2024-08-21  227  					 false);
17194be4c8e1e8 Maxime Chevallier 2024-08-21  228  	ctx->ifindex = 0;
17194be4c8e1e8 Maxime Chevallier 2024-08-21  229  	ctx->phy_index = 0;
355b18bd0d5516 Lizhi Xu          2024-09-13  230  	ctx->phy_req_info->dev_start_doit = 0;
17194be4c8e1e8 Maxime Chevallier 2024-08-21  231  
17194be4c8e1e8 Maxime Chevallier 2024-08-21  232  	if (ret)
17194be4c8e1e8 Maxime Chevallier 2024-08-21  233  		kfree(ctx->phy_req_info);
                                                                      ^^^^^^^^^^^^^^^^^
Freed

17194be4c8e1e8 Maxime Chevallier 2024-08-21  234  
355b18bd0d5516 Lizhi Xu          2024-09-13 @235  	if (ctx->phy_req_info->base.dev)
                                                            ^^^^^^^^^^^^^^^^^
Use after free

355b18bd0d5516 Lizhi Xu          2024-09-13  236  		ctx->phy_req_info->dev_start_doit = 1;
355b18bd0d5516 Lizhi Xu          2024-09-13  237  
17194be4c8e1e8 Maxime Chevallier 2024-08-21  238  	return ret;
17194be4c8e1e8 Maxime Chevallier 2024-08-21  239  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2024-09-16  7:38 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-11  8:00 [syzbot] [net?] WARNING: refcount bug in ethnl_phy_done syzbot
2024-09-11 10:04 ` Maxime Chevallier
2024-09-11 10:08   ` Eric Dumazet
2024-09-11 11:40     ` Maxime Chevallier
2024-09-13  8:07 ` [PATCH net-next] net: ethtool: phy: Distinguish whether dev is got by phy start or doit Lizhi Xu
2024-09-13 11:44   ` Simon Horman
2024-09-13 11:51   ` Maxime Chevallier
2024-09-16  7:38   ` Dan Carpenter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).