From: Jakub Kicinski <kuba@kernel.org>
To: Joe Damato <jdamato@fastly.com>
Cc: Stanislav Fomichev <stfomichev@gmail.com>,
netdev@vger.kernel.org, john.cs.hey@gmail.com,
jacob.e.keller@intel.com,
syzbot+846bb38dc67fe62cc733@syzkaller.appspotmail.com,
Tony Nguyen <anthony.l.nguyen@intel.com>,
Przemek Kitszel <przemyslaw.kitszel@intel.com>,
Andrew Lunn <andrew+netdev@lunn.ch>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Paolo Abeni <pabeni@redhat.com>,
"moderated list:INTEL ETHERNET DRIVERS"
<intel-wired-lan@lists.osuosl.org>,
open list <linux-kernel@vger.kernel.org>
Subject: Re: [Intel-wired-lan] [PATCH iwl-net] e1000: Move cancel_work_sync to avoid deadlock
Date: Fri, 30 May 2025 18:31:40 -0700 [thread overview]
Message-ID: <20250530183140.6cfad3ae@kernel.org> (raw)
In-Reply-To: <aDoKyVE7_hVENi4O@LQ3V64L9R2>
On Fri, 30 May 2025 12:45:13 -0700 Joe Damato wrote:
> > nit: as Jakub mentioned in another thread, it seems more about the
> > flush_work waiting for the reset_task to complete rather than
> > wq mutexes (which are fake)?
>
> Hm, I probably misunderstood something. Also, not sure what you
> meant by the wq mutexes being fake?
>
> My understanding (which is prob wrong) from the syzbot and user
> report was that the order of wq mutex and rtnl are inverted in the
> two paths, which can cause a deadlock if both paths run.
Take a look at touch_work_lockdep_map(), theres nosaj thing as wq mutex.
It's just a lockdep "annotation" that helps lockdep connect the dots
between waiting thread and the work item, not a real mutex. So the
commit msg may be better phrased like this (modulo the lines in front):
CPU 0:
, - RTNL is held
/ - e1000_close
| - e1000_down
+- - cancel_work_sync (cancel / wait for e1000_reset_task())
|
| CPU 1:
| - process_one_work
\ - e1000_reset_task
`- take RTNL
WARNING: multiple messages have this Message-ID (diff)
From: Jakub Kicinski <kuba@kernel.org>
To: Joe Damato <jdamato@fastly.com>
Cc: Stanislav Fomichev <stfomichev@gmail.com>,
netdev@vger.kernel.org, john.cs.hey@gmail.com,
jacob.e.keller@intel.com,
syzbot+846bb38dc67fe62cc733@syzkaller.appspotmail.com,
Tony Nguyen <anthony.l.nguyen@intel.com>,
Przemek Kitszel <przemyslaw.kitszel@intel.com>,
Andrew Lunn <andrew+netdev@lunn.ch>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Paolo Abeni <pabeni@redhat.com>,
"moderated list:INTEL ETHERNET DRIVERS"
<intel-wired-lan@lists.osuosl.org>,
open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH iwl-net] e1000: Move cancel_work_sync to avoid deadlock
Date: Fri, 30 May 2025 18:31:40 -0700 [thread overview]
Message-ID: <20250530183140.6cfad3ae@kernel.org> (raw)
In-Reply-To: <aDoKyVE7_hVENi4O@LQ3V64L9R2>
On Fri, 30 May 2025 12:45:13 -0700 Joe Damato wrote:
> > nit: as Jakub mentioned in another thread, it seems more about the
> > flush_work waiting for the reset_task to complete rather than
> > wq mutexes (which are fake)?
>
> Hm, I probably misunderstood something. Also, not sure what you
> meant by the wq mutexes being fake?
>
> My understanding (which is prob wrong) from the syzbot and user
> report was that the order of wq mutex and rtnl are inverted in the
> two paths, which can cause a deadlock if both paths run.
Take a look at touch_work_lockdep_map(), theres nosaj thing as wq mutex.
It's just a lockdep "annotation" that helps lockdep connect the dots
between waiting thread and the work item, not a real mutex. So the
commit msg may be better phrased like this (modulo the lines in front):
CPU 0:
, - RTNL is held
/ - e1000_close
| - e1000_down
+- - cancel_work_sync (cancel / wait for e1000_reset_task())
|
| CPU 1:
| - process_one_work
\ - e1000_reset_task
`- take RTNL
next prev parent reply other threads:[~2025-05-31 1:31 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-05-30 1:49 [Intel-wired-lan] [PATCH iwl-net] e1000: Move cancel_work_sync to avoid deadlock Joe Damato
2025-05-30 1:49 ` Joe Damato
2025-05-30 15:07 ` [Intel-wired-lan] " Stanislav Fomichev
2025-05-30 15:07 ` Stanislav Fomichev
2025-05-30 19:45 ` [Intel-wired-lan] " Joe Damato
2025-05-30 19:45 ` Joe Damato
2025-05-31 1:31 ` Jakub Kicinski [this message]
2025-05-31 1:31 ` Jakub Kicinski
2025-06-02 20:32 ` [Intel-wired-lan] " Joe Damato
2025-06-02 20:32 ` Joe Damato
2025-06-02 21:32 ` [Intel-wired-lan] " Keller, Jacob E
2025-06-02 21:32 ` Keller, Jacob E
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250530183140.6cfad3ae@kernel.org \
--to=kuba@kernel.org \
--cc=andrew+netdev@lunn.ch \
--cc=anthony.l.nguyen@intel.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=intel-wired-lan@lists.osuosl.org \
--cc=jacob.e.keller@intel.com \
--cc=jdamato@fastly.com \
--cc=john.cs.hey@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=przemyslaw.kitszel@intel.com \
--cc=stfomichev@gmail.com \
--cc=syzbot+846bb38dc67fe62cc733@syzkaller.appspotmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.