Re: [PATCH net] netdevsim: fib: fix use-after-free of FIB data via debugfs

[Zijing yin <yzjaurora@gmail.com>]

Thanks so much for the feedback!
I have sent the revised patch accordingly:
https://lore.kernel.org/netdev/20260529135718.1804031-1-yzjaurora@gmail.com/T/#u
Hope you have a nice weekend!

On 27.05.2026 10:32, Ido Schimmel wrote:
> On Tue, May 26, 2026 at 09:09:08AM -0700, Zijing Yin wrote:
>> @@ -1600,6 +1597,16 @@ struct nsim_fib_data *nsim_fib_create(struct devlink *devlink,
>>  		goto err_nexthop_nb_unregister;
>>  	}
>>  
>> +	/* Publish the debugfs interface only after every data structure it
>> +	 * operates on has been initialized. The files reference this
>> +	 * nsim_fib_data (e.g. "nexthop_bucket_activity" looks up
>> +	 * data->nexthop_ht), so a concurrent debugfs access must never be able
>> +	 * to observe a half-constructed instance.
>> +	 */
>> +	err = nsim_fib_debugfs_init(data, nsim_dev);
>> +	if (err)
>> +		goto err_fib_notifier_unregister;
>> +
>>  	devl_resource_occ_get_register(devlink,
>>  				       NSIM_RESOURCE_IPV4_FIB,
>>  				       nsim_fib_ipv4_resource_occ_get,
>> @@ -1622,6 +1629,8 @@ struct nsim_fib_data *nsim_fib_create(struct devlink *devlink,
>>  				       data);
>>  	return data;
>>  
>> +err_fib_notifier_unregister:
>> +	unregister_fib_notifier(devlink_net(devlink), &data->fib_nb);
>>  err_nexthop_nb_unregister:
>>  	unregister_nexthop_notifier(devlink_net(devlink), &data->nexthop_nb);
>>  err_rhashtable_fib_destroy:
>> @@ -1633,16 +1642,23 @@ struct nsim_fib_data *nsim_fib_create(struct devlink *devlink,
>>  	rhashtable_free_and_destroy(&data->nexthop_ht, nsim_nexthop_free,
>>  				    data);
>>  	mutex_destroy(&data->fib_lock);
>> -err_debugfs_exit:
>> +err_nh_lock_destroy:
>>  	mutex_destroy(&data->nh_lock);
>> -	nsim_fib_debugfs_exit(data);
>> -err_data_free:
>>  	kfree(data);
>>  	return ERR_PTR(err);
>>  }
>>  
>>  void nsim_fib_destroy(struct devlink *devlink, struct nsim_fib_data *data)
>>  {
>> +	/* Tear down the debugfs files before freeing the data structures they
>> +	 * operate on. debugfs_remove_recursive() waits for any in-flight file
>> +	 * operation (e.g. a write to "fib/nexthop_bucket_activity", which looks
>> +	 * up data->nexthop_ht) to finish and prevents new ones from starting,
>> +	 * so the rhashtables are not freed while a concurrent accessor still
>> +	 * dereferences them.
>> +	 */
>> +	nsim_fib_debugfs_exit(data);
> 
> Thanks for the patch. Let's try to keep both functions symmetric:
> 
> Call nsim_fib_debugfs_exit() just before unregister_fib_notifier().
> 
> Also, I would drop the comments.
> 
>> +
>>  	devl_resource_occ_get_unregister(devlink,
>>  					 NSIM_RESOURCE_NEXTHOPS);
>>  	devl_resource_occ_get_unregister(devlink,
>> @@ -1665,6 +1681,5 @@ void nsim_fib_destroy(struct devlink *devlink, struct nsim_fib_data *data)
>>  	WARN_ON_ONCE(!list_empty(&data->fib_rt_list));
>>  	mutex_destroy(&data->fib_lock);
>>  	mutex_destroy(&data->nh_lock);
>> -	nsim_fib_debugfs_exit(data);
>>  	kfree(data);
>>  }
>> -- 
>> 2.43.0
>>