| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<SN6PR02MB4157CDB89A61BA857E6FC0BFD4552@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Thu, 31 Oct 2024 00:11:50 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Dexuan Cui <decui@...rosoft.com>, KY Srinivasan <kys@...rosoft.com>,
Haiyang Zhang <haiyangz@...rosoft.com>, Wei Liu <wei.liu@...nel.org>, Vitaly
Kuznetsov <vkuznets@...hat.com>, Greg Kroah-Hartman
<gregkh@...uxfoundation.org>, "open list:Hyper-V/Azure CORE AND DRIVERS"
<linux-hyperv@...r.kernel.org>, open list <linux-kernel@...r.kernel.org>
CC: "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: RE: [PATCH] Drivers: hv: kvp/vss: Avoid accessing a ringbuffer not
initialized yet
From: Dexuan Cui <decui@...rosoft.com> Sent: Wednesday, October 30, 2024 12:04 PM
>
> > From: Michael Kelley <mhklinux@...look.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);
Powered by blists - more mailing lists