linux-kernel - Re: [PATCH 07/31] lmb: Add reserve_lmb/free

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:	Mon, 29 Mar 2010 23:22:02 +1100
From:	Michael Ellerman <michael@...erman.id.au>
To:	Yinghai Lu <yinghai@...nel.org>
Cc:	Ingo Molnar <mingo@...e.hu>, Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	David Miller <davem@...emloft.net>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Johannes Weiner <hannes@...xchg.org>,
	linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org
Subject: Re: [PATCH 07/31] lmb: Add reserve_lmb/free_lmb

On Sun, 2010-03-28 at 19:43 -0700, Yinghai Lu wrote:
> They will check if the region array is big enough.
> 
> __check_and_double_region_array will try to double the region if that array spare
> slots if not big enough.
> find_lmb_area() is used to find good postion for new region array.
> Old array will be copied to new array.
> 
> Arch code should provide to get_max_mapped, so the new array have accessiable
> address
..
> diff --git a/mm/lmb.c b/mm/lmb.c
> index d5d5dc4..9798458 100644
> --- a/mm/lmb.c
> +++ b/mm/lmb.c
> @@ -551,6 +551,95 @@ int lmb_find(struct lmb_property *res)
>  	return -1;
>  }
>  
> +u64 __weak __init get_max_mapped(void)
> +{
> +	u64 end = max_low_pfn;
> +
> +	end <<= PAGE_SHIFT;
> +
> +	return end;
> +}

^ This is (sort of) what lmb.rmo_size represents. So maybe instead of
adding this function, we could just say that the arch code needs to set
rmo_size up with an appropriate value, and then use that below. Though
maybe that's conflating things.

...
> +
> +void __init add_lmb_memory(u64 start, u64 end)
> +{
> +	__check_and_double_region_array(&lmb.memory, &lmb_memory_region[0], start, end);
> +	lmb_add(start, end - start);
> +}
> +
> +void __init reserve_lmb(u64 start, u64 end, char *name)
> +{
> +	if (start == end)
> +		return;
> +
> +	if (WARN_ONCE(start > end, "reserve_lmb: wrong range [%#llx, %#llx]\n", start, end))
> +		return;
> +
> +	__check_and_double_region_array(&lmb.reserved, &lmb_reserved_region[0], start, end);
> +	lmb_reserve(start, end - start);
> +}
> +
> +void __init free_lmb(u64 start, u64 end)
> +{
> +	if (start == end)
> +		return;
> +
> +	if (WARN_ONCE(start > end, "free_lmb: wrong range [%#llx, %#llx]\n", start, end))
> +		return;
> +
> +	/* keep punching hole, could run out of slots too */
> +	__check_and_double_region_array(&lmb.reserved, &lmb_reserved_region[0], start, end);
> +	lmb_free(start, end - start);
> +}

Doesn't this mean that if I call lmb_alloc() or lmb_free() too many
times then I'll potentially run out of space? So doesn't that
essentially break the existing API?

It seems to me that rather than adding these "special" routines that
check for enough space on the way in, instead you should be checking in
lmb_add_region() - which is where AFAICS all allocs/frees/reserves
eventually end up if they need to insert a new region.

cheers



Download attachment "signature.asc" of type "application/pgp-signature" (198 bytes)