Re: [patch net-next v4 05/13] net: devlink: track netdev with devlink_port assigned

[Jiri Pirko <jiri@resnulli.us>]

Sun, Nov 06, 2022 at 10:09:42AM CET, idosch@idosch.org wrote:
>On Wed, Nov 02, 2022 at 05:02:03PM +0100, Jiri Pirko wrote:
>> @@ -9645,10 +9649,13 @@ struct devlink *devlink_alloc_ns(const struct devlink_ops *ops,
>>  
>>  	ret = xa_alloc_cyclic(&devlinks, &devlink->index, devlink, xa_limit_31b,
>>  			      &last_id, GFP_KERNEL);
>> -	if (ret < 0) {
>> -		kfree(devlink);
>> -		return NULL;
>> -	}
>> +	if (ret < 0)
>> +		goto err_xa_alloc;
>> +
>> +	devlink->netdevice_nb.notifier_call = devlink_netdevice_event;
>> +	ret = register_netdevice_notifier_net(net, &devlink->netdevice_nb);
>> +	if (ret)
>> +		goto err_register_netdevice_notifier;
>>  
>>  	devlink->dev = dev;
>>  	devlink->ops = ops;
>> @@ -9675,6 +9682,12 @@ struct devlink *devlink_alloc_ns(const struct devlink_ops *ops,
>>  	init_completion(&devlink->comp);
>>  
>>  	return devlink;
>> +
>> +err_register_netdevice_notifier:
>> +	xa_erase(&devlinks, devlink->index);
>> +err_xa_alloc:
>> +	kfree(devlink);
>> +	return NULL;
>>  }
>>  EXPORT_SYMBOL_GPL(devlink_alloc_ns);
>>  
>> @@ -9828,6 +9841,10 @@ void devlink_free(struct devlink *devlink)
>>  	WARN_ON(!list_empty(&devlink->port_list));
>>  
>>  	xa_destroy(&devlink->snapshot_ids);
>> +
>> +	unregister_netdevice_notifier_net(devlink_net(devlink),
>> +					  &devlink->netdevice_nb);
>> +
>>  	xa_erase(&devlinks, devlink->index);
>>  
>>  	kfree(devlink);
>
>The network namespace of the devlink instance can change throughout the
>lifetime of the devlink instance, but the notifier block is always
>registered in the initial namespace. This leads to
>unregister_netdevice_notifier_net() failing to unregister the notifier
>block, which leads to use-after-free. Reproduce (with KASAN enabled):
>
># echo "10 0" > /sys/bus/netdevsim/new_device
># ip netns add bla
># devlink dev reload netdevsim/netdevsim10 netns bla
># echo 10 > /sys/bus/netdevsim/del_device
># ip link add dummy10 up type dummy
>
>I see two possible solutions:
>
>1. Use register_netdevice_notifier() instead of
>register_netdevice_notifier_net().
>
>2. Move the notifier block to the correct namespace in devlink_reload().

Yep, this was my intension, slipped my mind. Thanks! Will send a
follow-up.