Subject: RE: [Patch v3 2/3] Drivers: hv: add Azure Blob driver

From: Long Li <longli@microsoft.com> Sent: Friday, July 16, 2021 12:27 PM

[snip]

quoted

quoted

quoted

+
+static int az_blob_connect_to_vsp(struct hv_device *device, u32
+ring_size) {
+	int ret;
+
+	spin_lock_init(&az_blob_dev.file_lock);
+	INIT_LIST_HEAD(&az_blob_dev.file_list);
+	init_waitqueue_head(&az_blob_dev.file_wait);
+
+	az_blob_dev.device = device;
+	device->channel->rqstor_size = AZ_BLOB_QUEUE_DEPTH;
+
+	ret = vmbus_open(device->channel, ring_size, ring_size, NULL, 0,
+			 az_blob_on_channel_callback, device->channel);
+
+	if (ret)
+		return ret;
+
+	hv_set_drvdata(device, &az_blob_dev);
+
+	return ret;
+}
+
+static void az_blob_remove_vmbus(struct hv_device *device) {
+	/* At this point, no VSC/VSP traffic is possible over vmbus */
+	hv_set_drvdata(device, NULL);
+	vmbus_close(device->channel);
+}
+
+static int az_blob_probe(struct hv_device *device,
+			 const struct hv_vmbus_device_id *dev_id) {
+	int ret;
+
+	ret = az_blob_connect_to_vsp(device, AZ_BLOB_RING_SIZE);
+	if (ret) {
+		az_blob_err("error connecting to VSP ret=%d\n", ret);
+		return ret;
+	}
+
+	// create user-mode client library facing device
+	ret = az_blob_create_device(&az_blob_dev);
+	if (ret) {
+		az_blob_err("failed to create device ret=%d\n", ret);
+		az_blob_remove_vmbus(device);
+		return ret;
+	}
+
+	az_blob_dev.removing = false;

This line seems misplaced.  As soon as az_blob_create_device()
returns, some other thread could find the device and open it.  You
don't want the
az_blob_fop_open() function to find the "removing"
flag set to true.  So I think this line should go *before* the call
to az_blob_create_device().

quoted

+	az_blob_info("successfully probed device\n");
+
+	return 0;
+}
+
+static int az_blob_remove(struct hv_device *dev) {
+	az_blob_dev.removing = true;
+	/*
+	 * RCU lock guarantees that any future calls to az_blob_fop_open()
+	 * can not use device resources while the inode reference of
+	 * /dev/azure_blob is being held for that open, and device file is
+	 * being removed from /dev.
+	 */
+	synchronize_rcu();

I don't think this works as you have described.  While it will
ensure that any in-progress RCU read-side critical sections have
completed (i.e., in
az_blob_fop_open() ), it does not prevent new read-side critical
sections from being started.  So as soon as synchronize_rcu() is
finished, another thread could find and open the device, and be
executing in az_blob_fop_open().

But it's not clear to me that this (and the rcu_read_locks in
az_blob_fop_open) are really needed anyway.  If
az_blob_remove_device() is called while one or more threads have it
open, I think that's OK.  Or is there a scenario that I'm missing?

I was trying to address your comment earlier. Here were your comments (1

- 7):

quoted

1) The driver is unbound, so az_blob_remove() is called.
2) az_blob_remove() sets the "removing" flag to true, and calls

az_blob_remove_device().

quoted

3) az_blob_remove_device() waits for the file_list to become empty.
4) After the file_list becomes empty, but before misc_deregister() is called,

a separate thread opens the device again.

quoted

5) In the separate thread, az_blob_fop_open() obtains the file_lock spin

lock.

quoted

6) Before az_blob_fop_open() releases the spin lock, az
block_remove_device() completes, along with az_blob_remove().
7) Then the device gets rebound, and az_blob_connect_to_vsp() gets
called, all while az_blob_fop_open() still holds the spin lock.  So the spin

lock get re-initialized while it is held.

quoted

Between step 4 and step 5, I don't see any guarantee that
az_blob_fop_open() can't run concurrently on another CPU after
misc_deregister() finishes. misc_deregister() calls
devtmpfs_delete_node() to remove the device file from /dev/*, but it

doesn't check the return value, so the inode reference number can be non-
zero after it returns, somebody may still try to open it.

I'm no expert here, but once misc_deregister() finishes, I would expect that
any attempt to open the device with the assigned major:minor number will
fail.  However, existing opens may still be active.   So another thread could
still
have it open, but no new opens can be done.  As coded in this version of
your patch, az_blob_remove() waits until all those existing opens have been
closed,
so everything is clean once the wait is complete.   Of course, while waiting
here, the threads with the open fd could try to start another operation
against the VSP, but the "removing" flag will prevent that.  So the only access
these threads will have is to the singleton az_blob_dev instance and the list
of open files.

But as noted previously, waiting here for the opens to be closed is
problematic because the wait could be arbitrarily long.  And there's
messiness if the device gets re-added later, because there's no distinction
between the old instantiation that a thread might still have open vs. the new
instantiation that you want to initialize fresh.  That's where creating a new
dynamic instance will solve any problems.

As I said in my previous email, I'm moving to dynamically allocated objects to fix all of these.

quoted

This check guarantees that the code can't reference any driver's internal

data structures.

quoted

az_blob_dev.removing is set so this code can't be entered. Resetting
it after az_blob_create_device() is also for this purpose.

quoted

quoted

+
+	az_blob_info("removing device\n");
+	az_blob_remove_device();
+
+	/*
+	 * At this point, it's not possible to open more files.
+	 * Wait for all the opened files to be released.
+	 */
+	wait_event(az_blob_dev.file_wait,
+list_empty(&az_blob_dev.file_list));

As mentioned in my most recent comments on the previous version of
the code, I'm concerned about waiting for all open files to be
released in the case of a VMbus rescind.  We definitely have to wait
for all VSP operations to complete, but that's different from
waiting for the files to be closed.  The former depends on Hyper-V
being well-behaved and will presumably happen quickly in the case of
a rescind.  But the latter depends entirely on user space code that
is out of the kernel's control.  The user space process could hang
around for hours or days with the file still open, which would block this

function from completing, and hence block the global work queue.

quoted

quoted

Thinking about this, the core issue may be that having a single
static instance of az_blob_device is problematic if we allow the
device to be removed (either explicitly with an unbind, or
implicitly with a VMbus
rescind) and then re-added.  Perhaps what needs to happen is that
the removed device is allowed to continue to exist as long as user
space processes have an open file handle, but they can't perform any
operations because the "removing" flag is set (and stays set).
If the device is re-added, then a new instance of az_blob_device
should be created, and whether or not the old instance is still
hanging around is irrelevant.

I agree dynamic device objects is the way to go. Will implement this.

quoted

quoted

+
+	az_blob_remove_vmbus(dev);
+	return 0;
+}
+
+static struct hv_driver az_blob_drv = {
+	.name = KBUILD_MODNAME,
+	.id_table = id_table,
+	.probe = az_blob_probe,
+	.remove = az_blob_remove,
+	.driver = {
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+	},
+};
+
+static int __init az_blob_drv_init(void) {
+	return vmbus_driver_register(&az_blob_drv);
+}
+
+static void __exit az_blob_drv_exit(void) {
+	vmbus_driver_unregister(&az_blob_drv);
+}
+
+MODULE_LICENSE("Dual BSD/GPL");
+MODULE_DESCRIPTION("Microsoft Azure Blob driver");
+module_init(az_blob_drv_init); module_exit(az_blob_drv_exit);

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 705e95d..3095611 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c

@@ -154,6 +154,13 @@
 	  .allowed_in_isolated = false,
 	},

+	/* Azure Blob */
+	{ .dev_type = HV_AZURE_BLOB,
+	  HV_AZURE_BLOB_GUID,
+	  .perf_device = false,
+	  .allowed_in_isolated = false,
+	},
+
 	/* Unknown GUID */
 	{ .dev_type = HV_UNKNOWN,
 	  .perf_device = false,

diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h index

d1e59db..ac31362 100644

--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h

@@ -772,6 +772,7 @@ enum vmbus_device_type {
 	HV_FCOPY,
 	HV_BACKUP,
 	HV_DM,
+	HV_AZURE_BLOB,
 	HV_UNKNOWN,
 };

@@ -1350,6 +1351,14 @@ int vmbus_allocate_mmio(struct resource

**new, struct hv_device *device_obj,

quoted

 			  0x72, 0xe2, 0xff, 0xb1, 0xdc, 0x7f)

 /*
+ * Azure Blob GUID
+ * {0590b792-db64-45cc-81db-b8d70c577c9e}
+ */
+#define HV_AZURE_BLOB_GUID \
+	.guid = GUID_INIT(0x0590b792, 0xdb64, 0x45cc, 0x81, 0xdb, \
+			  0xb8, 0xd7, 0x0c, 0x57, 0x7c, 0x9e)
+
+/*
  * Shutdown GUID
  * {0e0b6031-5213-4934-818b-38d90ced39db}
  */

diff --git a/include/uapi/misc/azure_blob.h

b/include/uapi/misc/azure_blob.h new file mode 100644 index
0000000..f4168679

--- /dev/null
+++ b/include/uapi/misc/azure_blob.h

@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0-only WITH
+Linux-syscall-note */
+/* Copyright (c) Microsoft Corporation. */
+
+#ifndef _AZ_BLOB_H
+#define _AZ_BLOB_H
+
+#include <linux/kernel.h>
+#include <linux/uuid.h>
+
+/* user-mode sync request sent through ioctl */ struct
+az_blob_request_sync_response {
+	__u32 status;
+	__u32 response_len;
+};
+
+struct az_blob_request_sync {
+	guid_t guid;
+	__u32 timeout;
+	__u32 request_len;
+	__u32 response_len;
+	__u32 data_len;
+	__u32 data_valid;
+	__aligned_u64 request_buffer;

Is there an implied 32-bit padding field before "request_buffer"?
It seems like "yes", since there are five 32-bit field preceding.
Would it be better to explicitly list that padding field?

quoted

+	__aligned_u64 response_buffer;
+	__aligned_u64 data_buffer;
+	struct az_blob_request_sync_response response; };
+
+#define AZ_BLOB_MAGIC_NUMBER	'R'
+#define IOCTL_AZ_BLOB_DRIVER_USER_REQUEST \
+		_IOWR(AZ_BLOB_MAGIC_NUMBER, 0xf0, \
+			struct az_blob_request_sync)
+
+#endif /* define _AZ_BLOB_H */
--
1.8.3.1