netdev - Re: [PATCH net] net: team: do not use dynamic lockdep key

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: <79c0acf1-8ede-4e4c-c032-9365dbdb902e@gmail.com>
Date: Fri, 8 Sep 2023 11:33:29 +0900
From: Taehee Yoo <ap420073@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, pabeni@...hat.com, edumazet@...gle.com,
 jiri@...nulli.us, netdev@...r.kernel.org,
 syzbot+9bbbacfbf1e04d5221f7@...kaller.appspotmail.com
Subject: Re: [PATCH net] net: team: do not use dynamic lockdep key



On 2023. 9. 8. 오전 2:31, Jakub Kicinski wrote:

Hi Jakub,
Thank you so much for taking care of this.

 > On Tue,  5 Sep 2023 08:46:10 +0000 Taehee Yoo wrote:
 >> @@ -1203,18 +1203,31 @@ static int team_port_add(struct team *team, 
struct net_device *port_dev,
 >>
 >>   	memcpy(port->orig.dev_addr, port_dev->dev_addr, port_dev->addr_len);
 >>
 >> -	err = team_port_enter(team, port);
 >> +	err = dev_open(port_dev, extack);
 >>   	if (err) {
 >> -		netdev_err(dev, "Device %s failed to enter team mode\n",
 >> +		netdev_dbg(dev, "Device %s opening failed\n",
 >>   			   portname);
 >> -		goto err_port_enter;
 >> +		goto err_dev_open;
 >>   	}
 >>
 >> -	err = dev_open(port_dev, extack);
 >> +	err = team_upper_dev_link(team, port, extack);
 >
 > I'm guessing the syzbot complaint:
 >
 > https://lore.kernel.org/all/000000000000e44e4a0604c66b67@google.com/
 >
 > is related to this reordering of team_upper_dev_link() before things
 > are initialized. I'll revert this version in net, let's target v2 at
 > net-next, next week? "lockdep runs out of keys" isn't a real bug,
 > or at least I don't think the benefit is high enough for pushing
 > functional code changes into current release. Sounds reasonable?
 >

I agree with you.

I think the syzbot reported bug is a side effect of reordering of 
initialization path, especially the reordering of disable_lro() and 
team_upper_dev_link().
This is a more critical issue than the lockdep false-positive bug.

I will send the v2 patch after more tests.
Thanks a lot!
Taehee Yoo



 >>   	if (err) {
 >> -		netdev_dbg(dev, "Device %s opening failed\n",
 >> +		netdev_err(dev, "Device %s failed to set upper link\n",
 >>   			   portname);
 >> -		goto err_dev_open;
 >> +		goto err_set_upper_link;
 >> +	}
 >> +
 >> +	/* lockdep subclass variable(dev->nested_level) was updated by
 >> +	 * team_upper_dev_link().
 >> +	 */
 >> +	team_unlock(team);
 >> +	team_lock(team);
 >> +
 >> +	err = team_port_enter(team, port);
 >> +	if (err) {
 >> +		netdev_err(dev, "Device %s failed to enter team mode\n",
 >> +			   portname);
 >> +		goto err_port_enter;
 >>   	}
 >>
 >>   	err = vlan_vids_add_by_dev(port_dev, dev);
 >> @@ -1242,13 +1255,6 @@ static int team_port_add(struct team *team, 
struct net_device *port_dev,
 >>   		goto err_handler_register;
 >>   	}
 >>
 >> -	err = team_upper_dev_link(team, port, extack);
 >> -	if (err) {
 >> -		netdev_err(dev, "Device %s failed to set upper link\n",
 >> -			   portname);
 >> -		goto err_set_upper_link;
 >> -	}