linux-kernel - Re: [PATCH 2/2] pm_trace: Add sysfs attr for rechecking dev hash.

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:	Sun, 10 Oct 2010 00:13:13 +0100
From:	James Hogan <james@...anarts.com>
To:	"Rafael J. Wysocki" <rjw@...k.pl>
Cc:	linux-pm@...ts.linux-foundation.org, linux-kernel@...r.kernel.org,
	linux-doc@...r.kernel.org, Len Brown <len.brown@...el.com>,
	Pavel Machek <pavel@....cz>,
	Randy Dunlap <rdunlap@...otime.net>,
	"Greg Kroah-Hartman" <gregkh@...e.de>,
	Alan Stern <stern@...land.harvard.edu>,
	Jesse Barnes <jbarnes@...tuousgeek.org>,
	markgross <markgross@...gnar.org>
Subject: Re: [PATCH 2/2] pm_trace: Add sysfs attr for rechecking dev hash.

Thanks for taking a look...

On Saturday 09 October 2010 23:49:15 Rafael J. Wysocki wrote:
> On Sunday, October 10, 2010, James Hogan wrote:
> > If the device which fails to resume is part of a loadable kernel module
> > it won't be checked at startup against the magic number stored in the
> > RTC.
> > 
> > Add a read-only sysfs attribute /sys/power/pm_trace_dev_hash which
> > contains a list of newline separated devices (usually just the one)
> > which currently match the last magic number. This allows the device
> > which is failing to resume to be found after the modules are loaded
> > again.
> > 
> > Signed-off-by: James Hogan <james@...anarts.com>
> > ---
> > 
> >  Documentation/power/s2ram.txt |    7 +++++++
> >  drivers/base/power/trace.c    |   27 +++++++++++++++++++++++++++
> >  include/linux/resume-trace.h  |    2 ++
> >  kernel/power/main.c           |   18 ++++++++++++++++++
> >  4 files changed, 54 insertions(+), 0 deletions(-)
> > 
> > diff --git a/Documentation/power/s2ram.txt
> > b/Documentation/power/s2ram.txt index 514b94f..3a2801a 100644
> > --- a/Documentation/power/s2ram.txt
> > +++ b/Documentation/power/s2ram.txt
> > 
> > @@ -49,6 +49,13 @@ machine that doesn't boot) is:
> >     device (lspci and /sys/devices/pci* is your friend), and see if you
> >     can fix it, disable it, or trace into its resume function.
> > 
> > +   If no device matches the hash, it may be a device from a loadable
> > kernel +   module that is not loaded until after the hash is checked.
> > You can check +   the hash against the current devices again after more
> > modules are loaded +   using sysfs:
> > +
> > +	cat /sys/power/pm_trace_dev_hash
> > +
> 
> /sys/power/pm_trace_match perhaps?

The magic number stores 1 "user" number (given in a RESUME_TRACE call) and 2 
hashes (representing source file/line and device) in the RTC, but this sysfs 
attribute only returns the matches for the device part, so I think it's 
important to have dev or device in there in case we want attributes for 
file/line (which doesn't work with modules at the moment either, but the 
"user" number can be used as that's printed at boot directly), but I agree 
that match is better than hash so I'm happy to change it to pm_trace_dev_match 
or pm_trace_dev_matches.

> 
> How do we ensure it prints things that make sense when the last resume was
> successful or the system hasn't suspended at all?

If the last resume was successful, then the stored magic number won't have 
changed since the original boot, since it is read from the RTC in a 
core_initcall function (early_resume_init()).

The case of when pm_trace wasn't in use before boot is impossible to avoid 
when using the RTC. There is a chance that a genuine RTC value will not match 
any of the device hashes (there are 1009 possible device hash values), in 
which case nothing will be output, but the same thing happens at boot when it 
does it's first comparison against the devices and printk's any matches.

> 
> >  For example, the above happens to be the VGA device on my EVO, which I
> >  used to run with "radeonfb" (it's an ATI Radeon mobility). It turns out
> >  that "radeonfb" simply cannot resume that device - it tries to set the
> > 
> > diff --git a/drivers/base/power/trace.c b/drivers/base/power/trace.c
> > index 17e24e3..e0cdba1 100644
> > --- a/drivers/base/power/trace.c
> > +++ b/drivers/base/power/trace.c
> > @@ -207,6 +207,33 @@ static int show_dev_hash(unsigned int value)
> > 
> >  static unsigned int hash_value_early_read;
> > 
> > +int snprint_trace_dev_hash(char *buf, size_t size)
> > +{
> > +	unsigned int value = hash_value_early_read / (USERHASH * FILEHASH);
> > +	int ret = 0;
> > +	struct list_head *entry;
> > +
> > +	device_pm_lock();
> > +	entry = dpm_list.prev;
> > +	while (size && entry != &dpm_list) {
> > +		struct device *dev = to_device(entry);
> > +		unsigned int hash = hash_string(DEVSEED, dev_name(dev),
> > +						DEVHASH);
> > +		if (hash == value) {
> > +			int len = snprintf(buf, size, "%s\n",
> > +					    dev_driver_string(dev));
> > +			if (len > size)
> > +				len = size;
> > +			buf += len;
> > +			ret += len;
> > +			size -= len;
> 
> Don't we want to break; here and if so then why?

No, because it's possible that two devices will hash to the same value, in 
which case it is better to print both out so we know that the problem could be 
in either one of them. I'll add a comment to that effect.

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