linux-kernel - Re: [PATCH net-next] net: don't relock netdev when on qdisc

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250318151624.GC688833@kernel.org>
Date: Tue, 18 Mar 2025 15:16:24 +0000
From: Simon Horman <horms@...nel.org>
To: Stanislav Fomichev <sdf@...ichev.me>
Cc: netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, linux-kernel@...r.kernel.org,
	jhs@...atatu.com, xiyou.wangcong@...il.com, jiri@...nulli.us
Subject: Re: [PATCH net-next] net: don't relock netdev when on qdisc_create
 replay

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@...gle.com>
> Link: https://lore.kernel.org/netdev/20250305163732.2766420-1-sdf@fomichev.me/T/#me8dfd778ea4c4463acab55644e3f9836bc608771
> Signed-off-by: Stanislav Fomichev <sdf@...ichev.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
>