linux-kernel - Re: [PATCH] Hibernate: save e820 table to snapshot header for comparison

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, 12 Aug 2014 11:41:36 +0200
From:	Pavel Machek <pavel@....cz>
To:	"Lee, Chun-Yi" <joeyli.kernel@...il.com>
Cc:	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Len Brown <len.brown@...el.com>, Takashi Iwai <tiwai@...e.de>,
	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
	"Lee, Chun-Yi" <jlee@...e.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH] Hibernate: save e820 table to snapshot header for
 comparison

Hi!

> [    7.374714] e820: Check memory region: [mem 0x0000000100000000-0x000000187fffffff] usable
> [    7.378041] PM: Image mismatch: memory map changed
> [    7.381314] PM: Read 2398272 kbytes in 0.27 seconds (8882.48 MB/s)
> [    7.385476] PM: Error -1 resuming
> [    7.388730] PM: Failed to load hibernation image, recovering.
> [    7.688989] PM: Basic memory bitmaps freed

Nice!

> +int save_mem_chk_map(struct mementry *mem_chk_map)

I'd prefer _chk_ -> _check_

> +{
> +	int i;
> +
> +	for (i = 0; i < e820.nr_map; i++) {
> +		struct e820entry *ei = &e820.map[i];
> +
> +		if (i > MEMCHKMAX)
> +			break;

MEMCHKMAX -> MEM_CHECK_MAX?

What happens when there are more entries?

> +bool check_mem_map(int mem_chk_entries, struct mementry *mem_chk_map)
> +{
> +	int i;
> +	bool ret = true;
> +
> +	if (mem_chk_entries != e820.nr_map) {
> +		pr_err("PM: memory check entry number %d:%d\n",
> +			mem_chk_entries, e820.nr_map);
> +		ret = false;
> +		goto Print_map;
> +	}

I'd change name to something like mem_map_matches() or mem_map_ok(),
so that it is clear what true/false means.

Can you reduce ammount of gotos?

> +	for (i = 0; i < mem_chk_entries; i++) {
> +		struct e820entry *ei = &e820.map[i];
> +
> +		if (i > MEMCHKMAX)
> +			break;
> +
> +		/* check regions not E820_RAM or E820_RESERVED_KERN */
> +		if (ei->type != E820_RAM && ei->type != E820_RESERVED_KERN) {
> +			if (mem_chk_map[i].addr != ei->addr ||
> +			    mem_chk_map[i].size != ei->size ||
> +			    mem_chk_map[i].type != ei->type) {
> +				ret = false;
> +				goto Print_map;
> +			}
> +		}

Why don't you check RAM and RESERVED_KERN, too? If those changed, we
don't want to resume, either, right?

(Plus, you only check ei->type; you should check mem_chk_map[].type,
too AFAICT).

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
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/