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

[Taehee Yoo <ap420073@gmail.com>]

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;
 >> -	}