[PATCH] Drivers: hv: kvp/vss: Avoid accessing a ringbuffer not initialized yet

[PATCH] Drivers: hv: kvp/vss: Avoid accessing a ringbuffer not initialized yet

[Dexuan Cui]

If the KVP (or VSS) daemon starts before the VMBus channel's ringbuffer is
fully initialized, we can hit the panic below:

hv_utils: Registering HyperV Utility Driver
hv_vmbus: registering driver hv_utils
...
BUG: kernel NULL pointer dereference, address: 0000000000000000
CPU: 44 UID: 0 PID: 2552 Comm: hv_kvp_daemon Tainted: G E 6.11.0-rc3+ #1
RIP: 0010:hv_pkt_iter_first+0x12/0xd0
Call Trace:
...
 vmbus_recvpacket
 hv_kvp_onchannelcallback
 vmbus_on_event
 tasklet_action_common
 tasklet_action
 handle_softirqs
 irq_exit_rcu
 sysvec_hyperv_stimer0
 </IRQ>
 <TASK>
 asm_sysvec_hyperv_stimer0
...
 kvp_register_done
 hvt_op_read
 vfs_read
 ksys_read
 __x64_sys_read

This can happen because the KVP/VSS channel callback can be invoked
even before the channel is fully opened:
1) as soon as hv_kvp_init() -> hvutil_transport_init() creates
/dev/vmbus/hv_kvp, the kvp daemon can open the device file immediately and
register itself to the driver by writing a message KVP_OP_REGISTER1 to the
file (which is handled by kvp_on_msg() ->kvp_handle_handshake()) and
reading the file for the driver's response, which is handled by
hvt_op_read(), which calls hvt->on_read(), i.e. kvp_register_done().

2) the problem with kvp_register_done() is that it can cause the
channel callback to be called even before the channel is fully opened,
and when the channel callback is starting to run, util_probe()->
vmbus_open() may have not initialized the ringbuffer yet, so the
callback can hit the panic of NULL pointer dereference.

To reproduce the panic consistently, we can add a "ssleep(10)" for KVP in
__vmbus_open(), just before the first hv_ringbuffer_init(), and then we
unload and reload the driver hv_utils, and run the daemon manually within
the 10 seconds.

Fix the panic by checking the channel state in the channel callback.
To avoid the race condition with __vmbus_open(), we disable and enable
the channel callback temporarily in __vmbus_open().

The channel callbacks of the other VMBus devices don't need to check
the channel state since they can't run before the channels are fully
initialized.

Note: we would also need to fix the fcopy driver code, but that has
been removed in commit ec314f61e4fc ("Drivers: hv: Remove fcopy driver") in
the mainline kernel since v6.10. For old 6.x LTS kernels, and the 5.x
and 4.x LTS kernels, the fcopy driver needs to be fixed when the
fix is backported to the stable kernel branches.

Fixes: e0fa3e5e7df6 ("Drivers: hv: utils: fix a race on userspace daemons registration")
Cc: stable@vger.kernel.org
Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 drivers/hv/channel.c     | 11 +++++++++++
 drivers/hv/hv_kvp.c      |  3 +++
 drivers/hv/hv_snapshot.c |  3 +++
 3 files changed, 17 insertions(+)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index fb8cd8469328..685e407a3fdf 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -657,6 +657,14 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
 			return -ENOMEM;
 	}
 
+	/*
+	 * The channel callbacks of KVP/VSS may run before __vmbus_open()
+	 * finishes (see kvp_register_done() -> ... -> kvp_poll_wrapper()), so
+	 * they check newchannel->state to tell the ringbuffer has been fully
+	 * initialized or not. Disable and enable the tasklet to avoid the race.
+	 */
+	tasklet_disable(&newchannel->callback_event);
+
 	newchannel->state = CHANNEL_OPENING_STATE;
 	newchannel->onchannel_callback = onchannelcallback;
 	newchannel->channel_callback_context = context;
@@ -750,6 +758,8 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
 	}
 
 	newchannel->state = CHANNEL_OPENED_STATE;
+	tasklet_enable(&newchannel->callback_event);
+
 	kfree(open_info);
 	return 0;
 
@@ -766,6 +776,7 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
 	hv_ringbuffer_cleanup(&newchannel->inbound);
 	vmbus_free_requestor(&newchannel->requestor);
 	newchannel->state = CHANNEL_OPEN_STATE;
+	tasklet_enable(&newchannel->callback_event);
 	return err;
 }
 
diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
index d35b60c06114..ec098067e579 100644
--- a/drivers/hv/hv_kvp.c
+++ b/drivers/hv/hv_kvp.c
@@ -662,6 +662,9 @@ void hv_kvp_onchannelcallback(void *context)
 	if (kvp_transaction.state > HVUTIL_READY)
 		return;
 
+	if (channel->state != CHANNEL_OPENED_STATE)
+		return;
+
 	if (vmbus_recvpacket(channel, recv_buffer, HV_HYP_PAGE_SIZE * 4, &recvlen, &requestid)) {
 		pr_err_ratelimited("KVP request received. Could not read into recv buf\n");
 		return;
diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c
index 0d2184be1691..f7924c2fc62e 100644
--- a/drivers/hv/hv_snapshot.c
+++ b/drivers/hv/hv_snapshot.c
@@ -301,6 +301,9 @@ void hv_vss_onchannelcallback(void *context)
 	if (vss_transaction.state > HVUTIL_READY)
 		return;
 
+	if (channel->state != CHANNEL_OPENED_STATE)
+		return;
+
 	if (vmbus_recvpacket(channel, recv_buffer, VSS_MAX_PKT_SIZE, &recvlen, &requestid)) {
 		pr_err_ratelimited("VSS request received. Could not read into recv buf\n");
 		return;
-- 
2.25.1

RE: [EXTERNAL] [PATCH] Drivers: hv: kvp/vss: Avoid accessing a ringbuffer not initialized yet

[Saurabh Singh Sengar]

> Subject: [EXTERNAL] [PATCH] Drivers: hv: kvp/vss: Avoid accessing a ringbuffer
> not initialized yet
> 
> If the KVP (or VSS) daemon starts before the VMBus channel's ringbuffer is
> fully initialized, we can hit the panic below:
> 
> hv_utils: Registering HyperV Utility Driver
> hv_vmbus: registering driver hv_utils
> ...
> BUG: kernel NULL pointer dereference, address: 0000000000000000
> CPU: 44 UID: 0 PID: 2552 Comm: hv_kvp_daemon Tainted: G E 6.11.0-rc3+ #1
> RIP: 0010:hv_pkt_iter_first+0x12/0xd0
> Call Trace:
> ...
>  vmbus_recvpacket
>  hv_kvp_onchannelcallback
>  vmbus_on_event
>  tasklet_action_common
>  tasklet_action
>  handle_softirqs
>  irq_exit_rcu
>  sysvec_hyperv_stimer0
>  </IRQ>
>  <TASK>
>  asm_sysvec_hyperv_stimer0
> ...
>  kvp_register_done
>  hvt_op_read
>  vfs_read
>  ksys_read
>  __x64_sys_read
> 
> This can happen because the KVP/VSS channel callback can be invoked even
> before the channel is fully opened:
> 1) as soon as hv_kvp_init() -> hvutil_transport_init() creates
> /dev/vmbus/hv_kvp, the kvp daemon can open the device file immediately
> and register itself to the driver by writing a message KVP_OP_REGISTER1 to
> the file (which is handled by kvp_on_msg() ->kvp_handle_handshake()) and
> reading the file for the driver's response, which is handled by hvt_op_read(),
> which calls hvt->on_read(), i.e. kvp_register_done().
> 
> 2) the problem with kvp_register_done() is that it can cause the channel
> callback to be called even before the channel is fully opened, and when the
> channel callback is starting to run, util_probe()->
> vmbus_open() may have not initialized the ringbuffer yet, so the callback can
> hit the panic of NULL pointer dereference.
> 
> To reproduce the panic consistently, we can add a "ssleep(10)" for KVP in
> __vmbus_open(), just before the first hv_ringbuffer_init(), and then we
> unload and reload the driver hv_utils, and run the daemon manually within
> the 10 seconds.
> 
> Fix the panic by checking the channel state in the channel callback.
> To avoid the race condition with __vmbus_open(), we disable and enable the
> channel callback temporarily in __vmbus_open().
> 
> The channel callbacks of the other VMBus devices don't need to check the
> channel state since they can't run before the channels are fully initialized.
> 
> Note: we would also need to fix the fcopy driver code, but that has been
> removed in commit ec314f61e4fc ("Drivers: hv: Remove fcopy driver") in the
> mainline kernel since v6.10. For old 6.x LTS kernels, and the 5.x and 4.x LTS
> kernels, the fcopy driver needs to be fixed when the fix is backported to the
> stable kernel branches.
> 
> Fixes: e0fa3e5e7df6 ("Drivers: hv: utils: fix a race on userspace daemons
> registration")
> Cc: stable@vger.kernel.org
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  drivers/hv/channel.c     | 11 +++++++++++
>  drivers/hv/hv_kvp.c      |  3 +++
>  drivers/hv/hv_snapshot.c |  3 +++
>  3 files changed, 17 insertions(+)
> 
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c index
> fb8cd8469328..685e407a3fdf 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -657,6 +657,14 @@ static int __vmbus_open(struct vmbus_channel
> *newchannel,
>  			return -ENOMEM;
>  	}
> 
> +	/*
> +	 * The channel callbacks of KVP/VSS may run before __vmbus_open()
> +	 * finishes (see kvp_register_done() -> ... -> kvp_poll_wrapper()), so
> +	 * they check newchannel->state to tell the ringbuffer has been fully
> +	 * initialized or not. Disable and enable the tasklet to avoid the race.
> +	 */
> +	tasklet_disable(&newchannel->callback_event);
> +
>  	newchannel->state = CHANNEL_OPENING_STATE;
>  	newchannel->onchannel_callback = onchannelcallback;
>  	newchannel->channel_callback_context = context; @@ -750,6 +758,8
> @@ static int __vmbus_open(struct vmbus_channel *newchannel,
>  	}
> 
>  	newchannel->state = CHANNEL_OPENED_STATE;
> +	tasklet_enable(&newchannel->callback_event);
> +
>  	kfree(open_info);
>  	return 0;
> 
> @@ -766,6 +776,7 @@ static int __vmbus_open(struct vmbus_channel
> *newchannel,
>  	hv_ringbuffer_cleanup(&newchannel->inbound);
>  	vmbus_free_requestor(&newchannel->requestor);
>  	newchannel->state = CHANNEL_OPEN_STATE;
> +	tasklet_enable(&newchannel->callback_event);
>  	return err;
>  }
> 
> diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c index
> d35b60c06114..ec098067e579 100644
> --- a/drivers/hv/hv_kvp.c
> +++ b/drivers/hv/hv_kvp.c
> @@ -662,6 +662,9 @@ void hv_kvp_onchannelcallback(void *context)
>  	if (kvp_transaction.state > HVUTIL_READY)
>  		return;
> 
> +	if (channel->state != CHANNEL_OPENED_STATE)
> +		return;
> +
>  	if (vmbus_recvpacket(channel, recv_buffer, HV_HYP_PAGE_SIZE * 4,
> &recvlen, &requestid)) {
>  		pr_err_ratelimited("KVP request received. Could not read into
> recv buf\n");
>  		return;
> diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c index
> 0d2184be1691..f7924c2fc62e 100644
> --- a/drivers/hv/hv_snapshot.c
> +++ b/drivers/hv/hv_snapshot.c
> @@ -301,6 +301,9 @@ void hv_vss_onchannelcallback(void *context)
>  	if (vss_transaction.state > HVUTIL_READY)
>  		return;
> 
> +	if (channel->state != CHANNEL_OPENED_STATE)
> +		return;
> +
>  	if (vmbus_recvpacket(channel, recv_buffer, VSS_MAX_PKT_SIZE,
> &recvlen, &requestid)) {
>  		pr_err_ratelimited("VSS request received. Could not read into
> recv buf\n");
>  		return;
> --
> 2.25.1
> 

Thanks for the fix.
Reviewed-by: Saurabh Sengar <ssengar@linux.microsoft.com>

RE: [PATCH] Drivers: hv: kvp/vss: Avoid accessing a ringbuffer not initialized yet

[Michael Kelley]

From: Dexuan Cui <decui@microsoft.com> Sent: Monday, September 9, 2024 9:47 AM
> 
> If the KVP (or VSS) daemon starts before the VMBus channel's ringbuffer is
> fully initialized, we can hit the panic below:
> 
> hv_utils: Registering HyperV Utility Driver
> hv_vmbus: registering driver hv_utils
> ...
> BUG: kernel NULL pointer dereference, address: 0000000000000000
> CPU: 44 UID: 0 PID: 2552 Comm: hv_kvp_daemon Tainted: G E 6.11.0-rc3+ #1
> RIP: 0010:hv_pkt_iter_first+0x12/0xd0
> Call Trace:
> ...
>  vmbus_recvpacket
>  hv_kvp_onchannelcallback
>  vmbus_on_event
>  tasklet_action_common
>  tasklet_action
>  handle_softirqs
>  irq_exit_rcu
>  sysvec_hyperv_stimer0
>  </IRQ>
>  <TASK>
>  asm_sysvec_hyperv_stimer0
> ...
>  kvp_register_done
>  hvt_op_read
>  vfs_read
>  ksys_read
>  __x64_sys_read
> 
> This can happen because the KVP/VSS channel callback can be invoked
> even before the channel is fully opened:
> 1) as soon as hv_kvp_init() -> hvutil_transport_init() creates
> /dev/vmbus/hv_kvp, the kvp daemon can open the device file immediately and
> register itself to the driver by writing a message KVP_OP_REGISTER1 to the
> file (which is handled by kvp_on_msg() ->kvp_handle_handshake()) and
> reading the file for the driver's response, which is handled by
> hvt_op_read(), which calls hvt->on_read(), i.e. kvp_register_done().
> 
> 2) the problem with kvp_register_done() is that it can cause the
> channel callback to be called even before the channel is fully opened,
> and when the channel callback is starting to run, util_probe()->
> vmbus_open() may have not initialized the ringbuffer yet, so the
> callback can hit the panic of NULL pointer dereference.
> 
> To reproduce the panic consistently, we can add a "ssleep(10)" for KVP in
> __vmbus_open(), just before the first hv_ringbuffer_init(), and then we
> unload and reload the driver hv_utils, and run the daemon manually within
> the 10 seconds.
> 
> Fix the panic by checking the channel state in the channel callback.
> To avoid the race condition with __vmbus_open(), we disable and enable
> the channel callback temporarily in __vmbus_open().
> 
> The channel callbacks of the other VMBus devices don't need to check
> the channel state since they can't run before the channels are fully
> initialized.
> 
> Note: we would also need to fix the fcopy driver code, but that has
> been removed in commit ec314f61e4fc ("Drivers: hv: Remove fcopy driver") in
> the mainline kernel since v6.10. For old 6.x LTS kernels, and the 5.x
> and 4.x LTS kernels, the fcopy driver needs to be fixed when the
> fix is backported to the stable kernel branches.

An alternate approach occurs to me. util_probe() does these three
things in order:

1) Allocates the receive buffer
2) Calls the util_init() function, which for KVP and VSS creates the char dev
3) Sets up the VMBus channel, including calling vmbus_open()

What if the order of #2 and #3 were swapped in util_probe()? I
don't immediately see any interdependency between #2 and #3
for KVP and VSS, nor for Shutdown and Timesync. With the swap,
the VMBus channel would be fully open by the time the /dev entry
appears and the user space daemon can do anything.

I haven't though too deeply about this, so maybe there's a problem
somewhere. But if not, it seems a lot cleaner.

Michael

> 
> Fixes: e0fa3e5e7df6 ("Drivers: hv: utils: fix a race on userspace daemons registration")
> Cc: stable@vger.kernel.org
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  drivers/hv/channel.c     | 11 +++++++++++
>  drivers/hv/hv_kvp.c      |  3 +++
>  drivers/hv/hv_snapshot.c |  3 +++
>  3 files changed, 17 insertions(+)
> 
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index fb8cd8469328..685e407a3fdf 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -657,6 +657,14 @@ static int __vmbus_open(struct vmbus_channel
> *newchannel,
>  			return -ENOMEM;
>  	}
> 
> +	/*
> +	 * The channel callbacks of KVP/VSS may run before __vmbus_open()
> +	 * finishes (see kvp_register_done() -> ... -> kvp_poll_wrapper()), so
> +	 * they check newchannel->state to tell the ringbuffer has been fully
> +	 * initialized or not. Disable and enable the tasklet to avoid the race.
> +	 */
> +	tasklet_disable(&newchannel->callback_event);
> +
>  	newchannel->state = CHANNEL_OPENING_STATE;
>  	newchannel->onchannel_callback = onchannelcallback;
>  	newchannel->channel_callback_context = context;
> @@ -750,6 +758,8 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
>  	}
> 
>  	newchannel->state = CHANNEL_OPENED_STATE;
> +	tasklet_enable(&newchannel->callback_event);
> +
>  	kfree(open_info);
>  	return 0;
> 
> @@ -766,6 +776,7 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
>  	hv_ringbuffer_cleanup(&newchannel->inbound);
>  	vmbus_free_requestor(&newchannel->requestor);
>  	newchannel->state = CHANNEL_OPEN_STATE;
> +	tasklet_enable(&newchannel->callback_event);
>  	return err;
>  }
> 
> diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
> index d35b60c06114..ec098067e579 100644
> --- a/drivers/hv/hv_kvp.c
> +++ b/drivers/hv/hv_kvp.c
> @@ -662,6 +662,9 @@ void hv_kvp_onchannelcallback(void *context)
>  	if (kvp_transaction.state > HVUTIL_READY)
>  		return;
> 
> +	if (channel->state != CHANNEL_OPENED_STATE)
> +		return;
> +
>  	if (vmbus_recvpacket(channel, recv_buffer, HV_HYP_PAGE_SIZE * 4, &recvlen, &requestid)) {
>  		pr_err_ratelimited("KVP request received. Could not read into recv buf\n");
>  		return;
> diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c
> index 0d2184be1691..f7924c2fc62e 100644
> --- a/drivers/hv/hv_snapshot.c
> +++ b/drivers/hv/hv_snapshot.c
> @@ -301,6 +301,9 @@ void hv_vss_onchannelcallback(void *context)
>  	if (vss_transaction.state > HVUTIL_READY)
>  		return;
> 
> +	if (channel->state != CHANNEL_OPENED_STATE)
> +		return;
> +
>  	if (vmbus_recvpacket(channel, recv_buffer, VSS_MAX_PKT_SIZE, &recvlen, &requestid)) {
>  		pr_err_ratelimited("VSS request received. Could not read into recv buf\n");
>  		return;
> --
> 2.25.1

RE: [PATCH] Drivers: hv: kvp/vss: Avoid accessing a ringbuffer not initialized yet

[Dexuan Cui]

> From: Michael Kelley <mhklinux@outlook.com>
> Sent: Tuesday, October 29, 2024 4:45 PM
> [...]
> An alternate approach occurs to me. util_probe() does these three
> things in order:
> 
> 1) Allocates the receive buffer
> 2) Calls the util_init() function, which for KVP and VSS creates the char dev
> 3) Sets up the VMBus channel, including calling vmbus_open()
> 
> What if the order of #2 and #3 were swapped in util_probe()? I
> don't immediately see any interdependency between #2 and #3
> for KVP and VSS, nor for Shutdown and Timesync. With the swap,
> the VMBus channel would be fully open by the time the /dev entry
> appears and the user space daemon can do anything.
> 
> I haven't though too deeply about this, so maybe there's a problem
> somewhere. But if not, it seems a lot cleaner.
> 
> Michael

I think #3 depends on #2, e.g. hv_kvp_init() sets the channel's
preferred max_pkt_size, which is tested later in __vmbus_open().

Another example of dependency is: hv_timesync_init() initializes
host_ts.lock and adj_time_work, which are used by
timesync_onchannelcallback() -> adj_guesttime().
Note: the channel callback can be already running before
vmbus_open() returns.

Thanks,
Dexuan

RE: [PATCH] Drivers: hv: kvp/vss: Avoid accessing a ringbuffer not initialized yet

[Michael Kelley]

From: Dexuan Cui <decui@microsoft.com> Sent: Wednesday, October 30, 2024 12:04 PM
> 
> > From: Michael Kelley <mhklinux@outlook.com>
> > Sent: Tuesday, October 29, 2024 4:45 PM
> > [...]
> > An alternate approach occurs to me. util_probe() does these three
> > things in order:
> >
> > 1) Allocates the receive buffer
> > 2) Calls the util_init() function, which for KVP and VSS creates the char dev
> > 3) Sets up the VMBus channel, including calling vmbus_open()
> >
> > What if the order of #2 and #3 were swapped in util_probe()? I
> > don't immediately see any interdependency between #2 and #3
> > for KVP and VSS, nor for Shutdown and Timesync. With the swap,
> > the VMBus channel would be fully open by the time the /dev entry
> > appears and the user space daemon can do anything.
> >
> > I haven't though too deeply about this, so maybe there's a problem
> > somewhere. But if not, it seems a lot cleaner.
> >
> > Michael
> 
> I think #3 depends on #2, e.g. hv_kvp_init() sets the channel's
> preferred max_pkt_size, which is tested later in __vmbus_open().
> 
> Another example of dependency is: hv_timesync_init() initializes
> host_ts.lock and adj_time_work, which are used by
> timesync_onchannelcallback() -> adj_guesttime().
> Note: the channel callback can be already running before
> vmbus_open() returns.
> 

Indeed, you are right. I should have looked a little more closely. :-(

What do you think about this (compile tested only), which splits the
"init" function into two parts for devices that have char devs? I'm
trying to avoid adding yet another synchronization point by just
doing the init operations in the right order -- i.e., don't create the
user space /dev entry until the VMBus channel is ready.

Michael

diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
index d35b60c06114..77017d951826 100644
--- a/drivers/hv/hv_kvp.c
+++ b/drivers/hv/hv_kvp.c
@@ -767,6 +767,12 @@ hv_kvp_init(struct hv_util_service *srv)
 	 */
 	kvp_transaction.state = HVUTIL_DEVICE_INIT;
 
+	return 0;
+}
+
+int
+hv_kvp_init_transport(void)
+{
 	hvt = hvutil_transport_init(kvp_devname, CN_KVP_IDX, CN_KVP_VAL,
 				    kvp_on_msg, kvp_on_reset);
 	if (!hvt)
diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c
index 0d2184be1691..397f4c8fa46c 100644
--- a/drivers/hv/hv_snapshot.c
+++ b/drivers/hv/hv_snapshot.c
@@ -388,6 +388,12 @@ hv_vss_init(struct hv_util_service *srv)
 	 */
 	vss_transaction.state = HVUTIL_DEVICE_INIT;
 
+	return 0;
+}
+
+int
+hv_vss_init_transport(void)
+{
 	hvt = hvutil_transport_init(vss_devname, CN_VSS_IDX, CN_VSS_VAL,
 				    vss_on_msg, vss_on_reset);
 	if (!hvt) {
diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
index c4f525325790..025d9b9e509b 100644
--- a/drivers/hv/hv_util.c
+++ b/drivers/hv/hv_util.c
@@ -141,6 +141,7 @@ static struct hv_util_service util_heartbeat = {
 static struct hv_util_service util_kvp = {
 	.util_cb = hv_kvp_onchannelcallback,
 	.util_init = hv_kvp_init,
+	.util_init_transport = hv_kvp_init_transport,
 	.util_pre_suspend = hv_kvp_pre_suspend,
 	.util_pre_resume = hv_kvp_pre_resume,
 	.util_deinit = hv_kvp_deinit,
@@ -149,6 +150,7 @@ static struct hv_util_service util_kvp = {
 static struct hv_util_service util_vss = {
 	.util_cb = hv_vss_onchannelcallback,
 	.util_init = hv_vss_init,
+	.util_init_transport = hv_vss_init_transport,
 	.util_pre_suspend = hv_vss_pre_suspend,
 	.util_pre_resume = hv_vss_pre_resume,
 	.util_deinit = hv_vss_deinit,
@@ -613,8 +615,18 @@ static int util_probe(struct hv_device *dev,
 	if (ret)
 		goto error;
 
+	if (srv->util_init_transport) {
+		ret = srv->util_init_transport();
+		if (ret) {
+			ret = -ENODEV;
+			goto error2;
+		}
+	}
 	return 0;
 
+error2:
+	vmbus_close(dev->channel);
+
 error:
 	if (srv->util_deinit)
 		srv->util_deinit();
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index d2856023d53c..52cb744b4d7f 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -370,12 +370,14 @@ void vmbus_on_event(unsigned long data);
 void vmbus_on_msg_dpc(unsigned long data);
 
 int hv_kvp_init(struct hv_util_service *srv);
+int hv_kvp_init_transport(void);
 void hv_kvp_deinit(void);
 int hv_kvp_pre_suspend(void);
 int hv_kvp_pre_resume(void);
 void hv_kvp_onchannelcallback(void *context);
 
 int hv_vss_init(struct hv_util_service *srv);
+int hv_vss_init_transport(void);
 void hv_vss_deinit(void);
 int hv_vss_pre_suspend(void);
 int hv_vss_pre_resume(void);
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 22c22fb91042..02a226bcf0ed 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1559,6 +1559,7 @@ struct hv_util_service {
 	void *channel;
 	void (*util_cb)(void *);
 	int (*util_init)(struct hv_util_service *);
+	int (*util_init_transport)(void);
 	void (*util_deinit)(void);
 	int (*util_pre_suspend)(void);
 	int (*util_pre_resume)(void);

RE: [PATCH] Drivers: hv: kvp/vss: Avoid accessing a ringbuffer not initialized yet

[Dexuan Cui]

> From: Michael Kelley <mhklinux@outlook.com>
> Sent: Wednesday, October 30, 2024 5:12 PM
> [...]
> What do you think about this (compile tested only), which splits the
> "init" function into two parts for devices that have char devs? I'm
> trying to avoid adding yet another synchronization point by just
> doing the init operations in the right order -- i.e., don't create the
> user space /dev entry until the VMBus channel is ready.
> 
> Michael

Thanks, I think this works! This is a better fix.

> +	if (srv->util_init_transport) {
> +		ret = srv->util_init_transport();
> +		if (ret) {
> +			ret = -ENODEV;
IMO we don't need the line above, since the 'ret' from 
srv->util_init_transport()  is already a standard error code.

BTW, I noticed that the line "ret = -ENODEV;"
        if (srv->util_init) {
                ret = srv->util_init(srv);
                if (ret) {
                        ret = -ENODEV;
                        goto error1;
                }
        }
I think we don't really need that line, either. 
The existing 4 .util_init callbacks also already return a
standard error code. We can make a separate patch to clean
that up.

Thanks,
Dexuan

RE: [PATCH] Drivers: hv: kvp/vss: Avoid accessing a ringbuffer not initialized yet

[Michael Kelley]

From: Dexuan Cui <decui@microsoft.com> Sent: Thursday, October 31, 2024 5:17 PM
> 
> > From: Michael Kelley <mhklinux@outlook.com>
> > Sent: Wednesday, October 30, 2024 5:12 PM
> > [...]
> > What do you think about this (compile tested only), which splits the
> > "init" function into two parts for devices that have char devs? I'm
> > trying to avoid adding yet another synchronization point by just
> > doing the init operations in the right order -- i.e., don't create the
> > user space /dev entry until the VMBus channel is ready.
> >
> > Michael
> 
> Thanks, I think this works! This is a better fix.
> 
> > +	if (srv->util_init_transport) {
> > +		ret = srv->util_init_transport();
> > +		if (ret) {
> > +			ret = -ENODEV;
> IMO we don't need the line above, since the 'ret' from
> srv->util_init_transport()  is already a standard error code.

I was just now looking at call_driver_probe(), and it behaves
slightly differently for ENODEV and ENXIO vs. other error
codes. ENODEV and ENXIO don't output a message to the
console unless debugging is enabled, while other error codes
always output a message to the console. Forcing the error to
ENODEV has been there since the util_probe() code came out
of staging in year 2011. But I don't really have a preference
either way.

> 
> BTW, I noticed that the line "ret = -ENODEV;"
>         if (srv->util_init) {
>                 ret = srv->util_init(srv);
>                 if (ret) {
>                         ret = -ENODEV;
>                         goto error1;
>                 }
>         }
> I think we don't really need that line, either.
> The existing 4 .util_init callbacks also already return a
> standard error code. We can make a separate patch to clean
> that up.

Same here.

Michael

RE: [PATCH] Drivers: hv: kvp/vss: Avoid accessing a ringbuffer not initialized yet

[Dexuan Cui]

> From: Michael Kelley <mhklinux@outlook.com>
> Sent: Thursday, October 31, 2024 6:39 PM
> > > From: Michael Kelley <mhklinux@outlook.com>
> > > Sent: Wednesday, October 30, 2024 5:12 PM
> > > [...]
> > > What do you think about this (compile tested only), which splits the
> > > "init" function into two parts for devices that have char devs? I'm
> > > trying to avoid adding yet another synchronization point by just
> > > doing the init operations in the right order -- i.e., don't create the
> > > user space /dev entry until the VMBus channel is ready.
> > >
> > > Michael
> >
> > Thanks, I think this works! This is a better fix.

Michael, will you post a formal patch or want me to do it?
Either works for me.

> > > +	if (srv->util_init_transport) {
> > > +		ret = srv->util_init_transport();
> > > +		if (ret) {
> > > +			ret = -ENODEV;
> > IMO we don't need the line above, since the 'ret' from
> > srv->util_init_transport()  is already a standard error code.
> 
> I was just now looking at call_driver_probe(), and it behaves
> slightly differently for ENODEV and ENXIO vs. other error
> codes. ENODEV and ENXIO don't output a message to the
> console unless debugging is enabled, while other error codes
> always output a message to the console. Forcing the error to
> ENODEV has been there since the util_probe() code came out
> of staging in year 2011. But I don't really have a preference
> either way.

util_probe() is called by vmbus_probe(), which uses pr_err() to print
the 'ret'. If the 'ret' is forced to ENODEV, the message in vmbus_probe()
may be a little misleading since the real error code is hidden,
especially when srv->util_init_transport() doesn't print any error
message.

vmbus_probe() is called by call_driver_probe. I guess originally
KY wanted to use ENODEV to avoid the extra message for the util
devices in call_driver_probe() in non-debugging mode, but the other
VSC drivers don't follow this usage.

util_probe() can return a non-ENODEV error code anyway, e.g.
ENOMEM and whatever error code from vmbus_open(). IMO,
srv->util_init and srv->util_init_transport should not be treated
specially.

IMO it's better to not add new code to force the 'ret' to
ENODEV, and we'd want to clean up the existing use of ENODEV
in util_probe().
 
Thanks,
Dexuan

RE: [PATCH] Drivers: hv: kvp/vss: Avoid accessing a ringbuffer not initialized yet

[Michael Kelley]

From: Dexuan Cui <decui@microsoft.com> Sent: Friday, November 1, 2024 1:27 PM
> 
> > From: Michael Kelley <mhklinux@outlook.com>
> > Sent: Thursday, October 31, 2024 6:39 PM
> > > > From: Michael Kelley <mhklinux@outlook.com>
> > > > Sent: Wednesday, October 30, 2024 5:12 PM
> > > > [...]
> > > > What do you think about this (compile tested only), which splits the
> > > > "init" function into two parts for devices that have char devs? I'm
> > > > trying to avoid adding yet another synchronization point by just
> > > > doing the init operations in the right order -- i.e., don't create the
> > > > user space /dev entry until the VMBus channel is ready.
> > > >
> > > > Michael
> > >
> > > Thanks, I think this works! This is a better fix.
> 
> Michael, will you post a formal patch or want me to do it?
> Either works for me.

I can do it. You probably have more pressing issues to keep
you busy .... :-)

> 
> > > > +	if (srv->util_init_transport) {
> > > > +		ret = srv->util_init_transport();
> > > > +		if (ret) {
> > > > +			ret = -ENODEV;
> > > IMO we don't need the line above, since the 'ret' from
> > > srv->util_init_transport()  is already a standard error code.
> >
> > I was just now looking at call_driver_probe(), and it behaves
> > slightly differently for ENODEV and ENXIO vs. other error
> > codes. ENODEV and ENXIO don't output a message to the
> > console unless debugging is enabled, while other error codes
> > always output a message to the console. Forcing the error to
> > ENODEV has been there since the util_probe() code came out
> > of staging in year 2011. But I don't really have a preference
> > either way.
> 
> util_probe() is called by vmbus_probe(), which uses pr_err() to print
> the 'ret'. If the 'ret' is forced to ENODEV, the message in vmbus_probe()
> may be a little misleading since the real error code is hidden,
> especially when srv->util_init_transport() doesn't print any error
> message.
> 
> vmbus_probe() is called by call_driver_probe. I guess originally
> KY wanted to use ENODEV to avoid the extra message for the util
> devices in call_driver_probe() in non-debugging mode, but the other
> VSC drivers don't follow this usage.
> 
> util_probe() can return a non-ENODEV error code anyway, e.g.
> ENOMEM and whatever error code from vmbus_open(). IMO,
> srv->util_init and srv->util_init_transport should not be treated
> specially.
> 
> IMO it's better to not add new code to force the 'ret' to
> ENODEV, and we'd want to clean up the existing use of ENODEV
> in util_probe().
> 

Fair enough. I'll do it that way.

Michael

RE: [PATCH] Drivers: hv: kvp/vss: Avoid accessing a ringbuffer not initialized yet

[Dexuan Cui]

> From: Michael Kelley <mhklinux@outlook.com>
> Sent: Friday, November 1, 2024 2:11 PM
> > Michael, will you post a formal patch or want me to do it?
> > Either works for me.
> 
> I can do it. You probably have more pressing issues to keep
> you busy .... :-)
Thanks a lot! :-)

> Fair enough. I'll do it that way.
> 
> Michael

Thanks!