linux-kernel - Re: [PATCH v6 4/5] PM / runtime: Use device links

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]

Message-ID: <20161218140122.GA13141@wunner.de>
Date:   Sun, 18 Dec 2016 15:01:22 +0100
From:   Lukas Wunner <lukas@...ner.de>
To:     "Rafael J. Wysocki" <rjw@...ysocki.net>
Cc:     Linux PM list <linux-pm@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Alan Stern <stern@...land.harvard.edu>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Tomeu Vizoso <tomeu.vizoso@...labora.com>,
        Mark Brown <broonie@...nel.org>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Kevin Hilman <khilman@...nel.org>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        "Luis R. Rodriguez" <mcgrof@...e.com>
Subject: Re: [PATCH v6 4/5] PM / runtime: Use device links

Hi Rafael,

spotted what looks like a bug in the device links runtime PM code:

When resuming a device, __rpm_callback() calls rpm_get_suppliers(dev):

> +			retval = rpm_get_suppliers(dev);
> +			if (retval)
> +				goto fail;

This will walk the list of suppliers and call pm_runtime_get_sync()
for each of them:

> +static int rpm_get_suppliers(struct device *dev)
> +{
> +	struct device_link *link;
> +
> +	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) {
> +		int retval;
[...]
> +		retval = pm_runtime_get_sync(link->supplier);
> +		if (retval < 0) {
> +			pm_runtime_put_noidle(link->supplier);
> +			return retval;

If pm_runtime_get_sync() failed, e.g. because runtime PM is disabled
on a supplier, the function will put the reference of the failed
supplier and return.

Back in __rpm_callback() we jump to the fail mark, where we call
rpm_put_suppliers().

> + fail:
> +			rpm_put_suppliers(dev);
> +
> +			device_links_read_unlock(idx);

This walks the list of suppliers and releases a ref for each of them:

> +static void rpm_put_suppliers(struct device *dev)
> +{
> +	struct device_link *link;
> +
> +	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node)
> +		if (link->rpm_active &&
> +		    READ_ONCE(link->status) != DL_STATE_SUPPLIER_UNBIND) {
> +			pm_runtime_put(link->supplier);
> +			link->rpm_active = false;
> +		}
> +}

This looks wrong:  We've already put a ref on the failed supplier, so here
we're putting another one.  And if there are further suppliers in the list
following the failed one, we'll decrement their refcount even though we've
never incremented it.

Thanks,

Lukas