[PATCH 0/3] firewire: core: fix potential memory leak in build_tree()

[PATCH 0/3] firewire: core: fix potential memory leak in build_tree()

[Takashi Sakamoto]

Hi,

This patchset fixes a potential memory leak in the error path of
build_tree(), reported by Abdun Nihaal[1].

The first two patches refactor build tree() to optimize port counting and
the place of parent port validation. The last patch fixes the error path
to release allocated node instances properly.

The changes were verified using the kmem:kmalloc and kmem:kfree
tracepoints together with the KUnit tests added in my former patchset[2].
The kmem:kfree events appear in the issued cases:

```
kmalloc: call_site=build_tree+0x228/0x620 ptr=ffffa10b013f0b00 bytes_req=64 bytes_alloc=64 gfp_flags=GFP_ATOMIC|__GFP_ZERO node=-1 accounted=false
kmalloc: call_site=build_tree+0x228/0x620 ptr=ffffa10b013f0b40 bytes_req=48 bytes_alloc=64 gfp_flags=GFP_ATOMIC|__GFP_ZERO node=-1 accounted=false
kmalloc: call_site=build_tree+0x228/0x620 ptr=ffffa10b013f0b80 bytes_req=64 bytes_alloc=64 gfp_flags=GFP_ATOMIC|__GFP_ZERO node=-1 accounted=false
firewire_core node_tree_test_invalid_parent_port_count.dummy-device: parent port inconsistency for node 3: parent_count=2
kfree: call_site=for_each_fw_node+0x17c/0x230 ptr=ffffa10b013f0b80
kfree: call_site=for_each_fw_node+0x17c/0x230 ptr=ffffa10b013f0b00
kfree: call_site=for_each_fw_node+0x17c/0x230 ptr=ffffa10b013f0b40
```

[1][PATCH v2] firewire: core: fix possible memory leak in build_tree()
https://lore.kernel.org/lkml/20260801110915.82561-1-nihaal@cse.iitm.ac.in/
[2][PATCH 0/3] firewire: core: add KUnit tests for tree building
https://lore.kernel.org/lkml/20260810064119.410324-1-o-takashi@sakamocchi.jp/


Takashi Sakamoto (3):
  firewire: core: consolidate port counting in build_tree()
  firewire: core: validate parent port count before allocating nodes in
    build_tree()
  firewire: core: fix memory leak in error path of build_tree()

 drivers/firewire/core-topology.c | 58 ++++++++++++++++++++------------
 1 file changed, 36 insertions(+), 22 deletions(-)


base-commit: f744022705b7eb479a1931ddd95aae4e9a4be221
-- 
2.53.0

[PATCH 1/3] firewire: core: consolidate port counting in build_tree()

[Takashi Sakamoto]

The self ID sequence describes the state of each port for each PHY.
Currently, build_tree() counts the ports in two separate places.

Consolidate the port counting in one place.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-topology.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c
index 1d3a4419f554..4f610205576c 100644
--- a/drivers/firewire/core-topology.c
+++ b/drivers/firewire/core-topology.c
@@ -119,8 +119,8 @@ static struct fw_node *build_tree(struct fw_card *card, const u32 *sid, int self
 
 	while (enumerator.quadlet_count > 0) {
 		unsigned int child_port_count = 0;
+		unsigned int parent_port_count = 0;
 		unsigned int total_port_count = 0;
-		unsigned int parent_count = 0;
 		unsigned int quadlet_count;
 		const u32 *self_id_sequence;
 		unsigned int port_capacity;
@@ -148,16 +148,19 @@ static struct fw_node *build_tree(struct fw_card *card, const u32 *sid, int self
 			switch (port_status) {
 			case PHY_PACKET_SELF_ID_PORT_STATUS_CHILD:
 				++child_port_count;
-				fallthrough;
+				break;
 			case PHY_PACKET_SELF_ID_PORT_STATUS_PARENT:
+				++parent_port_count;
+				break;
 			case PHY_PACKET_SELF_ID_PORT_STATUS_NCONN:
 				++total_port_count;
-				fallthrough;
+				break;
 			case PHY_PACKET_SELF_ID_PORT_STATUS_NONE:
 			default:
 				break;
 			}
 		}
+		total_port_count += child_port_count + parent_port_count;
 
 		if (phy_id != phy_packet_self_id_get_phy_id(self_id_sequence[0])) {
 			fw_err(card, "PHY ID mismatch in self ID: %d != %d\n",
@@ -203,7 +206,6 @@ static struct fw_node *build_tree(struct fw_card *card, const u32 *sid, int self
 				// we temporarily abuse node->color for remembering the entry in
 				// the node->ports array where the parent node should be.  Later,
 				// when we handle the parent node, we fix up the reference.
-				++parent_count;
 				node->color = port_index;
 				break;
 
@@ -223,10 +225,10 @@ static struct fw_node *build_tree(struct fw_card *card, const u32 *sid, int self
 
 		// Check that the node reports exactly one parent port, except for the root, which
 		// of course should have no parents.
-		if ((enumerator.quadlet_count == 0 && parent_count != 0) ||
-		    (enumerator.quadlet_count > 0 && parent_count != 1)) {
+		if ((enumerator.quadlet_count == 0 && parent_port_count != 0) ||
+		    (enumerator.quadlet_count > 0 && parent_port_count != 1)) {
 			fw_err(card, "parent port inconsistency for node %d: "
-			       "parent_count=%d\n", phy_id, parent_count);
+			       "parent_count=%d\n", phy_id, parent_port_count);
 			return NULL;
 		}
 
@@ -235,7 +237,7 @@ static struct fw_node *build_tree(struct fw_card *card, const u32 *sid, int self
 		list_add_tail(&node->link, &stack);
 		stack_depth += 1 - child_port_count;
 
-		if (node->phy_speed == SCODE_BETA && parent_count + child_port_count > 1)
+		if (node->phy_speed == SCODE_BETA && parent_port_count + child_port_count > 1)
 			beta_repeaters_present = true;
 
 		// If PHYs report different gap counts, set an invalid count which will force a gap
-- 
2.53.0

[PATCH 2/3] firewire: core: validate parent port count before allocating nodes in build_tree()

[Takashi Sakamoto]

The node tree requires each child node to have exactly one port connected
to a parent node, while the root node must have no such port. This can be
validated by comparing the parent port count for a PHY with the rest of
the self ID sequence.

Currently, this validation is done after the node has been allocated. Move
it before the allocation so that an invalid self ID sequence can cause
an error without having to clean up the newly allocated node.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-topology.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c
index 4f610205576c..e032497b2594 100644
--- a/drivers/firewire/core-topology.c
+++ b/drivers/firewire/core-topology.c
@@ -162,6 +162,15 @@ static struct fw_node *build_tree(struct fw_card *card, const u32 *sid, int self
 		}
 		total_port_count += child_port_count + parent_port_count;
 
+		// Check that the node reports exactly one parent port, except for the root, which
+		// of course should have no parents.
+		if ((enumerator.quadlet_count == 0 && parent_port_count != 0) ||
+		    (enumerator.quadlet_count > 0 && parent_port_count != 1)) {
+			fw_err(card, "parent port inconsistency for node %d: parent_count=%d\n",
+			       phy_id, parent_port_count);
+			return NULL;
+		}
+
 		if (phy_id != phy_packet_self_id_get_phy_id(self_id_sequence[0])) {
 			fw_err(card, "PHY ID mismatch in self ID: %d != %d\n",
 			       phy_id, phy_packet_self_id_get_phy_id(self_id_sequence[0]));
@@ -223,15 +232,6 @@ static struct fw_node *build_tree(struct fw_card *card, const u32 *sid, int self
 			}
 		}
 
-		// Check that the node reports exactly one parent port, except for the root, which
-		// of course should have no parents.
-		if ((enumerator.quadlet_count == 0 && parent_port_count != 0) ||
-		    (enumerator.quadlet_count > 0 && parent_port_count != 1)) {
-			fw_err(card, "parent port inconsistency for node %d: "
-			       "parent_count=%d\n", phy_id, parent_port_count);
-			return NULL;
-		}
-
 		/* Pop the child nodes off the stack and push the new node. */
 		__list_del(h->prev, &stack);
 		list_add_tail(&node->link, &stack);
-- 
2.53.0

[PATCH 3/3] firewire: core: fix memory leak in error path of build_tree()

[Takashi Sakamoto]

In the error path of build_tree(), node instances can remain in the local
linked list when the function returns.

Whenever an invalid value is detected in the self ID sequence, each
allocated node instance is either an entry in the linked list or an
entry in the ports array of its parent node. Therefore, the allocate
node instances can be safely released by traversing the linked list from
its head.

Release the remaining node instances with for_each_fw_node() before
returning to the caller.

Fixes: 3038e353cfaf ("firewire: Add core firewire stack.")
Reported-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
Link: https://lore.kernel.org/all/20260727095955.104972-1-nihaal@cse.iitm.ac.in/
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 drivers/firewire/core-topology.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c
index e032497b2594..ee6b54f89859 100644
--- a/drivers/firewire/core-topology.c
+++ b/drivers/firewire/core-topology.c
@@ -88,6 +88,17 @@ static inline struct fw_node *fw_node(struct list_head *l)
 	return list_entry(l, struct fw_node, link);
 }
 
+typedef void (*fw_node_callback_t)(struct fw_card *card, struct fw_node *node,
+				   struct fw_node *parent);
+
+static void for_each_fw_node(struct fw_card *card, struct fw_node *root,
+			     fw_node_callback_t callback);
+
+static void free_fw_node(struct fw_card *card, struct fw_node *node, struct fw_node *parent)
+{
+	kfree(node);
+}
+
 /*
  * This function builds the tree representation of the topology given
  * by the self IDs from the latest bus reset.  During the construction
@@ -134,7 +145,7 @@ static struct fw_node *build_tree(struct fw_card *card, const u32 *sid, int self
 			if (PTR_ERR(self_id_sequence) != -ENODATA) {
 				fw_err(card, "inconsistent extended self IDs: %ld\n",
 				       PTR_ERR(self_id_sequence));
-				return NULL;
+				goto error;
 			}
 			break;
 		}
@@ -168,18 +179,18 @@ static struct fw_node *build_tree(struct fw_card *card, const u32 *sid, int self
 		    (enumerator.quadlet_count > 0 && parent_port_count != 1)) {
 			fw_err(card, "parent port inconsistency for node %d: parent_count=%d\n",
 			       phy_id, parent_port_count);
-			return NULL;
+			goto error;
 		}
 
 		if (phy_id != phy_packet_self_id_get_phy_id(self_id_sequence[0])) {
 			fw_err(card, "PHY ID mismatch in self ID: %d != %d\n",
 			       phy_id, phy_packet_self_id_get_phy_id(self_id_sequence[0]));
-			return NULL;
+			goto error;
 		}
 
 		if (child_port_count > stack_depth) {
 			fw_err(card, "topology stack underflow\n");
-			return NULL;
+			goto error;
 		}
 
 		/*
@@ -197,7 +208,7 @@ static struct fw_node *build_tree(struct fw_card *card, const u32 *sid, int self
 		node = fw_node_create(self_id_sequence[0], total_port_count, card->color);
 		if (node == NULL) {
 			fw_err(card, "out of memory while building topology\n");
-			return NULL;
+			goto error;
 		}
 
 		if (phy_id == (card->node_id & 0x3f))
@@ -256,12 +267,13 @@ static struct fw_node *build_tree(struct fw_card *card, const u32 *sid, int self
 	card->beta_repeaters_present = beta_repeaters_present;
 
 	return local_node;
+error:
+	++card->color;
+	list_for_each_entry_safe(node, child, &stack, link)
+		for_each_fw_node(card, node, free_fw_node);
+	return NULL;
 }
 
-typedef void (*fw_node_callback_t)(struct fw_card * card,
-				   struct fw_node * node,
-				   struct fw_node * parent);
-
 static void for_each_fw_node(struct fw_card *card, struct fw_node *root,
 			     fw_node_callback_t callback)
 {
-- 
2.53.0

Re: [PATCH 3/3] firewire: core: fix memory leak in error path of build_tree()

[Abdun Nihaal]

On Tue, Aug 11, 2026 at 09:09:28PM +0900, Takashi Sakamoto wrote:
> In the error path of build_tree(), node instances can remain in the local
> linked list when the function returns.
> 
> Whenever an invalid value is detected in the self ID sequence, each
> allocated node instance is either an entry in the linked list or an
> entry in the ports array of its parent node. Therefore, the allocate
> node instances can be safely released by traversing the linked list from
> its head.
> 
> Release the remaining node instances with for_each_fw_node() before
> returning to the caller.
> 
> Fixes: 3038e353cfaf ("firewire: Add core firewire stack.")
> Reported-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
> Link: https://lore.kernel.org/all/20260727095955.104972-1-nihaal@cse.iitm.ac.in/
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

Looks good to me. Thanks for fixing this.

I don't have a proper understanding of how the Firewire topology
construction happens, and was a bit confused by how the node->color
is used in the code, that's why I couldn't follow up with the fix.

Regards,
Nihaal

Re: [PATCH 3/3] firewire: core: fix memory leak in error path of build_tree()

[Takashi Sakamoto]

Hi,

On Wed, Aug 12, 2026 at 02:35:36PM +0530, Abdun Nihaal wrote:
> On Tue, Aug 11, 2026 at 09:09:28PM +0900, Takashi Sakamoto wrote:
> > In the error path of build_tree(), node instances can remain in the local
> > linked list when the function returns.
> > 
> > Whenever an invalid value is detected in the self ID sequence, each
> > allocated node instance is either an entry in the linked list or an
> > entry in the ports array of its parent node. Therefore, the allocate
> > node instances can be safely released by traversing the linked list from
> > its head.
> > 
> > Release the remaining node instances with for_each_fw_node() before
> > returning to the caller.
> > 
> > Fixes: 3038e353cfaf ("firewire: Add core firewire stack.")
> > Reported-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
> > Link: https://lore.kernel.org/all/20260727095955.104972-1-nihaal@cse.iitm.ac.in/
> > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> 
> Looks good to me. Thanks for fixing this.
> 
> I don't have a proper understanding of how the Firewire topology
> construction happens, and was a bit confused by how the node->color
> is used in the code, that's why I couldn't follow up with the fix.

Yea. How the color member of fw_node/fw_card works was unclear to me too.
Writing some KUnit tests finally brought it to me.

Anyway, thank for your pointing and working for the issue.


Regards

Takashi Sakamoto

Re: [PATCH 0/3] firewire: core: fix potential memory leak in build_tree()

[Takashi Sakamoto]

On Tue, Aug 11, 2026 at 09:09:25PM +0900, Takashi Sakamoto wrote:
> Hi,
> 
> This patchset fixes a potential memory leak in the error path of
> build_tree(), reported by Abdun Nihaal[1].
> 
> The first two patches refactor build tree() to optimize port counting and
> the place of parent port validation. The last patch fixes the error path
> to release allocated node instances properly.
> 
> The changes were verified using the kmem:kmalloc and kmem:kfree
> tracepoints together with the KUnit tests added in my former patchset[2].
> The kmem:kfree events appear in the issued cases:
> 
> ```
> kmalloc: call_site=build_tree+0x228/0x620 ptr=ffffa10b013f0b00 bytes_req=64 bytes_alloc=64 gfp_flags=GFP_ATOMIC|__GFP_ZERO node=-1 accounted=false
> kmalloc: call_site=build_tree+0x228/0x620 ptr=ffffa10b013f0b40 bytes_req=48 bytes_alloc=64 gfp_flags=GFP_ATOMIC|__GFP_ZERO node=-1 accounted=false
> kmalloc: call_site=build_tree+0x228/0x620 ptr=ffffa10b013f0b80 bytes_req=64 bytes_alloc=64 gfp_flags=GFP_ATOMIC|__GFP_ZERO node=-1 accounted=false
> firewire_core node_tree_test_invalid_parent_port_count.dummy-device: parent port inconsistency for node 3: parent_count=2
> kfree: call_site=for_each_fw_node+0x17c/0x230 ptr=ffffa10b013f0b80
> kfree: call_site=for_each_fw_node+0x17c/0x230 ptr=ffffa10b013f0b00
> kfree: call_site=for_each_fw_node+0x17c/0x230 ptr=ffffa10b013f0b40
> ```
> 
> [1][PATCH v2] firewire: core: fix possible memory leak in build_tree()
> https://lore.kernel.org/lkml/20260801110915.82561-1-nihaal@cse.iitm.ac.in/
> [2][PATCH 0/3] firewire: core: add KUnit tests for tree building
> https://lore.kernel.org/lkml/20260810064119.410324-1-o-takashi@sakamocchi.jp/
> 
> 
> Takashi Sakamoto (3):
>   firewire: core: consolidate port counting in build_tree()
>   firewire: core: validate parent port count before allocating nodes in
>     build_tree()
>   firewire: core: fix memory leak in error path of build_tree()
> 
>  drivers/firewire/core-topology.c | 58 ++++++++++++++++++++------------
>  1 file changed, 36 insertions(+), 22 deletions(-)

Applied to for-next branch, with some removals of code comments in
node-tree-test.c addressing to the memory leak.


Regards

Takashi Sakamoto