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

[Ido Schimmel <idosch@nvidia.com>]

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
>