netdev - Re: [PATCH v3 2/6] PM / Runtime: introduce pm_runtime_set[get]_memalloc

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:	Tue, 30 Oct 2012 11:38:46 -0400 (EDT)
From:	Alan Stern <stern@...land.harvard.edu>
To:	Ming Lei <ming.lei@...onical.com>
cc:	linux-kernel@...r.kernel.org, Oliver Neukum <oneukum@...e.de>,
	Minchan Kim <minchan@...nel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rjw@...k.pl>, Jens Axboe <axboe@...nel.dk>,
	"David S. Miller" <davem@...emloft.net>,
	Andrew Morton <akpm@...ux-foundation.org>,
	<netdev@...r.kernel.org>, <linux-usb@...r.kernel.org>,
	<linux-pm@...r.kernel.org>, <linux-mm@...ck.org>
Subject: Re: [PATCH v3 2/6] PM / Runtime: introduce pm_runtime_set[get]_memalloc_noio()

On Tue, 30 Oct 2012, Ming Lei wrote:

> >> +bool pm_runtime_get_memalloc_noio(struct device *dev)
> >> +{
> >> +     bool ret;
> >> +     spin_lock_irq(&dev->power.lock);
> >> +     ret = dev->power.memalloc_noio_resume;
> >> +     spin_unlock_irq(&dev->power.lock);
> >> +     return ret;
> >> +}
> >
> > You don't need to acquire and release a spinlock just to read the
> > value.  Reading bitfields _is_ SMP-safe; writing them is not.
> 
> Thanks for your review.
> 
> As you pointed out before, the flag need to be checked before
> resetting usb devices, so the lock should be held to make another
> context(CPU) see the updated value suppose one context(CPU)
> call pm_runtime_set_memalloc_noio() to change the flag at the
> same time.

Okay, I see your point.  But acquiring the lock here doesn't solve the 
problem.  Suppose a thread is about to reset a USB mass-storage device.  
It acquires the lock and sees that the noio flag is clear.  But before 
it can issue the reset, another thread sets the noio flag.

I'm not sure what the best solution is.

> The lock needn't to be held when the function is called inside
> pm_runtime_set_memalloc_noio(),  so the bitfield flag should
> be checked directly without holding power lock in dev_memalloc_noio().

Yes.

A couple of other things...  Runtime resume can be blocked by runtime
suspend, if a resume is requested while the suspend is in progress.  
Therefore the runtime suspend code also needs to save-set-restore the
noio flag.

Also, we should set the noio flag at the start of 
usb_stor_control_thread, because everything that thread does can 
potentially block an I/O operation.

Lastly, pm_runtime_get_memalloc_noio always returns false when 
CONFIG_PM_RUNTIME is disabled.  But we still need to prevent I/O during 
usb_reset_device even when there's no runtime PM.  Maybe the simplest 
answer is always to set noio during resets.  That would also help with 
the race described above.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html