Re: [PATCH AUTOSEL 6.18-5.15] nvme-tcp: look up host_iface in the current netns

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] nvme-tcp: use-after-free in nvme_tcp_alloc_ctrl() via __dev_get_by_name()
--

commit 04aa71fe23c7dec86f240fa7773c4020c75d2bb7
Author: Geliang Tang <geliang@kernel.org>

nvme-tcp: look up host_iface in the current netns

This commit changes nvme_tcp_alloc_ctrl() to look up opts->host_iface in
current->nsproxy->net_ns rather than init_net so that interfaces in the
calling task's network namespace can be found.

> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 9a96df1a511c0..fb6858023f19d 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -2935,7 +2935,8 @@ static struct nvme_tcp_ctrl *nvme_tcp_alloc_ctrl(struct device *dev,
>  	}
>  
>  	if (opts->mask & NVMF_OPT_HOST_IFACE) {
> -		if (!__dev_get_by_name(&init_net, opts->host_iface)) {
> +		if (!__dev_get_by_name(current->nsproxy->net_ns,
> +				       opts->host_iface)) {

[Severity: Critical]
Is it safe to call __dev_get_by_name() here without holding rtnl_lock()?

When userspace writes connection parameters with host_iface=<name> to
/dev/nvme-fabrics, the call chain:

  nvmf_dev_write()
    -> nvmf_create_ctrl()
      -> nvme_tcp_create_ctrl()
        -> nvme_tcp_alloc_ctrl()

runs in sleepable process context holding only nvmf_dev_mutex, with
neither rtnl_lock() nor rcu_read_lock() held.

In net/core/dev.c, __dev_get_by_name() requires the RTNL semaphore and
calls netdev_name_node_lookup(), which traverses the per-netns hash bucket
with non-RCU list primitives:

net/core/dev.c:netdev_name_node_lookup() {
	struct hlist_head *head = dev_name_hash(net, name);
	struct netdev_name_node *name_node;

	hlist_for_each_entry(name_node, head, hlist)
		if (!strcmp(name_node->name, name))
			return name_node;
	return NULL;
}

net/core/dev.c:__dev_get_by_name() {
	struct netdev_name_node *node_name;

	node_name = netdev_name_node_lookup(net, name);
	return node_name ? node_name->dev : NULL;
}

If a network device in current->nsproxy->net_ns is concurrently
unregistered via unregister_netdevice_many_notify() ->
rollback_registered_many() (which unlinks the node in unlist_netdevice(),
waits for an RCU grace period via synchronize_net(), and then frees the
nodes via netdev_name_node_alt_flush() and netdev_name_node_free()), has an
altname deleted via netdev_name_node_alt_destroy() ->
__netdev_name_node_alt_destroy(), or is renamed via dev_change_name(),
neither synchronize_net() nor call_rcu() will wait for a preempted reader
in nvme_tcp_alloc_ctrl().

Could this trigger a use-after-free on name_node or name_node->name during
the hash list traversal, and should this lookup use dev_get_by_name_rcu()
under rcu_read_lock() (or netdev_name_in_use()) instead?

>  			pr_err("invalid interface passed: %s\n",
>  			       opts->host_iface);
>  			ret = -ENODEV;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019135441.1420968-231-sashal@kernel.org?part=1