| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c3983568-203d-4d90-838c-c87d1c6ab605@intel.com>
Date: Tue, 26 Aug 2025 13:49:18 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Alexander Duyck <alexander.duyck@...il.com>, <AlexanderDuyck@...il.com>
CC: <kuba@...nel.org>, <kernel-team@...a.com>, <andrew+netdev@...n.ch>,
<pabeni@...hat.com>, <davem@...emloft.net>, <netdev@...r.kernel.org>
Subject: Re: [net PATCH 2/2] fbnic: Move phylink resume out of service_task
and into open/close
On 8/26/25 00:56, Alexander Duyck wrote:
> From: Alexander Duyck <alexanderduyck@...com>
>
> The fbnic driver was presenting with the following locking assert coming
> out of a PM resume:
> [ 42.208116][ T164] RTNL: assertion failed at drivers/net/phy/phylink.c (2611)
> [ 42.208492][ T164] WARNING: CPU: 1 PID: 164 at drivers/net/phy/phylink.c:2611 phylink_resume+0x190/0x1e0
> [ 42.208872][ T164] Modules linked in:
> [ 42.209140][ T164] CPU: 1 UID: 0 PID: 164 Comm: bash Not tainted 6.17.0-rc2-virtme #134 PREEMPT(full)
> [ 42.209496][ T164] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.17.0-5.fc42 04/01/2014
> [ 42.209861][ T164] RIP: 0010:phylink_resume+0x190/0x1e0
> [ 42.210057][ T164] Code: 83 e5 01 0f 85 b0 fe ff ff c6 05 1c cd 3e 02 01 90 ba 33 0a 00 00 48 c7 c6 20 3a 1d a5 48 c7 c7 e0 3e 1d a5 e8 21 b8 90 fe 90 <0f> 0b 90 90 e9 86 fe ff ff e8 42 ea 1f ff e9 e2 fe ff ff 48 89 ef
> [ 42.210708][ T164] RSP: 0018:ffffc90000affbd8 EFLAGS: 00010296
> [ 42.210983][ T164] RAX: 0000000000000000 RBX: ffff8880078d8400 RCX: 0000000000000000
> [ 42.211235][ T164] RDX: 0000000000000000 RSI: 1ffffffff4f10938 RDI: 0000000000000001
> [ 42.211466][ T164] RBP: 0000000000000000 R08: ffffffffa2ae79ea R09: fffffbfff4b3eb84
> [ 42.211707][ T164] R10: 0000000000000003 R11: 0000000000000000 R12: ffff888007ad8000
> [ 42.211997][ T164] R13: 0000000000000002 R14: ffff888006a18800 R15: ffffffffa34c59e0
> [ 42.212234][ T164] FS: 00007f0dc8e39740(0000) GS:ffff88808f51f000(0000) knlGS:0000000000000000
> [ 42.212505][ T164] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 42.212704][ T164] CR2: 00007f0dc8e9fe10 CR3: 000000000b56d003 CR4: 0000000000772ef0
> [ 42.213227][ T164] PKRU: 55555554
> [ 42.213366][ T164] Call Trace:
> [ 42.213483][ T164] <TASK>
> [ 42.213565][ T164] __fbnic_pm_attach.isra.0+0x8e/0xa0
> [ 42.213725][ T164] pci_reset_function+0x116/0x1d0
> [ 42.213895][ T164] reset_store+0xa0/0x100
> [ 42.214025][ T164] ? pci_dev_reset_attr_is_visible+0x50/0x50
> [ 42.214221][ T164] ? sysfs_file_kobj+0xc1/0x1e0
> [ 42.214374][ T164] ? sysfs_kf_write+0x65/0x160
> [ 42.214526][ T164] kernfs_fop_write_iter+0x2f8/0x4c0
> [ 42.214677][ T164] ? kernfs_vma_page_mkwrite+0x1f0/0x1f0
> [ 42.214836][ T164] new_sync_write+0x308/0x6f0
> [ 42.214987][ T164] ? __lock_acquire+0x34c/0x740
> [ 42.215135][ T164] ? new_sync_read+0x6f0/0x6f0
> [ 42.215288][ T164] ? lock_acquire.part.0+0xbc/0x260
> [ 42.215440][ T164] ? ksys_write+0xff/0x200
> [ 42.215590][ T164] ? perf_trace_sched_switch+0x6d0/0x6d0
> [ 42.215742][ T164] vfs_write+0x65e/0xbb0
> [ 42.215876][ T164] ksys_write+0xff/0x200
> [ 42.215994][ T164] ? __ia32_sys_read+0xc0/0xc0
> [ 42.216141][ T164] ? do_user_addr_fault+0x269/0x9f0
> [ 42.216292][ T164] ? rcu_is_watching+0x15/0xd0
> [ 42.216442][ T164] do_syscall_64+0xbb/0x360
> [ 42.216591][ T164] entry_SYSCALL_64_after_hwframe+0x4b/0x53
> [ 42.216784][ T164] RIP: 0033:0x7f0dc8ea9986
>
> A bit of digging showed that we were invoking the phylink_resume as a part
> of the fbnic_up path when we were enabling the service task while not
> holding the RTNL lock. We should be enabling this sooner as a part of the
> ndo_open path and then just letting the service task come online later.
> This will help to enforce the correct locking and brings the phylink
> interface online at the same time as the network interface, instead of at a
> later time.
>
> I tested this on QEMU to verify this was working by putting the system to
> sleep using "echo mem > /sys/power/state" to put the system to sleep in the
> guest and then using the command "system_wakeup" in the QEMU monitor.
>
> Fixes: 69684376eed5 ("eth: fbnic: Add link detection")
> Signed-off-by: Alexander Duyck <alexanderduyck@...com>
changes look good,
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
next time please minify dmesg output:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#backtraces-in-commit-messages
> ---
> drivers/net/ethernet/meta/fbnic/fbnic_netdev.c | 4 ++++
> drivers/net/ethernet/meta/fbnic/fbnic_pci.c | 2 --
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
> index e67e99487a27..40581550da1a 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
> @@ -52,6 +52,8 @@ int __fbnic_open(struct fbnic_net *fbn)
> fbnic_bmc_rpc_init(fbd);
> fbnic_rss_reinit(fbd, fbn);
>
> + phylink_resume(fbn->phylink);
> +
> return 0;
> time_stop:
> fbnic_time_stop(fbn);
> @@ -84,6 +86,8 @@ static int fbnic_stop(struct net_device *netdev)
> {
> struct fbnic_net *fbn = netdev_priv(netdev);
>
> + phylink_suspend(fbn->phylink, fbnic_bmc_present(fbn->fbd));
> +
> fbnic_down(fbn);
> fbnic_pcs_free_irq(fbn->fbd);
>
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> index a7784deea88f..28e23e3ffca8 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> @@ -118,14 +118,12 @@ static void fbnic_service_task_start(struct fbnic_net *fbn)
> struct fbnic_dev *fbd = fbn->fbd;
>
> schedule_delayed_work(&fbd->service_task, HZ);
> - phylink_resume(fbn->phylink);
> }
>
> static void fbnic_service_task_stop(struct fbnic_net *fbn)
> {
> struct fbnic_dev *fbd = fbn->fbd;
>
> - phylink_suspend(fbn->phylink, fbnic_bmc_present(fbd));
> cancel_delayed_work(&fbd->service_task);
> }
>
>
>
>
Powered by blists - more mailing lists