Commit 2eb6c6a3 authored by Stanislav Fomichev's avatar Stanislav Fomichev Committed by Jakub Kicinski
Browse files

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: a0527ee2 ("net: hold netdev instance lock during qdisc ndo_setup_tc")
Reported-by: default avatarEric Dumazet <edumazet@google.com>
Link: https://lore.kernel.org/netdev/20250305163732.2766420-1-sdf@fomichev.me/T/#me8dfd778ea4c4463acab55644e3f9836bc608771


Signed-off-by: default avatarStanislav Fomichev <sdf@fomichev.me>
Reviewed-by: default avatarEric Dumazet <edumazet@google.com>
Link: https://patch.msgid.link/20250325175427.3818808-1-sdf@fomichev.me


Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
parent 67d1a895

net/sched/sch_api.c

+27 −46
Original line number Diff line number Diff line
@@ -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 @@ static int __tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n, 
				 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 @@ static int __tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n, 
	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;

}