linux-kernel - Re: [PATCH 2/2] PM / sleep: prohibit devices probing during suspend/hibernation

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:	Thu, 8 Oct 2015 13:24:37 -0400 (EDT)
From:	Alan Stern <stern@...land.harvard.edu>
To:	Grygorii Strashko <grygorii.strashko@...com>
cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Pavel Machek <pavel@....cz>, Len Brown <len.brown@...el.com>,
	<linux-kernel@...r.kernel.org>, <linux-pm@...r.kernel.org>,
	Thierry Reding <thierry.reding@...il.com>
Subject: Re: [PATCH 2/2] PM / sleep: prohibit devices probing during
 suspend/hibernation

On Thu, 8 Oct 2015, Grygorii Strashko wrote:

> It is unsafe [1] if probing of devices will happen during suspend or
> hibernation and system behavior will be unpredictable in this case.
> So, lets prohibit device's probing in dpm_prepare() and defer their

s/lets/let's/, and same for the comment in the code.

> probing instead. The normal behavior will be restored in
> dpm_complete().


> @@ -172,6 +179,26 @@ static void driver_deferred_probe_trigger(void)
>  }
>  
>  /**
> + * device_defer_all_probes() - Enable/disable probing of devices
> + * @enable:  Enable/disable probing of devices
> + *
> + * if @enable = true
> + *	It will disable probing of devices and defer their probes.
> + * otherwise
> + *	It will restore normal behavior and trigger re-probing of deferred
> + *	devices.
> + */
> +void device_defer_all_probes(bool enable)
> +{
> +	defer_all_probes = enable;
> +	if (enable)
> +		/* sync with probes to avoid any races. */
> +		wait_for_device_probe();
> +	else
> +		driver_deferred_probe_trigger();
> +}

Some people might prefer to see two separate functions, an enable
routine and a disable routine.  I don't much care.

> @@ -277,9 +304,15 @@ static DECLARE_WAIT_QUEUE_HEAD(probe_waitqueue);
>  
>  static int really_probe(struct device *dev, struct device_driver *drv)
>  {
> -	int ret = 0;
> +	int ret = -EPROBE_DEFER;
>  	int local_trigger_count = atomic_read(&deferred_trigger_count);
>  
> +	if (defer_all_probes) {
> +		dev_dbg(dev, "Driver %s force probe deferral\n", drv->name);
> +		driver_deferred_probe_add(dev);
> +		return ret;
> +	}

In theory there's a race here.  If one CPU sets defer_all_probes, the 
new value might not be perceived by another CPU until a little while 
later.  Is there an easy way to insure that this race won't cause any 
problems?

Or do we already know that when this mechanism gets used, the system is 
already running on a single CPU?  I forget when that happens.

> @@ -1624,6 +1627,16 @@ int dpm_prepare(pm_message_t state)
>  	trace_suspend_resume(TPS("dpm_prepare"), state.event, true);
>  	might_sleep();
>  
> +	/* Give a chance for the known devices to complete their probing. */
> +	wait_for_device_probe();
> +	/*
> +	 * It is unsafe if probing of devices will happen during suspend or
> +	 * hibernation and system behavior will be unpredictable in this case.
> +	 * So, lets prohibit device's probing here and defer their probes
> +	 * instead. The normal behavior will be restored in dpm_complete().
> +	 */
> +	device_defer_all_probes(true);

Don't you want to call these two functions in the opposite order?  
First prevent new probes from occurring, then wait for any probes that 
are already in progress?  The way you have it here, a new probe could 
start between these two lines.

Alan Stern

--
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/