Re: [PATCH v2 2/2] Drivers: hv: vmbus: Do not remove the channel when device registration fails

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] hv: vmbus: dangling channel->device_obj on vmbus_device_register() failure
- [High] hv: vmbus: sub-channel refusal in vmbus_add_channel_kobj() causes deadlock
- [High] hv: vmbus: unlocked channel dereference in vmbus_match() without RCU
- [Medium] hv: vmbus: leaked sysfs state when primary vmbus_add_channel_kobj() fails
--

commit 1c2eec91c8b747f69eceb0c4ce1f5ab28e5429b8
Author: Yuqiang Sun <izaiahsun@gmail.com>

Drivers: hv: vmbus: Do not remove the channel when device registration fails

This commit avoids calling hv_process_channel_removal() in
vmbus_device_release() when device_register() fails by clearing the
device's channel pointer beforehand, and keeps the device registered if
creating the channels kset or primary channel kobject fails.

> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index fc0f16f512669..1b9bc82bebd06 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -840,9 +840,14 @@ static int vmbus_match(struct device *device, const struct device_driver *driver
>  {
>  	const struct hv_driver *drv = drv_to_hv_drv(driver);
>  	struct hv_device *hv_dev = device_to_hv_device(device);
> +	struct vmbus_channel *channel = READ_ONCE(hv_dev->channel);
> +
> +	/* None if vmbus_device_register() failed */
> +	if (!channel)
> +		return 0;
>  
>  	/* The hv_sock driver handles all hv_sock offers. */
> -	if (is_hvsock_channel(hv_dev->channel))
> +	if (is_hvsock_channel(channel))
>  		return drv->hvsock;

[Severity: High]
As reported by Sashiko on v1 [1], can channel be freed before
is_hvsock_channel(channel) dereferences it here?

During device_register() -> device_add(), bus_add_device() links the
device onto the bus klist_devices before later steps in device_add()
(such as dpm_sysfs_add()) can fail. On failure, bus_remove_device() calls
klist_del(), which does not wait for a concurrent __driver_attach() (for
example via vmbus_add_dynid()) that already took a device reference in
bus_for_each_dev() and runs driver_match_device() -> vmbus_match() before
acquiring device_lock(dev).

If vmbus_match() loads hv_dev->channel with READ_ONCE() just before
vmbus_device_register() clears child_device_obj->channel to NULL, the
registering thread returns to vmbus_add_channel_work(), which runs
err_deq_chan and calls free_channel() -> kobject_put() ->
vmbus_chan_release() -> kfree_rcu(channel, rcu).

Because vmbus_match() runs in preemptible process context without
rcu_read_lock(), if the matching thread is preempted after loading
channel, could the RCU grace period complete and free channel before
is_hvsock_channel(channel) reads channel->offermsg.offer.chn_flags?

[ ... ]

> @@ -2103,6 +2114,10 @@ int vmbus_add_channel_kobj(struct hv_device *dev, struct vmbus_channel *channel)
>  	u32 relid = channel->offermsg.child_relid;
>  	int ret;
>  
> +	/* The device has no channels directory if creating it failed */
> +	if (!dev->channels_kset)
> +		return -ENOENT;
> +
>  	/*
>  	 * On failure the kobject keeps its reference: the callers' error
>  	 * paths drop it in free_channel(), which also frees the channel.

[Severity: High]
Does returning -ENOENT when !dev->channels_kset cause sub-channel setup
to deadlock or hang system suspend?

In vmbus_device_register(), device_register() runs before
kset_create_and_add("channels", ...) assigns child_device_obj->channels_kset,
and if kset_create_and_add() fails, the device now stays registered and
bound to its driver with channels_kset == NULL.

During device_register() -> device_add() -> bus_probe_device(), drivers
such as hv_storvsc and hv_netvsc request sub-channels from Hyper-V before
device_register() returns. Because vmbus_process_offer() queues
sub-channel work onto handle_sub_chan_wq concurrently with
handle_primary_chan_wq, vmbus_add_channel_work() for an offered
sub-channel can call vmbus_add_channel_kobj() either before
kset_create_and_add() sets dev->channels_kset or after
kset_create_and_add() fails:

drivers/hv/channel_mgmt.c:vmbus_add_channel_work() {
    ...
		if (vmbus_add_channel_kobj(dev, newchannel))
			goto err_deq_chan;

		if (primary_channel->sc_creation_callback != NULL)
			primary_channel->sc_creation_callback(newchannel);
    ...
}

When vmbus_add_channel_kobj() fails and jumps to err_deq_chan,
sc_creation_callback is skipped. In hv_netvsc, netvsc_subchan_work() ->
rndis_set_subchannel() holds rtnl_lock() and waits uninterruptibly for
netvsc_sc_open() to increment nvdev->open_chn for each requested
sub-channel:

drivers/net/hyperv/rndis_filter.c:rndis_set_subchannel() {
    ...
	/* wait for all sub channels to open */
	wait_event(nvdev->subchan_open,
		   atomic_read(&nvdev->open_chn) == nvdev->num_chn);
}

In addition, vmbus_process_offer() increments
vmbus_connection.nr_chan_close_on_suspend for every sub-channel and
allocates a target CPU in init_vp_index(), but err_deq_chan in
vmbus_add_channel_work() frees newchannel without decrementing
nr_chan_close_on_suspend or calling hv_clear_allocated_cpu(). Wouldn't
this leave rndis_set_subchannel() deadlocked holding rtnl_lock(), and
cause vmbus_bus_suspend() to hang forever in
wait_for_completion(&vmbus_connection.ready_for_suspend_event)?

> @@ -2189,6 +2204,7 @@ int vmbus_device_register(struct hv_device *child_device_obj)
>  	ret = device_register(&child_device_obj->device);
>  	if (ret) {
>  		pr_err("Unable to register child device\n");
> +		WRITE_ONCE(child_device_obj->channel, NULL);
>  		put_device(&child_device_obj->device);
>  		return ret;
>  	}

[Severity: Critical]
When device_register() fails here and put_device() frees
child_device_obj via vmbus_device_release(), does newchannel->device_obj
remain a dangling pointer to the freed struct hv_device?

In vmbus_add_channel_work(), newchannel->device_obj is assigned before
calling vmbus_device_register(newchannel->device_obj), and on failure it
jumps to err_deq_chan without clearing newchannel->device_obj:

drivers/hv/channel_mgmt.c:vmbus_add_channel_work() {
    ...
	ret = vmbus_device_register(newchannel->device_obj);

	if (ret != 0) {
		pr_err("unable to add child device object (relid %d)\n",
			newchannel->offermsg.child_relid);
		goto err_deq_chan;
	}
    ...
err_deq_chan:
	mutex_lock(&vmbus_connection.channel_mutex);
    ...
	WRITE_ONCE(newchannel->probe_done, true);
}

If the host rescinds the channel before err_deq_chan unmaps the relid,
vmbus_onoffer_rescind() looks up the channel via relid2channel(), waits
for channel->probe_done to become true, and then checks
channel->device_obj:

drivers/hv/channel_mgmt.c:vmbus_onoffer_rescind() {
    ...
	if (channel->device_obj) {
		if (channel->chn_rescind_callback) {
			channel->chn_rescind_callback(channel);
            ...
			return;
		}
		dev = get_device(&channel->device_obj->device);
		if (dev) {
			vmbus_device_unregister(channel->device_obj);
			put_device(dev);
		}
	}
}

Once err_deq_chan sets probe_done = true and frees newchannel, wouldn't
vmbus_onoffer_rescind() dereference the already-freed
channel->device_obj (and access channel while err_deq_chan is freeing
it)?

> @@ -2205,31 +2221,17 @@ int vmbus_device_register(struct hv_device *child_device_obj)
>  	 * here. The race can't result in a kernel failure, but the user space
>  	 * program may get an error in accessing "channels" or its
>  	 * subdirectories. See also comments with vmbus_add_dynid() about a
> -	 * related race condition.
> +	 * related race condition. For the same reason, the device stays
> +	 * registered if its channel cannot be shown in sysfs.
>  	 */
>  	child_device_obj->channels_kset = kset_create_and_add("channels",
>  							      NULL, kobj);
> -	if (!child_device_obj->channels_kset) {
> -		ret = -ENOMEM;
> -		goto err_dev_unregister;
> -	}
> -
> -	ret = vmbus_add_channel_kobj(child_device_obj,
> -				     child_device_obj->channel);
> -	if (ret) {
> +	if (!child_device_obj->channels_kset ||
> +	    vmbus_add_channel_kobj(child_device_obj, child_device_obj->channel))
>  		pr_err("Unable to register primary channel\n");

[Severity: Medium]
What happens to child_device_obj->channel->kobj and
child_device_obj->channels_kset if kset_create_and_add() succeeds,
kobject_add() inside vmbus_add_channel_kobj() succeeds, and then
sysfs_create_group() fails?

In vmbus_add_channel_kobj(), if sysfs_create_group() fails, kobj is not
removed from sysfs because the function relies on the caller's error
path to clean up the channel:

drivers/hv/vmbus_drv.c:vmbus_add_channel_kobj() {
    ...
	kobj->kset = dev->channels_kset;
	ret = kobject_add(kobj, NULL, "%u", relid);
	if (ret)
		return ret;

	ret = sysfs_create_group(kobj, &vmbus_chan_group);

	if (ret) {
		/*
		 * The calling functions' error handling paths will cleanup the
		 * empty channel directory.
		 */
		dev_err(device, "Unable to set up channel sysfs files\n");
		return ret;
	}
    ...
}

Because vmbus_device_register() no longer unwinds on
vmbus_add_channel_kobj() failure, doesn't this leave an empty
/sys/bus/vmbus/devices/<dev>/channels/<relid> directory in sysfs with
kobj->state_in_sysfs == 1 (causing vmbus_remove_channel_attr_group() to
later call sysfs_remove_group() on a group that failed creation), while
also leaving child_device_obj->channels_kset non-NULL so sub-channels
are not refused as described in the commit message?

[1] https://lore.kernel.org/r/20261102094719.35760-3-izaiahsun@gmail.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103080347.90310-1-izaiahsun@gmail.com?part=2