net: move replay logic to tc_modify_qdisc

Eric reports that by the time we call netdev_lock_ops after
rtnl_unlock/rtnl_lock, the dev might point to an invalid device.
As suggested by Jakub in [0], move rtnl lock/unlock and request_module
outside of qdisc_create. This removes extra complexity with relocking
the netdev.

0: https://lore.kernel.org/netdev/20250325032803.1542c15e@kernel.org/

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>
Reviewed-by: Eric Dumazet <edumazet@google.com>
Link: https://patch.msgid.link/20250325175427.3818808-1-sdf@fomichev.me
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index defb05c1fba43610ba814d0d5d209e2afb14f714..f74a097f54ae7636b3add43576eaf7be5859071b 100644 (file)
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1267,38 +1267,8 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
        struct qdisc_size_table *stab;
 
        ops = qdisc_lookup_ops(kind);
-#ifdef CONFIG_MODULES
-       if (ops == NULL && kind != NULL) {
-               char name[IFNAMSIZ];
-               if (nla_strscpy(name, kind, IFNAMSIZ) >= 0) {
-                       /* We dropped the RTNL semaphore in order to
-                        * perform the module load.  So, even if we
-                        * succeeded in loading the module we have to
-                        * 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.
-                        */
-                       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,
-                                * so don't keep a reference.
-                                */
-                               module_put(ops->owner);
-                               err = -EAGAIN;
-                               goto err_out;
-                       }
-               }
-       }
-#endif
-
-       err = -ENOENT;
        if (!ops) {
+               err = -ENOENT;
                NL_SET_ERR_MSG(extack, "Specified qdisc kind is unknown");
                goto err_out;
        }
@@ -1623,8 +1593,7 @@ static int __tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
                             struct netlink_ext_ack *extack,
                             struct net_device *dev,
                             struct nlattr *tca[TCA_MAX + 1],
-                            struct tcmsg *tcm,
-                            bool *replay)
+                            struct tcmsg *tcm)
 {
        struct Qdisc *q = NULL;
        struct Qdisc *p = NULL;
@@ -1789,13 +1758,8 @@ create_n_graft2:
                                 tcm->tcm_parent, tcm->tcm_handle,
                                 tca, &err, extack);
        }
-       if (q == NULL) {
-               if (err == -EAGAIN) {
-                       *replay = true;
-                       return 0;
-               }
+       if (!q)
                return err;
-       }
 
 graft:
        err = qdisc_graft(dev, p, skb, n, clid, q, NULL, extack);
@@ -1808,6 +1772,27 @@ graft:
        return 0;
 }
 
+static void request_qdisc_module(struct nlattr *kind)
+{
+       struct Qdisc_ops *ops;
+       char name[IFNAMSIZ];
+
+       if (!kind)
+               return;
+
+       ops = qdisc_lookup_ops(kind);
+       if (ops) {
+               module_put(ops->owner);
+               return;
+       }
+
+       if (nla_strscpy(name, kind, IFNAMSIZ) >= 0) {
+               rtnl_unlock();
+               request_module(NET_SCH_ALIAS_PREFIX "%s", name);
+               rtnl_lock();
+       }
+}
+
 /*
  * Create/change qdisc.
  */
@@ -1818,27 +1803,23 @@ static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
        struct nlattr *tca[TCA_MAX + 1];
        struct net_device *dev;
        struct tcmsg *tcm;
-       bool replay;
        int err;
 
-replay:
-       /* Reinit, just in case something touches this. */
        err = nlmsg_parse_deprecated(n, sizeof(*tcm), tca, TCA_MAX,
                                     rtm_tca_policy, extack);
        if (err < 0)
                return err;
 
+       request_qdisc_module(tca[TCA_KIND]);
+
        tcm = nlmsg_data(n);
        dev = __dev_get_by_index(net, tcm->tcm_ifindex);
        if (!dev)
                return -ENODEV;
 
-       replay = false;
        netdev_lock_ops(dev);
-       err = __tc_modify_qdisc(skb, n, extack, dev, tca, tcm, &replay);
+       err = __tc_modify_qdisc(skb, n, extack, dev, tca, tcm);
        netdev_unlock_ops(dev);
-       if (replay)
-               goto replay;
 
        return err;
 }