Re: [PATCH] firewire: core: fix possible memory leak in build_tree()

Hello,

On Tue, Jul 28, 2026 at 08:43:08AM +0900, Takashi Sakamoto wrote:
> Indeed, there is a case to leak the memory object for the node found
> newly. However, I have a slight concern that the simple call of kfree()
> would cause another issue, perhaps.
> 
> > diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c
> > index df2ac0dab106..317f8f57e521 100644
> > --- a/drivers/firewire/core-topology.c
> > +++ b/drivers/firewire/core-topology.c
> > @@ -227,6 +227,7 @@ static struct fw_node *build_tree(struct fw_card *card, const u32 *sid, int self
> >              (enumerator.quadlet_count > 0 && parent_count != 1)) {
> >              fw_err(card, "parent port inconsistency for node %d: "
> >                     "parent_count=%d\n", phy_id, parent_count);
> > +            kfree(node);
> >              return NULL;
> >          }
> 
> When arriving at the line, all of children nodes have the reference to
> the issued node.
> 
> ======== 8< --------
> 250                 for (i = 0; i < port_count; i++) {
> 251                         switch (get_port_type(sid, i)) {
> 252                         case SELFID_PORT_PARENT:
>                                     ...
> 265                                 break;
> 266 
> 267                         case SELFID_PORT_CHILD:
>                                     ...
> 273                                 child->ports[child->color] = node;
>                                     ...
> 276                                 break;
> 277                         }
> 278                 }
> ======== 8< --------
> 
> I guess it required to invalidate the above references, since the
> reference would be used to release the child node.
> 
> ======== 8< --------
> diff --git a/drivers/firewire/core-topology.c b/drivers/firewire/core-topology.c
> index 75a6b0df670a..8bb6aa1b5d54 100644
> --- a/drivers/firewire/core-topology.c
> +++ b/drivers/firewire/core-topology.c
> @@ -286,6 +286,11 @@ static struct fw_node *build_tree(struct fw_card *card,
>                     (next_sid < end && parent_count != 1)) {
>                         fw_err(card, "parent port inconsistency for node %d: "
>                                "parent_count=%d\n", phy_id, parent_count);
> +                       for (i = 0; i < child->port_count; ++i) {
> +                               if (child->ports[i] == node)
> +                                       child->ports[i] = NULL;
> +                       }
> +                       kfree(node);
>                         return NULL;
>                 }
> ======== 8< --------
> 
> I'd like you to recheck the above point, since the above perspective comes
> from my rough code review.

Thanks for your detailed review. I'll go through this again, and send an
updated v2 patch.

Regards,
Nihaal