[PATCH v2 0/2] Drivers: hv: util: Two fixes in util_probe()

[PATCH v2 0/2] Drivers: hv: util: Two fixes in util_probe()

[mhkelley58]

From: Michael Kelley <mhklinux@outlook.com>

Patch 1 fixes util_probe() to not force the error return value to
ENODEV when the util_init function fails -- just return the error
code from util_init so the real error code is displayed in messages.

Patch 2 fixes a more serious race condition between initialization
of the VMBus channel and initial operations of the user space
daemons for KVP and VSS. The fix reorders the initialization in
util_probe() so the race condition can't happen.

The two fixes are functionally independent, but Patch 2 introduces
the util_init_transport function that parallels the existing code
for the util_init function. Doing Patch 1 first avoids an
inconsistency in the error handling in similar code for these two
parts of util_probe().

This series is v2 of a single patch first posted by Dexuan Cui
to fix the race condition.[1] I've taken over the patch per
discussion with Dexuan.

[1] https://lore.kernel.org/linux-hyperv/20240909164719.41000-1-decui@microsoft.com/

Michael Kelley (2):
  Drivers: hv: util: Don't force error code to ENODEV in util_probe()
  Drivers: hv: util: Avoid accessing a ringbuffer not initialized yet

 drivers/hv/hv_kvp.c       |  6 ++++++
 drivers/hv/hv_snapshot.c  |  6 ++++++
 drivers/hv/hv_util.c      | 13 ++++++++++---
 drivers/hv/hyperv_vmbus.h |  2 ++
 include/linux/hyperv.h    |  1 +
 5 files changed, 25 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] Drivers: hv: util: Don't force error code to ENODEV in util_probe()

[mhkelley58]

From: Michael Kelley <mhklinux@outlook.com>

If the util_init function call in util_probe() returns an error code,
util_probe() always return ENODEV, and the error code from the util_init
function is lost. The error message output in the caller, vmbus_probe(),
doesn't show the real error code.

Fix this by just returning the error code from the util_init function.
There doesn't seem to be a reason to force ENODEV, as other errors
such as ENOMEM can already be returned from util_probe(). And the
code in call_driver_probe() implies that ENODEV should mean that a
matching driver wasn't found, which is not the case here.

Suggested-by: Dexuan Cui <decui@microsoft.com>
Signed-off-by: Michael Kelley <mhklinux@outlook.com>
---
Changes in v2: None. This is the first version of Patch 1 of this series.
The "v2" is due to changes to Patch 2 of the series.

 drivers/hv/hv_util.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
index c4f525325790..370722220134 100644
--- a/drivers/hv/hv_util.c
+++ b/drivers/hv/hv_util.c
@@ -590,10 +590,8 @@ static int util_probe(struct hv_device *dev,
 	srv->channel = dev->channel;
 	if (srv->util_init) {
 		ret = srv->util_init(srv);
-		if (ret) {
-			ret = -ENODEV;
+		if (ret)
 			goto error1;
-		}
 	}
 
 	/*
-- 
2.25.1

Re: [PATCH v2 1/2] Drivers: hv: util: Don't force error code to ENODEV in util_probe()

[Saurabh Singh Sengar]

On Wed, Nov 06, 2024 at 07:42:46AM -0800, mhkelley58@gmail.com wrote:
> From: Michael Kelley <mhklinux@outlook.com>
> 
> If the util_init function call in util_probe() returns an error code,
> util_probe() always return ENODEV, and the error code from the util_init
> function is lost. The error message output in the caller, vmbus_probe(),
> doesn't show the real error code.
> 
> Fix this by just returning the error code from the util_init function.
> There doesn't seem to be a reason to force ENODEV, as other errors
> such as ENOMEM can already be returned from util_probe(). And the
> code in call_driver_probe() implies that ENODEV should mean that a
> matching driver wasn't found, which is not the case here.
> 
> Suggested-by: Dexuan Cui <decui@microsoft.com>
> Signed-off-by: Michael Kelley <mhklinux@outlook.com>
> ---
> Changes in v2: None. This is the first version of Patch 1 of this series.
> The "v2" is due to changes to Patch 2 of the series.
> 
>  drivers/hv/hv_util.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> index c4f525325790..370722220134 100644
> --- a/drivers/hv/hv_util.c
> +++ b/drivers/hv/hv_util.c
> @@ -590,10 +590,8 @@ static int util_probe(struct hv_device *dev,
>  	srv->channel = dev->channel;
>  	if (srv->util_init) {
>  		ret = srv->util_init(srv);
> -		if (ret) {
> -			ret = -ENODEV;
> +		if (ret)
>  			goto error1;
> -		}

After reviewing V2 of this series, I couldn’t find any scenario where
'util_init' in any driver returns a value other than 0. In such cases,
could we consider making all these functions 'void' ?

After this ee can remove the check for util_int return type.

- Saurabh

RE: [PATCH v2 1/2] Drivers: hv: util: Don't force error code to ENODEV in util_probe()

[Michael Kelley]

From: Saurabh Singh Sengar <ssengar@linux.microsoft.com> Sent: Sunday, December 8, 2024 9:31 AM
> 
> On Wed, Nov 06, 2024 at 07:42:46AM -0800, mhkelley58@gmail.com wrote:
> > From: Michael Kelley <mhklinux@outlook.com>
> >
> > If the util_init function call in util_probe() returns an error code,
> > util_probe() always return ENODEV, and the error code from the util_init
> > function is lost. The error message output in the caller, vmbus_probe(),
> > doesn't show the real error code.
> >
> > Fix this by just returning the error code from the util_init function.
> > There doesn't seem to be a reason to force ENODEV, as other errors
> > such as ENOMEM can already be returned from util_probe(). And the
> > code in call_driver_probe() implies that ENODEV should mean that a
> > matching driver wasn't found, which is not the case here.
> >
> > Suggested-by: Dexuan Cui <decui@microsoft.com>
> > Signed-off-by: Michael Kelley <mhklinux@outlook.com>
> > ---
> > Changes in v2: None. This is the first version of Patch 1 of this series.
> > The "v2" is due to changes to Patch 2 of the series.
> >
> >  drivers/hv/hv_util.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> > index c4f525325790..370722220134 100644
> > --- a/drivers/hv/hv_util.c
> > +++ b/drivers/hv/hv_util.c
> > @@ -590,10 +590,8 @@ static int util_probe(struct hv_device *dev,
> >  	srv->channel = dev->channel;
> >  	if (srv->util_init) {
> >  		ret = srv->util_init(srv);
> > -		if (ret) {
> > -			ret = -ENODEV;
> > +		if (ret)
> >  			goto error1;
> > -		}
> 
> After reviewing V2 of this series, I couldn’t find any scenario where
> 'util_init' in any driver returns a value other than 0. 

Yeah, I noticed the same thing when doing this patch set.

> In such cases,
> could we consider making all these functions 'void' ?
> 
> After this ee can remove the check for util_int return type.

I decided against making these changes. It seemed like code churn
for not much benefit. And there's the possibility of some future
change reintroducing an error code in one of the util_init functions,
in which case we would need to put things back like they are now.
Certainly this is a judgment call, but my take was to leave things
as they are.

The changes you suggest would probably go as a third patch in
the series. Wei Liu has already picked up the two patches as they
are, so it would be fine to create an independent patch with the
changes you suggest, if we want to go that route. My preference
isn't that strong either way.

Michael

Re: [PATCH v2 1/2] Drivers: hv: util: Don't force error code to ENODEV in util_probe()

[Saurabh Singh Sengar]

On Sun, Dec 08, 2024 at 11:12:15PM +0000, Michael Kelley wrote:
> From: Saurabh Singh Sengar <ssengar@linux.microsoft.com> Sent: Sunday, December 8, 2024 9:31 AM
> > 
> > On Wed, Nov 06, 2024 at 07:42:46AM -0800, mhkelley58@gmail.com wrote:
> > > From: Michael Kelley <mhklinux@outlook.com>
> > >
> > > If the util_init function call in util_probe() returns an error code,
> > > util_probe() always return ENODEV, and the error code from the util_init
> > > function is lost. The error message output in the caller, vmbus_probe(),
> > > doesn't show the real error code.
> > >
> > > Fix this by just returning the error code from the util_init function.
> > > There doesn't seem to be a reason to force ENODEV, as other errors
> > > such as ENOMEM can already be returned from util_probe(). And the
> > > code in call_driver_probe() implies that ENODEV should mean that a
> > > matching driver wasn't found, which is not the case here.
> > >
> > > Suggested-by: Dexuan Cui <decui@microsoft.com>
> > > Signed-off-by: Michael Kelley <mhklinux@outlook.com>
> > > ---
> > > Changes in v2: None. This is the first version of Patch 1 of this series.
> > > The "v2" is due to changes to Patch 2 of the series.
> > >
> > >  drivers/hv/hv_util.c | 4 +---
> > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> > > index c4f525325790..370722220134 100644
> > > --- a/drivers/hv/hv_util.c
> > > +++ b/drivers/hv/hv_util.c
> > > @@ -590,10 +590,8 @@ static int util_probe(struct hv_device *dev,
> > >  	srv->channel = dev->channel;
> > >  	if (srv->util_init) {
> > >  		ret = srv->util_init(srv);
> > > -		if (ret) {
> > > -			ret = -ENODEV;
> > > +		if (ret)
> > >  			goto error1;
> > > -		}
> > 
> > After reviewing V2 of this series, I couldn’t find any scenario where
> > 'util_init' in any driver returns a value other than 0. 
> 
> Yeah, I noticed the same thing when doing this patch set.
> 
> > In such cases,
> > could we consider making all these functions 'void' ?
> > 
> > After this ee can remove the check for util_int return type.
> 
> I decided against making these changes. It seemed like code churn
> for not much benefit. And there's the possibility of some future
> change reintroducing an error code in one of the util_init functions,
> in which case we would need to put things back like they are now.
> Certainly this is a judgment call, but my take was to leave things
> as they are.
> 
> The changes you suggest would probably go as a third patch in
> the series. Wei Liu has already picked up the two patches as they
> are, so it would be fine to create an independent patch with the
> changes you suggest, if we want to go that route. My preference
> isn't that strong either way.

I realized later that the patch is already merged. I believe it's fine
to leave it as is unless someone feels motivated enough to push this change.

- Saurabh

[PATCH v2 2/2] Drivers: hv: util: Avoid accessing a ringbuffer not initialized yet

[mhkelley58]

From: Michael Kelley <mhklinux@outlook.com>

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 reordering the steps in util_probe() so the char dev
entry used by the KVP or VSS daemon is not created until after
vmbus_open() has completed. This reordering prevents the race condition
from happening.

Reported-by: Dexuan Cui <decui@microsoft.com>
Fixes: e0fa3e5e7df6 ("Drivers: hv: utils: fix a race on userspace daemons registration")
Cc: stable@vger.kernel.org
Signed-off-by: Michael Kelley <mhklinux@outlook.com>
---
Changes in v2:

v2 is a completely different approach to fixing the problem compared
to v1 [1]. v1 adds synchronization to deal with the race condition if
it happens. This v2 reorders initialization steps so the race condition
can never happen in the first place.

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.

[1] https://lore.kernel.org/linux-hyperv/20240909164719.41000-1-decui@microsoft.com/

 drivers/hv/hv_kvp.c       | 6 ++++++
 drivers/hv/hv_snapshot.c  | 6 ++++++
 drivers/hv/hv_util.c      | 9 +++++++++
 drivers/hv/hyperv_vmbus.h | 2 ++
 include/linux/hyperv.h    | 1 +
 5 files changed, 24 insertions(+)

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 370722220134..36ee89c0358b 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,
@@ -611,6 +613,13 @@ static int util_probe(struct hv_device *dev,
 	if (ret)
 		goto error;
 
+	if (srv->util_init_transport) {
+		ret = srv->util_init_transport();
+		if (ret) {
+			vmbus_close(dev->channel);
+			goto error;
+		}
+	}
 	return 0;
 
 error:
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);
-- 
2.25.1

RE: [PATCH v2 0/2] Drivers: hv: util: Two fixes in util_probe()

[Michael Kelley]

From: mhkelley58@gmail.com <mhkelley58@gmail.com> Sent: Wednesday, November 6, 2024 7:43 AM
> 
> Patch 1 fixes util_probe() to not force the error return value to
> ENODEV when the util_init function fails -- just return the error
> code from util_init so the real error code is displayed in messages.
> 
> Patch 2 fixes a more serious race condition between initialization
> of the VMBus channel and initial operations of the user space
> daemons for KVP and VSS. The fix reorders the initialization in
> util_probe() so the race condition can't happen.
> 
> The two fixes are functionally independent, but Patch 2 introduces
> the util_init_transport function that parallels the existing code
> for the util_init function. Doing Patch 1 first avoids an
> inconsistency in the error handling in similar code for these two
> parts of util_probe().
> 
> This series is v2 of a single patch first posted by Dexuan Cui
> to fix the race condition.[1] I've taken over the patch per
> discussion with Dexuan.
> 
> [1] https://lore.kernel.org/linux-hyperv/20240909164719.41000-1-decui@microsoft.com/
> 

Gentle ping. :-)

Is anyone in the Linux-on-Hyper-V community able to review this short
patch series?  It's pretty straightforward ....

Michael

> Michael Kelley (2):
>   Drivers: hv: util: Don't force error code to ENODEV in util_probe()
>   Drivers: hv: util: Avoid accessing a ringbuffer not initialized yet
> 
>  drivers/hv/hv_kvp.c       |  6 ++++++
>  drivers/hv/hv_snapshot.c  |  6 ++++++
>  drivers/hv/hv_util.c      | 13 ++++++++++---
>  drivers/hv/hyperv_vmbus.h |  2 ++
>  include/linux/hyperv.h    |  1 +
>  5 files changed, 25 insertions(+), 3 deletions(-)
> 
> --
> 2.25.1
> 

Re: [PATCH v2 0/2] Drivers: hv: util: Two fixes in util_probe()

[Wei Liu]

On Wed, Nov 06, 2024 at 07:42:45AM -0800, mhkelley58@gmail.com wrote:
> From: Michael Kelley <mhklinux@outlook.com>
> 
> Patch 1 fixes util_probe() to not force the error return value to
> ENODEV when the util_init function fails -- just return the error
> code from util_init so the real error code is displayed in messages.
> 
> Patch 2 fixes a more serious race condition between initialization
> of the VMBus channel and initial operations of the user space
> daemons for KVP and VSS. The fix reorders the initialization in
> util_probe() so the race condition can't happen.
> 
> The two fixes are functionally independent, but Patch 2 introduces
> the util_init_transport function that parallels the existing code
> for the util_init function. Doing Patch 1 first avoids an
> inconsistency in the error handling in similar code for these two
> parts of util_probe().
> 
> This series is v2 of a single patch first posted by Dexuan Cui
> to fix the race condition.[1] I've taken over the patch per
> discussion with Dexuan.
> 
> [1] https://lore.kernel.org/linux-hyperv/20240909164719.41000-1-decui@microsoft.com/
> 
> Michael Kelley (2):
>   Drivers: hv: util: Don't force error code to ENODEV in util_probe()
>   Drivers: hv: util: Avoid accessing a ringbuffer not initialized yet

Acked-by: Wei Liu <wei.liu@kernel.org>

Re: [PATCH v2 0/2] Drivers: hv: util: Two fixes in util_probe()

[Wei Liu]

On Sat, Dec 07, 2024 at 07:02:55AM +0000, Wei Liu wrote:
> On Wed, Nov 06, 2024 at 07:42:45AM -0800, mhkelley58@gmail.com wrote:
> > From: Michael Kelley <mhklinux@outlook.com>
> > 
> > Patch 1 fixes util_probe() to not force the error return value to
> > ENODEV when the util_init function fails -- just return the error
> > code from util_init so the real error code is displayed in messages.
> > 
> > Patch 2 fixes a more serious race condition between initialization
> > of the VMBus channel and initial operations of the user space
> > daemons for KVP and VSS. The fix reorders the initialization in
> > util_probe() so the race condition can't happen.
> > 
> > The two fixes are functionally independent, but Patch 2 introduces
> > the util_init_transport function that parallels the existing code
> > for the util_init function. Doing Patch 1 first avoids an
> > inconsistency in the error handling in similar code for these two
> > parts of util_probe().
> > 
> > This series is v2 of a single patch first posted by Dexuan Cui
> > to fix the race condition.[1] I've taken over the patch per
> > discussion with Dexuan.
> > 
> > [1] https://lore.kernel.org/linux-hyperv/20240909164719.41000-1-decui@microsoft.com/
> > 
> > Michael Kelley (2):
> >   Drivers: hv: util: Don't force error code to ENODEV in util_probe()
> >   Drivers: hv: util: Avoid accessing a ringbuffer not initialized yet
> 
> Acked-by: Wei Liu <wei.liu@kernel.org>

Applied to hyperv-fixes, thanks.