[PATCH 0/5] firewire: cdev: refactor for bus reset event filling

[PATCH 0/5] firewire: cdev: refactor for bus reset event filling

[Takashi Sakamoto]

Hi,

In the cdev layer, there are two places where bus reset events are filed;
the update path for the associated fw_device instance and the get_info
ioctl call. There is still some code that can be refactored in these
paths.

This series refactors the code to make the memory ordering and locking
scope more explicit, and adds a KUnit test for the bus reset event UAPI
structure.

Takashi Sakamoto (5):
  firewire: cdev: use memory barrier to access fw_device members
  firewire: cdev: refactor using scoped_guard() for get_info ioctl call
  firewire: cdev: refactor copy_to_user() in get_info ioctl call
  firewire: cdev: add annotations and comments about race condition of
    bus_reset_closure
  firewire: cdev: add KUnit test for bus reset event UAPI structure

 drivers/firewire/core-cdev.c | 63 +++++++++++++++++++++++-------------
 drivers/firewire/uapi-test.c | 20 ++++++++++++
 2 files changed, 60 insertions(+), 23 deletions(-)


base-commit: a20545fbf9226591017fc371bdfb1bfd490b0e0b
-- 
2.53.0

[PATCH 1/5] firewire: cdev: use memory barrier to access fw_device members

[Takashi Sakamoto]

The core code uses a read memory barrier when accessing the generation
and node_id members of the fw_device structure. It assumes that the
latest value of the former is visible to a processor core when the latest
value of the latter is visible, since these members are updated in reverse
order with a write memory barrier.

However, the function that fills the fw_cdev_event_bus_reset structure
does not follow this approach. Use a read memory barrier when accessing
these members.
---
 drivers/firewire/core-cdev.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index e49d8a58be09..9a3be1ceee2a 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -399,18 +399,21 @@ static ssize_t fw_device_op_read(struct file *file, char __user *buffer,
 static void fill_bus_reset_event(struct fw_cdev_event_bus_reset *event,
 				 struct client *client)
 {
-	struct fw_card *card = client->device->card;
-
-	guard(spinlock_irq)(&card->lock);
-
 	event->closure	     = client->bus_reset_closure;
 	event->type          = FW_CDEV_EVENT_BUS_RESET;
+
 	event->generation    = client->device->generation;
+	smp_rmb();
 	event->node_id       = client->device->node_id;
-	event->local_node_id = card->local_node->node_id;
-	event->bm_node_id    = card->bm_node_id;
-	event->irm_node_id   = card->irm_node->node_id;
-	event->root_node_id  = card->root_node->node_id;
+
+	struct fw_card *card = client->device->card;
+
+	scoped_guard(spinlock_irq, &card->lock) {
+		event->local_node_id = card->local_node->node_id;
+		event->bm_node_id    = card->bm_node_id;
+		event->irm_node_id   = card->irm_node->node_id;
+		event->root_node_id  = card->root_node->node_id;
+	}
 }
 
 static void for_each_client(struct fw_device *device,
-- 
2.53.0

[PATCH 2/5] firewire: cdev: refactor using scoped_guard() for get_info ioctl call

[Takashi Sakamoto]

The scope of scoped_guard() corresponds to the code block, making it
easier to understand which code is protected by the locking primitive.
---
 drivers/firewire/core-cdev.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 9a3be1ceee2a..bdc16207d743 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -491,7 +491,6 @@ union ioctl_arg {
 static int ioctl_get_info(struct client *client, union ioctl_arg *arg)
 {
 	struct fw_cdev_get_info *a = &arg->get_info;
-	struct fw_cdev_event_bus_reset bus_reset;
 	unsigned long ret = 0;
 
 	client->version = a->version;
@@ -511,16 +510,21 @@ static int ioctl_get_info(struct client *client, union ioctl_arg *arg)
 		a->rom_length = client->device->config_rom_length * 4;
 	}
 
-	guard(mutex)(&client->device->client_list_mutex);
+	scoped_guard(mutex, &client->device->client_list_mutex) {
+		client->bus_reset_closure = a->bus_reset_closure;
 
-	client->bus_reset_closure = a->bus_reset_closure;
-	if (a->bus_reset != 0) {
-		fill_bus_reset_event(&bus_reset, client);
-		/* unaligned size of bus_reset is 36 bytes */
-		ret = copy_to_user(u64_to_uptr(a->bus_reset), &bus_reset, 36);
+		if (a->bus_reset != 0) {
+			struct fw_cdev_event_bus_reset bus_reset;
+
+			memset(&bus_reset, 0, sizeof(bus_reset));
+			fill_bus_reset_event(&bus_reset, client);
+
+			/* unaligned size of bus_reset is 36 bytes */
+			ret = copy_to_user(u64_to_uptr(a->bus_reset), &bus_reset, 36);
+		}
+		if (ret == 0 && list_empty(&client->link))
+			list_add_tail(&client->link, &client->device->client_list);
 	}
-	if (ret == 0 && list_empty(&client->link))
-		list_add_tail(&client->link, &client->device->client_list);
 
 	return ret ? -EFAULT : 0;
 }
-- 
2.53.0

[PATCH 3/5] firewire: cdev: refactor copy_to_user() in get_info ioctl call

[Takashi Sakamoto]

A non-zero return value from copy_to_user() is handled by returning
-EFAULT. Using scoped_guard() simplifies this handling, since it
eliminates the need for a local variable to store the return value and
check it after the locking primitive is released.

Refactor the code to remove the local variable.
---
 drivers/firewire/core-cdev.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index bdc16207d743..25590580b993 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -491,7 +491,6 @@ union ioctl_arg {
 static int ioctl_get_info(struct client *client, union ioctl_arg *arg)
 {
 	struct fw_cdev_get_info *a = &arg->get_info;
-	unsigned long ret = 0;
 
 	client->version = a->version;
 	a->version = FW_CDEV_KERNEL_VERSION;
@@ -499,12 +498,10 @@ static int ioctl_get_info(struct client *client, union ioctl_arg *arg)
 
 	scoped_guard(rwsem_read, &fw_device_rwsem) {
 		if (a->rom != 0) {
-			size_t want = a->rom_length;
-			size_t have = client->device->config_rom_length * 4;
+			size_t length = min_t(size_t, a->rom_length,
+					      client->device->config_rom_length * 4);
 
-			ret = copy_to_user(u64_to_uptr(a->rom), client->device->config_rom,
-					   min(want, have));
-			if (ret != 0)
+			if (copy_to_user(u64_to_uptr(a->rom), client->device->config_rom, length))
 				return -EFAULT;
 		}
 		a->rom_length = client->device->config_rom_length * 4;
@@ -520,13 +517,15 @@ static int ioctl_get_info(struct client *client, union ioctl_arg *arg)
 			fill_bus_reset_event(&bus_reset, client);
 
 			/* unaligned size of bus_reset is 36 bytes */
-			ret = copy_to_user(u64_to_uptr(a->bus_reset), &bus_reset, 36);
+			if (copy_to_user(u64_to_uptr(a->bus_reset), &bus_reset, 36))
+				return -EFAULT;
 		}
-		if (ret == 0 && list_empty(&client->link))
+
+		if (list_empty(&client->link))
 			list_add_tail(&client->link, &client->device->client_list);
 	}
 
-	return ret ? -EFAULT : 0;
+	return 0;
 }
 
 static int add_client_resource(struct client *client, struct client_resource *resource,
-- 
2.53.0

[PATCH 4/5] firewire: cdev: add annotations and comments about race condition of bus_reset_closure

[Takashi Sakamoto]

There is a race condition involving the bus_reset_closure member of the
fw_cdev_event_bus_reset structure, which was addressed by commit
93b37905f70 ("firewire: cdev: prevent race between first get_info ioctl
and bus reset event queuing").

Add code comments and annotations to document it.
---
 drivers/firewire/core-cdev.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 25590580b993..50c675113d5c 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -398,7 +398,12 @@ static ssize_t fw_device_op_read(struct file *file, char __user *buffer,
 
 static void fill_bus_reset_event(struct fw_cdev_event_bus_reset *event,
 				 struct client *client)
+__must_hold(&client->device->client_list_mutex)
 {
+	lockdep_assert_held(&client->device->client_list_mutex);
+
+	// This member is related to the above mutex. In detail, see 93b37905f70 ("firewire: cdev:
+	// prevent race between first get_info ioctl and bus reset event queuing").
 	event->closure	     = client->bus_reset_closure;
 	event->type          = FW_CDEV_EVENT_BUS_RESET;
 
@@ -508,6 +513,9 @@ static int ioctl_get_info(struct client *client, union ioctl_arg *arg)
 	}
 
 	scoped_guard(mutex, &client->device->client_list_mutex) {
+		// Coordinate concurrent access to this member with bus reset event handling, see
+		// 93b37905f70 ("firewire: cdev: prevent race between first get_info ioctl and bus
+		// reset event queuing").
 		client->bus_reset_closure = a->bus_reset_closure;
 
 		if (a->bus_reset != 0) {
-- 
2.53.0

[PATCH 5/5] firewire: cdev: add KUnit test for bus reset event UAPI structure

[Takashi Sakamoto]

The bus reset event structure exposed through the UAPI has a different
size depending on the target architecture. Extra care is required when
copying it to/from userspace.

Add a KUnit test for the structure to ensure that it has the expected
size and offsets.
---
 drivers/firewire/core-cdev.c |  5 ++++-
 drivers/firewire/uapi-test.c | 20 ++++++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 50c675113d5c..6d91a6e7ad0d 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -524,7 +524,10 @@ static int ioctl_get_info(struct client *client, union ioctl_arg *arg)
 			memset(&bus_reset, 0, sizeof(bus_reset));
 			fill_bus_reset_event(&bus_reset, client);
 
-			/* unaligned size of bus_reset is 36 bytes */
+			// This structure has 4 bytes of trailing padding under the System V ABI
+			// on most architectures (due to 8-byte alignment of the long long type),
+			// except for Intel386 (where long long type is aligned to 4 bytes). In
+			// either case, the effective length is 36 bytes.
 			if (copy_to_user(u64_to_uptr(a->bus_reset), &bus_reset, 36))
 				return -EFAULT;
 		}
diff --git a/drivers/firewire/uapi-test.c b/drivers/firewire/uapi-test.c
index bc3f10a2e516..0bdacb1caeb8 100644
--- a/drivers/firewire/uapi-test.c
+++ b/drivers/firewire/uapi-test.c
@@ -72,11 +72,31 @@ static void structure_layout_event_phy_packet2(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, 24, offsetof(struct fw_cdev_event_phy_packet2, data));
 }
 
+static void structure_layout_event_bus_reset(struct kunit *test)
+{
+#if defined(CONFIG_X86_32)
+       // 4 bytes alignment for aggregate type including 8 bytes storage types.
+       KUNIT_EXPECT_EQ(test, 36, sizeof(struct fw_cdev_event_bus_reset));
+#else
+       KUNIT_EXPECT_EQ(test, 40, sizeof(struct fw_cdev_event_bus_reset));
+#endif
+
+       KUNIT_EXPECT_EQ(test, 0, offsetof(struct fw_cdev_event_bus_reset, closure));
+       KUNIT_EXPECT_EQ(test, 8, offsetof(struct fw_cdev_event_bus_reset, type));
+       KUNIT_EXPECT_EQ(test, 12, offsetof(struct fw_cdev_event_bus_reset, node_id));
+       KUNIT_EXPECT_EQ(test, 16, offsetof(struct fw_cdev_event_bus_reset, local_node_id));
+       KUNIT_EXPECT_EQ(test, 20, offsetof(struct fw_cdev_event_bus_reset, bm_node_id));
+       KUNIT_EXPECT_EQ(test, 24, offsetof(struct fw_cdev_event_bus_reset, irm_node_id));
+       KUNIT_EXPECT_EQ(test, 28, offsetof(struct fw_cdev_event_bus_reset, root_node_id));
+       KUNIT_EXPECT_EQ(test, 32, offsetof(struct fw_cdev_event_bus_reset, generation));
+}
+
 static struct kunit_case structure_layout_test_cases[] = {
 	KUNIT_CASE(structure_layout_event_response),
 	KUNIT_CASE(structure_layout_event_request3),
 	KUNIT_CASE(structure_layout_event_response2),
 	KUNIT_CASE(structure_layout_event_phy_packet2),
+	KUNIT_CASE(structure_layout_event_bus_reset),
 	{}
 };
 
-- 
2.53.0

Re: [PATCH 0/5] firewire: cdev: refactor for bus reset event filling

[Takashi Sakamoto]

On Sun, Sep 20, 2026 at 04:14:56PM +0900, Takashi Sakamoto wrote:
> Hi,
> 
> In the cdev layer, there are two places where bus reset events are filed;
> the update path for the associated fw_device instance and the get_info
> ioctl call. There is still some code that can be refactored in these
> paths.
> 
> This series refactors the code to make the memory ordering and locking
> scope more explicit, and adds a KUnit test for the bus reset event UAPI
> structure.
> 
> Takashi Sakamoto (5):
>   firewire: cdev: use memory barrier to access fw_device members
>   firewire: cdev: refactor using scoped_guard() for get_info ioctl call
>   firewire: cdev: refactor copy_to_user() in get_info ioctl call
>   firewire: cdev: add annotations and comments about race condition of
>     bus_reset_closure
>   firewire: cdev: add KUnit test for bus reset event UAPI structure
> 
>  drivers/firewire/core-cdev.c | 63 +++++++++++++++++++++++-------------
>  drivers/firewire/uapi-test.c | 20 ++++++++++++
>  2 files changed, 60 insertions(+), 23 deletions(-)

Applied to for-next branch.


Regards

Takashi Sakamoto