* [PATCH net-next] net: don't relock netdev when on qdisc_create replay
@ 2025-03-13 10:04 Stanislav Fomichev
2025-03-18 15:16 ` Simon Horman
0 siblings, 1 reply; 3+ messages in thread
From: Stanislav Fomichev @ 2025-03-13 10:04 UTC (permalink / raw)
To: netdev
Cc: davem, edumazet, kuba, pabeni, linux-kernel, jhs, xiyou.wangcong,
jiri, horms, sdf
Eric reports that by the time we call netdev_lock_ops after
rtnl_unlock/rtnl_lock, the dev might point to an invalid device.
Don't relock the device after request_module and don't try
to unlock it in the caller (tc_modify_qdisc) in case of replay.
Fixes: a0527ee2df3f ("net: hold netdev instance lock during qdisc ndo_setup_tc")
Reported-by: Eric Dumazet <edumazet@google.com>
Link: https://lore.kernel.org/netdev/20250305163732.2766420-1-sdf@fomichev.me/T/#me8dfd778ea4c4463acab55644e3f9836bc608771
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
net/sched/sch_api.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index abace7665cfe..f1ec6ec0cf05 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1278,13 +1278,14 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
* tell the caller to replay the request. We
* indicate this using -EAGAIN.
* We replay the request because the device may
- * go away in the mean time.
+ * go away in the mean time. Note that we also
+ * don't relock the device because it might
+ * be gone at this point.
*/
netdev_unlock_ops(dev);
rtnl_unlock();
request_module(NET_SCH_ALIAS_PREFIX "%s", name);
rtnl_lock();
- netdev_lock_ops(dev);
ops = qdisc_lookup_ops(kind);
if (ops != NULL) {
/* We will try again qdisc_lookup_ops,
@@ -1837,9 +1838,10 @@ static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
replay = false;
netdev_lock_ops(dev);
err = __tc_modify_qdisc(skb, n, extack, dev, tca, tcm, &replay);
- netdev_unlock_ops(dev);
+ /* __tc_modify_qdisc returns with unlocked dev in case of replay */
if (replay)
goto replay;
+ netdev_unlock_ops(dev);
return err;
}
--
2.48.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH net-next] net: don't relock netdev when on qdisc_create replay
2025-03-13 10:04 [PATCH net-next] net: don't relock netdev when on qdisc_create replay Stanislav Fomichev
@ 2025-03-18 15:16 ` Simon Horman
2025-03-19 18:55 ` Stanislav Fomichev
0 siblings, 1 reply; 3+ messages in thread
From: Simon Horman @ 2025-03-18 15:16 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: netdev, davem, edumazet, kuba, pabeni, linux-kernel, jhs,
xiyou.wangcong, jiri
On Thu, Mar 13, 2025 at 03:04:07AM -0700, Stanislav Fomichev wrote:
> Eric reports that by the time we call netdev_lock_ops after
> rtnl_unlock/rtnl_lock, the dev might point to an invalid device.
> Don't relock the device after request_module and don't try
> to unlock it in the caller (tc_modify_qdisc) in case of replay.
>
> Fixes: a0527ee2df3f ("net: hold netdev instance lock during qdisc ndo_setup_tc")
> Reported-by: Eric Dumazet <edumazet@google.com>
> Link: https://lore.kernel.org/netdev/20250305163732.2766420-1-sdf@fomichev.me/T/#me8dfd778ea4c4463acab55644e3f9836bc608771
> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> ---
> net/sched/sch_api.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index abace7665cfe..f1ec6ec0cf05 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -1278,13 +1278,14 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
> * tell the caller to replay the request. We
> * indicate this using -EAGAIN.
> * We replay the request because the device may
> - * go away in the mean time.
> + * go away in the mean time. Note that we also
> + * don't relock the device because it might
> + * be gone at this point.
> */
> netdev_unlock_ops(dev);
> rtnl_unlock();
> request_module(NET_SCH_ALIAS_PREFIX "%s", name);
> rtnl_lock();
> - netdev_lock_ops(dev);
> ops = qdisc_lookup_ops(kind);
> if (ops != NULL) {
Hi Stan,
I see that if this condition is met then the replay logic
in the next hunk works as intended by this patch.
But what if this condition is not met?
It seems to me that qdisc_create(), and thus __tc_modify_qdisc()
will return with an unlocked device, but the replay logic
won't take effect in tc_modify_qdisc().
Am I missing something?
> /* We will try again qdisc_lookup_ops,
> @@ -1837,9 +1838,10 @@ static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
> replay = false;
> netdev_lock_ops(dev);
> err = __tc_modify_qdisc(skb, n, extack, dev, tca, tcm, &replay);
> - netdev_unlock_ops(dev);
> + /* __tc_modify_qdisc returns with unlocked dev in case of replay */
> if (replay)
> goto replay;
> + netdev_unlock_ops(dev);
>
> return err;
> }
> --
> 2.48.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net-next] net: don't relock netdev when on qdisc_create replay
2025-03-18 15:16 ` Simon Horman
@ 2025-03-19 18:55 ` Stanislav Fomichev
0 siblings, 0 replies; 3+ messages in thread
From: Stanislav Fomichev @ 2025-03-19 18:55 UTC (permalink / raw)
To: Simon Horman
Cc: Stanislav Fomichev, netdev, davem, edumazet, kuba, pabeni,
linux-kernel, jhs, xiyou.wangcong, jiri
On 03/18, Simon Horman wrote:
> On Thu, Mar 13, 2025 at 03:04:07AM -0700, Stanislav Fomichev wrote:
> > Eric reports that by the time we call netdev_lock_ops after
> > rtnl_unlock/rtnl_lock, the dev might point to an invalid device.
> > Don't relock the device after request_module and don't try
> > to unlock it in the caller (tc_modify_qdisc) in case of replay.
> >
> > Fixes: a0527ee2df3f ("net: hold netdev instance lock during qdisc ndo_setup_tc")
> > Reported-by: Eric Dumazet <edumazet@google.com>
> > Link: https://lore.kernel.org/netdev/20250305163732.2766420-1-sdf@fomichev.me/T/#me8dfd778ea4c4463acab55644e3f9836bc608771
> > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> > ---
> > net/sched/sch_api.c | 8 +++++---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> > index abace7665cfe..f1ec6ec0cf05 100644
> > --- a/net/sched/sch_api.c
> > +++ b/net/sched/sch_api.c
> > @@ -1278,13 +1278,14 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
> > * tell the caller to replay the request. We
> > * indicate this using -EAGAIN.
> > * We replay the request because the device may
> > - * go away in the mean time.
> > + * go away in the mean time. Note that we also
> > + * don't relock the device because it might
> > + * be gone at this point.
> > */
> > netdev_unlock_ops(dev);
> > rtnl_unlock();
> > request_module(NET_SCH_ALIAS_PREFIX "%s", name);
> > rtnl_lock();
> > - netdev_lock_ops(dev);
> > ops = qdisc_lookup_ops(kind);
> > if (ops != NULL) {
>
> Hi Stan,
>
> I see that if this condition is met then the replay logic
> in the next hunk works as intended by this patch.
>
> But what if this condition is not met?
> It seems to me that qdisc_create(), and thus __tc_modify_qdisc()
> will return with an unlocked device, but the replay logic
> won't take effect in tc_modify_qdisc().
>
> Am I missing something?
Oh, yes, thanks for catching this. Let me think on how to handle the
-ENOENT as well..
---
pw-bot: cr
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-03-19 18:55 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-13 10:04 [PATCH net-next] net: don't relock netdev when on qdisc_create replay Stanislav Fomichev
2025-03-18 15:16 ` Simon Horman
2025-03-19 18:55 ` Stanislav Fomichev
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).