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;
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:
- 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().
- 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
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:
- 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().
- 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
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:
- Allocates the receive buffer
- Calls the util_init() function, which for KVP and VSS creates the char dev
- 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
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:
- Allocates the receive buffer
- Calls the util_init() function, which for KVP and VSS creates the char dev
- 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);
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
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
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
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
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!
linux-stable-mirror@lists.linaro.org
-
Dexuan Cui -
Michael Kelley -
Saurabh Singh Sengar