Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1304647 > unrolled thread

Re: [PATCH v2 11/13] arm64: allow kernel Image to be loaded anywhere in physical memory

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 11/13] arm64: allow kernel Image to be loaded anywhere  in physical memory Catalin Marinas <catalin.marinas@arm.com> - 2016-01-08 16:30 +0100
    Re: [PATCH v2 11/13] arm64: allow kernel Image to be loaded anywhere  in physical memory Mark Rutland <mark.rutland@arm.com> - 2016-01-08 16:40 +0100
      Re: [PATCH v2 11/13] arm64: allow kernel Image to be loaded anywhere  in physical memory Catalin Marinas <catalin.marinas@arm.com> - 2016-01-08 16:50 +0100
        Re: [PATCH v2 11/13] arm64: allow kernel Image to be loaded anywhere  in physical memory Mark Rutland <mark.rutland@arm.com> - 2016-01-08 17:20 +0100
    Re: [PATCH v2 11/13] arm64: allow kernel Image to be loaded anywhere  in physical memory Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-01-08 16:40 +0100

#1304647 — Re: [PATCH v2 11/13] arm64: allow kernel Image to be loaded anywhere in physical memory

On Wed, Dec 30, 2015 at 04:26:10PM +0100, Ard Biesheuvel wrote:
> +static void __init enforce_memory_limit(void)
> +{
> +	const phys_addr_t kbase = round_down(__pa(_text), MIN_KIMG_ALIGN);
> +	u64 to_remove = memblock_phys_mem_size() - memory_limit;
> +	phys_addr_t max_addr = 0;
> +	struct memblock_region *r;
> +
> +	if (memory_limit == (phys_addr_t)ULLONG_MAX)
> +		return;
> +
> +	/*
> +	 * The kernel may be high up in physical memory, so try to apply the
> +	 * limit below the kernel first, and only let the generic handling
> +	 * take over if it turns out we haven't clipped enough memory yet.
> +	 */
> +	for_each_memblock(memory, r) {
> +		if (r->base + r->size > kbase) {
> +			u64 rem = min(to_remove, kbase - r->base);
> +
> +			max_addr = r->base + rem;
> +			to_remove -= rem;
> +			break;
> +		}
> +		if (to_remove <= r->size) {
> +			max_addr = r->base + to_remove;
> +			to_remove = 0;
> +			break;
> +		}
> +		to_remove -= r->size;
> +	}
> +
> +	memblock_remove(0, max_addr);
> +
> +	if (to_remove)
> +		memblock_enforce_memory_limit(memory_limit);
> +}

IIUC, this is changing the user expectations a bit. There are people
using the mem= limit to hijack some top of the RAM for other needs
(though they could do it in a saner way like changing the DT memory
nodes). Your patch first tries to remove the memory below the kernel
image and only remove the top if additional limitation is necessary.

Can you not remove memory from the top and block the limit if it goes
below the end of the kernel image, with some warning that memory limit
was not entirely fulfilled?

-- 
Catalin

[toc] | [next] | [standalone]

#1304654

On Fri, Jan 08, 2016 at 03:27:38PM +0000, Catalin Marinas wrote:
> On Wed, Dec 30, 2015 at 04:26:10PM +0100, Ard Biesheuvel wrote:
> > +static void __init enforce_memory_limit(void)
> > +{
> > +	const phys_addr_t kbase = round_down(__pa(_text), MIN_KIMG_ALIGN);
> > +	u64 to_remove = memblock_phys_mem_size() - memory_limit;
> > +	phys_addr_t max_addr = 0;
> > +	struct memblock_region *r;
> > +
> > +	if (memory_limit == (phys_addr_t)ULLONG_MAX)
> > +		return;
> > +
> > +	/*
> > +	 * The kernel may be high up in physical memory, so try to apply the
> > +	 * limit below the kernel first, and only let the generic handling
> > +	 * take over if it turns out we haven't clipped enough memory yet.
> > +	 */
> > +	for_each_memblock(memory, r) {
> > +		if (r->base + r->size > kbase) {
> > +			u64 rem = min(to_remove, kbase - r->base);
> > +
> > +			max_addr = r->base + rem;
> > +			to_remove -= rem;
> > +			break;
> > +		}
> > +		if (to_remove <= r->size) {
> > +			max_addr = r->base + to_remove;
> > +			to_remove = 0;
> > +			break;
> > +		}
> > +		to_remove -= r->size;
> > +	}
> > +
> > +	memblock_remove(0, max_addr);
> > +
> > +	if (to_remove)
> > +		memblock_enforce_memory_limit(memory_limit);
> > +}
> 
> IIUC, this is changing the user expectations a bit. There are people
> using the mem= limit to hijack some top of the RAM for other needs
> (though they could do it in a saner way like changing the DT memory
> nodes).

Which will be hopelessly broken in the presence of KASLR, the kernel
being loaded at a different address, pages betting reserved differently
due to page size, etc.

I hope that no-one usees this for anything other than testing low-memory
conditions. If they want to steal memory they need to carve it out
explicitly.

We can behave as we used to, but we shouldn't give the impression that
such usage is supported.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1304668

On Fri, Jan 08, 2016 at 03:36:54PM +0000, Mark Rutland wrote:
> On Fri, Jan 08, 2016 at 03:27:38PM +0000, Catalin Marinas wrote:
> > On Wed, Dec 30, 2015 at 04:26:10PM +0100, Ard Biesheuvel wrote:
> > > +static void __init enforce_memory_limit(void)
> > > +{
> > > +	const phys_addr_t kbase = round_down(__pa(_text), MIN_KIMG_ALIGN);
> > > +	u64 to_remove = memblock_phys_mem_size() - memory_limit;
> > > +	phys_addr_t max_addr = 0;
> > > +	struct memblock_region *r;
> > > +
> > > +	if (memory_limit == (phys_addr_t)ULLONG_MAX)
> > > +		return;
> > > +
> > > +	/*
> > > +	 * The kernel may be high up in physical memory, so try to apply the
> > > +	 * limit below the kernel first, and only let the generic handling
> > > +	 * take over if it turns out we haven't clipped enough memory yet.
> > > +	 */
> > > +	for_each_memblock(memory, r) {
> > > +		if (r->base + r->size > kbase) {
> > > +			u64 rem = min(to_remove, kbase - r->base);
> > > +
> > > +			max_addr = r->base + rem;
> > > +			to_remove -= rem;
> > > +			break;
> > > +		}
> > > +		if (to_remove <= r->size) {
> > > +			max_addr = r->base + to_remove;
> > > +			to_remove = 0;
> > > +			break;
> > > +		}
> > > +		to_remove -= r->size;
> > > +	}
> > > +
> > > +	memblock_remove(0, max_addr);
> > > +
> > > +	if (to_remove)
> > > +		memblock_enforce_memory_limit(memory_limit);
> > > +}
> > 
> > IIUC, this is changing the user expectations a bit. There are people
> > using the mem= limit to hijack some top of the RAM for other needs
> > (though they could do it in a saner way like changing the DT memory
> > nodes).
> 
> Which will be hopelessly broken in the presence of KASLR, the kernel
> being loaded at a different address, pages betting reserved differently
> due to page size, etc.

With KASLR disabled, I think we should aim for the existing behaviour as
much as possible. The original aim of these patches was to relax the
kernel image placement rules, to make it easier for boot loaders rather
than completely randomising it.

With KASLR enabled, I agree it's hard to make any assumptions about what
memory is available. But removing memory only from the top would also
help with the point you already raised - keeping lower memory for
devices with narrower DMA mask.

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1304701

Hi Catalin,

I think we agree w.r.t. the code you suggest. I just disagree with the
suggestion that using mem= for carveouts is something we must, or even
could support -- it's already fragile.

More on that below.

On Fri, Jan 08, 2016 at 03:48:15PM +0000, Catalin Marinas wrote:
> On Fri, Jan 08, 2016 at 03:36:54PM +0000, Mark Rutland wrote:
> > On Fri, Jan 08, 2016 at 03:27:38PM +0000, Catalin Marinas wrote:
> > > On Wed, Dec 30, 2015 at 04:26:10PM +0100, Ard Biesheuvel wrote:
> > > > +static void __init enforce_memory_limit(void)
> > > > +{
> > > > +	const phys_addr_t kbase = round_down(__pa(_text), MIN_KIMG_ALIGN);
> > > > +	u64 to_remove = memblock_phys_mem_size() - memory_limit;
> > > > +	phys_addr_t max_addr = 0;
> > > > +	struct memblock_region *r;
> > > > +
> > > > +	if (memory_limit == (phys_addr_t)ULLONG_MAX)
> > > > +		return;
> > > > +
> > > > +	/*
> > > > +	 * The kernel may be high up in physical memory, so try to apply the
> > > > +	 * limit below the kernel first, and only let the generic handling
> > > > +	 * take over if it turns out we haven't clipped enough memory yet.
> > > > +	 */
> > > > +	for_each_memblock(memory, r) {
> > > > +		if (r->base + r->size > kbase) {
> > > > +			u64 rem = min(to_remove, kbase - r->base);
> > > > +
> > > > +			max_addr = r->base + rem;
> > > > +			to_remove -= rem;
> > > > +			break;
> > > > +		}
> > > > +		if (to_remove <= r->size) {
> > > > +			max_addr = r->base + to_remove;
> > > > +			to_remove = 0;
> > > > +			break;
> > > > +		}
> > > > +		to_remove -= r->size;
> > > > +	}
> > > > +
> > > > +	memblock_remove(0, max_addr);
> > > > +
> > > > +	if (to_remove)
> > > > +		memblock_enforce_memory_limit(memory_limit);
> > > > +}
> > > 
> > > IIUC, this is changing the user expectations a bit. There are people
> > > using the mem= limit to hijack some top of the RAM for other needs
> > > (though they could do it in a saner way like changing the DT memory
> > > nodes).
> > 
> > Which will be hopelessly broken in the presence of KASLR, the kernel
> > being loaded at a different address, pages betting reserved differently
> > due to page size, etc.
> 
> With KASLR disabled, I think we should aim for the existing behaviour as
> much as possible. The original aim of these patches was to relax the
> kernel image placement rules, to make it easier for boot loaders rather
> than completely randomising it.

Sure. My point was there were other reasons this is extremely fragile
currently, regardless of KASLR. For example, due to reservations
occurring differently.

Consider that when we add memory we may shave off portions of memory due
to page size, as we do in early_init_dt_add_memory_arch. Regions may be
fused or split for other reasons which may change over time, leading to
a different amount of memory being shaved off.

Afterwards memblock_enforce_memory_limit figures out the max address to keep
with:

        /* find out max address */
        for_each_memblock(memory, r) { 
                if (limit <= r->size) {
                        max_addr = r->base + limit;
                        break;
                }    
                limit -= r->size;
        }

Given all that, you cannot use mem= to prevent use of some memory, except for a
specific kernel binary with some value found by experimentation.

I think we need to make it clear that this is completely and hopelessly broken,
and should not pretend to support that.

> With KASLR enabled, I agree it's hard to make any assumptions about what
> memory is available.

As above, I do not think this is safe at all across kernel binaries.

> But removing memory only from the top would also > help with the point
> you already raised - keeping lower memory for > devices with narrower
> DMA mask.

I'm happy with the logic you suggest for the purpose of keeping low DMA
memory.

I think we must make it clear that mem= cannot be used to protect or
carve out memory -- it's a best effort tool for test purposes.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1304663

On 8 January 2016 at 16:27, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Wed, Dec 30, 2015 at 04:26:10PM +0100, Ard Biesheuvel wrote:
>> +static void __init enforce_memory_limit(void)
>> +{
>> +     const phys_addr_t kbase = round_down(__pa(_text), MIN_KIMG_ALIGN);
>> +     u64 to_remove = memblock_phys_mem_size() - memory_limit;
>> +     phys_addr_t max_addr = 0;
>> +     struct memblock_region *r;
>> +
>> +     if (memory_limit == (phys_addr_t)ULLONG_MAX)
>> +             return;
>> +
>> +     /*
>> +      * The kernel may be high up in physical memory, so try to apply the
>> +      * limit below the kernel first, and only let the generic handling
>> +      * take over if it turns out we haven't clipped enough memory yet.
>> +      */
>> +     for_each_memblock(memory, r) {
>> +             if (r->base + r->size > kbase) {
>> +                     u64 rem = min(to_remove, kbase - r->base);
>> +
>> +                     max_addr = r->base + rem;
>> +                     to_remove -= rem;
>> +                     break;
>> +             }
>> +             if (to_remove <= r->size) {
>> +                     max_addr = r->base + to_remove;
>> +                     to_remove = 0;
>> +                     break;
>> +             }
>> +             to_remove -= r->size;
>> +     }
>> +
>> +     memblock_remove(0, max_addr);
>> +
>> +     if (to_remove)
>> +             memblock_enforce_memory_limit(memory_limit);
>> +}
>
> IIUC, this is changing the user expectations a bit. There are people
> using the mem= limit to hijack some top of the RAM for other needs
> (though they could do it in a saner way like changing the DT memory
> nodes). Your patch first tries to remove the memory below the kernel
> image and only remove the top if additional limitation is necessary.
>
> Can you not remove memory from the top and block the limit if it goes
> below the end of the kernel image, with some warning that memory limit
> was not entirely fulfilled?
>

I'm in the middle of rewriting this code from scratch. The general idea is

static void __init clip_mem_range(u64 min, u64 max);

/*
* Clip memory in order of preference:
* - above the kernel and above 4 GB
* - between 4 GB and the start of the kernel
* - below 4 GB
* Note that tho
*/
clip_mem_range(max(sz_4g, PAGE_ALIGN(__pa(_end))), ULLONG_MAX);
clip_mem_range(sz_4g, round_down(__pa(_text), MIN_KIMG_ALIGN));
clip_mem_range(0, sz_4g);

where clip_mem_range() iterates over the memblocks to remove memory
between min and max iff min < max and the limit has not been met yet.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web