Re: [PATCH] Fix reference count leak in pci_create_slot

From: Bjorn Helgaas <helgaas@kernel.org>
To: wu000273@umn.edu
Cc: kjlu@umn.edu, Bjorn Helgaas <bhelgaas@google.com>,
	Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>,
	Jesse Barnes <jbarnes@virtuousgeek.org>,
	Alex Chiang <achiang@hp.com>,
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Fix reference count leak in pci_create_slot
Date: Fri, 10 Jul 2020 16:02:35 -0500	[thread overview]
Message-ID: <20200710210235.GA79952@bjorn-Precision-5520> (raw)
In-Reply-To: <20200528021322.1984-1-wu000273@umn.edu>

On Wed, May 27, 2020 at 09:13:22PM -0500, wu000273@umn.edu wrote:
> From: Qiushi Wu <wu000273@umn.edu>
> 
> kobject_init_and_add() takes reference even when it fails.
> If this function returns an error, kobject_put() must be called to
> properly clean up the memory associated with the object. Thus,
> when call of kobject_init_and_add() fail, we should call kobject_put()
> instead of kfree(). Previous commit "b8eb718348b8" fixed a similar problem.

Looks like you are also checking other kobject_init_and_add() callers?
I see some similar messages on linux-kernel.  Thanks for doing that!

I looked at the first dozen or so callers and the following look
broken and I don't see patches for them (yet):

  cache_add_dev()
  sq_dev_add
  blk_integrity_add
  acpi_expose_nondev_subnodes
  rnbd_clt_add_dev_kobj

> Fixes: 5fe6cc60680d ("PCI: prevent duplicate slot names")

I'm not sure this is correct.  5fe6cc60680d didn't *add* a
kobject_init_and_add() call; it was there before.  And even before
5fe6cc60680d there was no kobject_put() in the error path.  Am I
missing something?

For now, I applied this to pci/hotplug for v5.9.  I'll update the
commit log to add the Fixes tag if necessary.

> Signed-off-by: Qiushi Wu <wu000273@umn.edu>
> ---
>  drivers/pci/slot.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
> index cc386ef2fa12..3861505741e6 100644
> --- a/drivers/pci/slot.c
> +++ b/drivers/pci/slot.c
> @@ -268,13 +268,16 @@ struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
>  	slot_name = make_slot_name(name);
>  	if (!slot_name) {
>  		err = -ENOMEM;
> +		kfree(slot);
>  		goto err;
>  	}
>  
>  	err = kobject_init_and_add(&slot->kobj, &pci_slot_ktype, NULL,
>  				   "%s", slot_name);
> -	if (err)
> +	if (err) {
> +		kobject_put(&slot->kobj);
>  		goto err;
> +	}
>  
>  	INIT_LIST_HEAD(&slot->list);
>  	list_add(&slot->list, &parent->slots);
> @@ -293,7 +296,6 @@ struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
>  	mutex_unlock(&pci_slot_mutex);
>  	return slot;
>  err:
> -	kfree(slot);
>  	slot = ERR_PTR(err);
>  	goto out;
>  }
> -- 
> 2.17.1
> 