linux-kernel - Re: [PATCH v3 1/3] msi: free msi_desc entry only after we've released the kobject

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 25 Nov 2013 16:12:48 -0700
From:	Bjorn Helgaas <bhelgaas@...gle.com>
To:	Veaceslav Falico <vfalico@...hat.com>
Cc:	linux-pci@...r.kernel.org, torvalds@...ux-foundation.org,
	tglx@...utronix.de, yinghai@...nel.org, Knut_Petersen@...nline.de,
	mingo@...nel.org, paulmck@...ux.vnet.ibm.com, fweisbec@...il.com,
	linux-kernel@...r.kernel.org, Neil Horman <nhorman@...driver.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v3 1/3] msi: free msi_desc entry only after we've
 released the kobject

On Tue, Oct 29, 2013 at 11:30:30AM +0100, Veaceslav Falico wrote:
> Currently, we first do kobject_put(&entry->kobj) and the kfree(entry),
> however kobject_put() doesn't guarantee us that it was the last reference
> and that the kobj isn't used currently by someone else, so after we
> kfree(entry) with the struct kobject - other users will begin using the
> freed memory, instead of the actual kobject.
> 
> Fix this by using the kobject->release callback, which is called last when
> the kobject is indeed not used and is cleaned up - it's msi_kobj_release(),
> which can do the kfree(entry) safely (kobject_put/cleanup doesn't use the
> kobj itself after ->release() was called, so we're safe).
> 
> In case we've failed to create the sysfs directories - just kfree()
> it - cause we don't have the kobjects attached.
> 
> Also, remove the same functionality from populate_msi_sysfs(), cause on
> failure we anyway call free_msi_irqs(), which will take care of all the
> kobjects properly.
> 
> And add the forgotten pci_dev_put(pdev) in case of failure to register the
> kobject in populate_msi_sysfs().
> 
> CC: Bjorn Helgaas <bhelgaas@...gle.com>
> CC: Neil Horman <nhorman@...driver.com>
> CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> CC: linux-pci@...r.kernel.org
> CC: linux-kernel@...r.kernel.org
> Signed-off-by: Veaceslav Falico <vfalico@...hat.com>

I'm still hoping that Greg (or somebody else; Greg already posted the bulk
of the work) will finish up the conversion to attributes, and that Neil
will confirm that works with no changes to irqbalance.  So I'm going to
drop these patches for now.  Poke me if we need to revive them.

Bjorn

> ---
>  drivers/pci/msi.c | 31 ++++++++++++-------------------
>  1 file changed, 12 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index d5f90d6..5d70f49 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -374,19 +374,22 @@ static void free_msi_irqs(struct pci_dev *dev)
>  				iounmap(entry->mask_base);
>  		}
>  
> +		list_del(&entry->list);
> +
>  		/*
>  		 * Its possible that we get into this path
>  		 * When populate_msi_sysfs fails, which means the entries
>  		 * were not registered with sysfs.  In that case don't
> -		 * unregister them.
> +		 * unregister them, and just free. Otherwise the
> +		 * kobject->release will take care of freeing the entry via
> +		 * msi_kobj_release().
>  		 */
>  		if (entry->kobj.parent) {
>  			kobject_del(&entry->kobj);
>  			kobject_put(&entry->kobj);
> +		} else {
> +			kfree(entry);
>  		}
> -
> -		list_del(&entry->list);
> -		kfree(entry);
>  	}
>  }
>  
> @@ -509,6 +512,7 @@ static void msi_kobj_release(struct kobject *kobj)
>  	struct msi_desc *entry = to_msi_desc(kobj);
>  
>  	pci_dev_put(entry->dev);
> +	kfree(entry);
>  }
>  
>  static struct kobj_type msi_irq_ktype = {
> @@ -522,7 +526,6 @@ static int populate_msi_sysfs(struct pci_dev *pdev)
>  	struct msi_desc *entry;
>  	struct kobject *kobj;
>  	int ret;
> -	int count = 0;
>  
>  	pdev->msi_kset = kset_create_and_add("msi_irqs", NULL, &pdev->dev.kobj);
>  	if (!pdev->msi_kset)
> @@ -534,23 +537,13 @@ static int populate_msi_sysfs(struct pci_dev *pdev)
>  		pci_dev_get(pdev);
>  		ret = kobject_init_and_add(kobj, &msi_irq_ktype, NULL,
>  				     "%u", entry->irq);
> -		if (ret)
> -			goto out_unroll;
> -
> -		count++;
> +		if (ret) {
> +			pci_dev_put(pdev);
> +			return ret;
> +		}
>  	}
>  
>  	return 0;
> -
> -out_unroll:
> -	list_for_each_entry(entry, &pdev->msi_list, list) {
> -		if (!count)
> -			break;
> -		kobject_del(&entry->kobj);
> -		kobject_put(&entry->kobj);
> -		count--;
> -	}
> -	return ret;
>  }
>  
>  /**
> -- 
> 1.8.4
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/