[PATCH 0/4] arm64: Make kexec_file_load honor iomem reservations

[PATCH 0/4] arm64: Make kexec_file_load honor iomem reservations

[Marc Zyngier]

This series is a complete departure from the approach I initially sent
almost a month ago[1]. Instead of trying to teach EFI, ACPI and other
subsystem to use memblock, I've decided to stick with the iomem
resource tree and use that exclusively for arm64.

This means that my current approach is (despite what I initially
replied to both Dave and Catalin) to provide an arm64-specific
implementation of arch_kexec_locate_mem_hole() which walks the
resource tree and excludes ranges of RAM that have been registered for
any odd purpose. This is exactly what the userspace implementation
does, and I don't really see a good reason to diverge from it.

Again, this allows my Synquacer board to reliably use kexec_file_load
with as little as 256M, something that would always fail before as it
would overwrite most of the reserved tables.

Obviously, this is now at least 5.14 material. Given how broken
kexec_file_load is for non-crash kernels on arm64 at the moment,
should we at least disable it in 5.13 and all previous stable kernels?

Thanks,

	M.

[1] https://lore.kernel.org/r/20210429133533.1750721-1-maz@kernel.org

Marc Zyngier (4):
  kexec_file: Make locate_mem_hole_callback global
  kernel/resource: Populate child pointer in find_next_iomem_res()
  kernel/resource: Add walk_excluding_child_res() helper
  arm64: kexec_image: Implement arch_kexec_locate_mem_hole()

 arch/arm64/kernel/kexec_image.c | 45 ++++++++++++++++++
 include/linux/ioport.h          |  4 ++
 include/linux/kexec.h           |  1 +
 kernel/kexec_file.c             |  6 +--
 kernel/resource.c               | 81 +++++++++++++++++++++++++++++++++
 5 files changed, 134 insertions(+), 3 deletions(-)

-- 
2.30.2

[PATCH 1/4] kexec_file: Make locate_mem_hole_callback global

[Marc Zyngier]

In order for architectures to make use of locate_mem_hole_callback()
and avoid reinventing a square wheel, make this function global
and rename it to kexec_locate_mem_hole_callback() to match the
other global kexec symbols.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 include/linux/kexec.h | 1 +
 kernel/kexec_file.c   | 6 +++---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 0c994ae37729..4b507efdb623 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -204,6 +204,7 @@ int arch_kexec_locate_mem_hole(struct kexec_buf *kbuf);
 
 extern int kexec_add_buffer(struct kexec_buf *kbuf);
 int kexec_locate_mem_hole(struct kexec_buf *kbuf);
+int kexec_locate_mem_hole_callback(struct resource *res, void *arg);
 
 /* Alignment required for elf header segment */
 #define ELF_CORE_HEADER_ALIGN   4096
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 33400ff051a8..960aefc4501d 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -517,7 +517,7 @@ static int locate_mem_hole_bottom_up(unsigned long start, unsigned long end,
 	return 1;
 }
 
-static int locate_mem_hole_callback(struct resource *res, void *arg)
+int kexec_locate_mem_hole_callback(struct resource *res, void *arg)
 {
 	struct kexec_buf *kbuf = (struct kexec_buf *)arg;
 	u64 start = res->start, end = res->end;
@@ -634,9 +634,9 @@ int kexec_locate_mem_hole(struct kexec_buf *kbuf)
 		return 0;
 
 	if (!IS_ENABLED(CONFIG_ARCH_KEEP_MEMBLOCK))
-		ret = kexec_walk_resources(kbuf, locate_mem_hole_callback);
+		ret = kexec_walk_resources(kbuf, kexec_locate_mem_hole_callback);
 	else
-		ret = kexec_walk_memblock(kbuf, locate_mem_hole_callback);
+		ret = kexec_walk_memblock(kbuf, kexec_locate_mem_hole_callback);
 
 	return ret == 1 ? 0 : -EADDRNOTAVAIL;
 }
-- 
2.30.2

[PATCH 2/4] kernel/resource: Populate child pointer in find_next_iomem_res()

[Marc Zyngier]

When find_next_iomem_res() returns a resource, it doesn't
populate the child pointer (but does so with the parent).

As we are about to need to arse child resources, populate
this pointer as well.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 kernel/resource.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/resource.c b/kernel/resource.c
index ca9f5198a01f..311b8d2c9957 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -377,6 +377,7 @@ static int find_next_iomem_res(resource_size_t start, resource_size_t end,
 			.flags = p->flags,
 			.desc = p->desc,
 			.parent = p->parent,
+			.child = p->child,
 		};
 	}
 
-- 
2.30.2

Re: [PATCH 2/4] kernel/resource: Populate child pointer in find_next_iomem_res()

[Catalin Marinas]

On Wed, May 26, 2021 at 08:05:29PM +0100, Marc Zyngier wrote:
> When find_next_iomem_res() returns a resource, it doesn't
> populate the child pointer (but does so with the parent).
> 
> As we are about to need to arse child resources, populate
> this pointer as well.

Did you mean "parse"? ;)

-- 
Catalin

Re: [PATCH 2/4] kernel/resource: Populate child pointer in find_next_iomem_res()

[Marc Zyngier]

On Thu, 27 May 2021 17:53:56 +0100,
Catalin Marinas <catalin.marinas@arm.com> wrote:
> 
> On Wed, May 26, 2021 at 08:05:29PM +0100, Marc Zyngier wrote:
> > When find_next_iomem_res() returns a resource, it doesn't
> > populate the child pointer (but does so with the parent).
> > 
> > As we are about to need to arse child resources, populate
> > this pointer as well.
> 
> Did you mean "parse"? ;)

This is so embarrassing... :-/ Let me fix that right now.

	M.

-- 
Without deviation from the norm, progress is not possible.

[PATCH 3/4] kernel/resource: Add walk_excluding_child_res() helper

[Marc Zyngier]

Once we have obtained a resource of a certain type from
find_next_iomem_res(), it doesn't necessarily mean that the whole
resource is usable, and we have cases where a child resource
denotes an exclusion in the initial resource.

Provide a new walker that deals with this exact case, and calls
a callback on each resource fragment that doesn't have a child.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 include/linux/ioport.h |  4 +++
 kernel/resource.c      | 80 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 8359c50f9988..526314a42ad2 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -323,6 +323,10 @@ extern int
 walk_iomem_res_desc(unsigned long desc, unsigned long flags, u64 start, u64 end,
 		    void *arg, int (*func)(struct resource *, void *));
 
+extern int
+walk_excluding_child_res(struct resource *res, void *arg,
+			 int (*func)(struct resource *, void *));
+
 struct resource *devm_request_free_mem_region(struct device *dev,
 		struct resource *base, unsigned long size);
 struct resource *request_free_mem_region(struct resource *base,
diff --git a/kernel/resource.c b/kernel/resource.c
index 311b8d2c9957..1d9b5f653938 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -318,6 +318,86 @@ int release_resource(struct resource *old)
 
 EXPORT_SYMBOL(release_resource);
 
+/**
+ * walk_excluding_child_res - call a callback function on each fragment of
+ *			      a resource that do not have a child resource
+ *
+ * @res: the root resource containing the initial range
+ * @arg: function argument for the callback @func
+ * @func: callback function that is called for each qualifying resource area
+ *
+ * For a given resource, remove all the child resources and feed the
+ * resulting fragments to kexec_locate_mem_hole_callback().
+ */
+int walk_excluding_child_res(struct resource *res, void *arg,
+			     int (*func)(struct resource *, void *))
+{
+	struct resource *tmp, cursor;
+	int ret = 0;
+
+	cursor = *res;
+
+	/* Use .child for the head of the list, .sibling for the tail */
+	cursor.child = cursor.sibling = NULL;
+
+	read_lock(&resource_lock);
+
+	for (tmp = res->child; tmp; tmp = tmp->sibling) {
+		struct resource *new;
+
+		if (cursor.start < tmp->start) {
+			new = kmalloc(sizeof(*new), GFP_KERNEL);
+			if (!new)
+				goto cleanup;
+
+			*new = (struct resource) {
+				.start	= cursor.start,
+				.end	= tmp->start - 1,
+				.flags	= res->flags,
+				.desc	= res->desc,
+				.parent	= res->parent,
+			};
+
+			if (!cursor.child)
+				cursor.child = new;
+			if (cursor.sibling)
+				cursor.sibling->sibling = new;
+			cursor.sibling = new;
+		}
+
+		/*
+		 * This may result in a resource with a negative size
+		 * at the very end of the loop.
+		 */
+		cursor.start = tmp->end + 1;
+	}
+
+	read_unlock(&resource_lock);
+
+	/*
+	 * At this stage, the list pointed to by cursor.child contains
+	 * every non-reserved blocks, completed by 'cursor' which
+	 * contains the potential last block (may be empty).
+	 */
+	for (tmp = cursor.child; tmp; tmp = tmp->sibling) {
+		ret = func(tmp, arg);
+		if (ret)
+			break;
+	}
+
+	if (!ret && cursor.start <= cursor.end)
+		ret = func(&cursor, tmp);
+
+cleanup:
+	while (cursor.child) {
+		tmp = cursor.child;
+		cursor.child = cursor.child->sibling;
+		kfree(tmp);
+	}
+
+	return ret;
+}
+
 /**
  * find_next_iomem_res - Finds the lowest iomem resource that covers part of
  *			 [@start..@end].
-- 
2.30.2

[PATCH 4/4] arm64: kexec_image: Implement arch_kexec_locate_mem_hole()

[Marc Zyngier]

Provide an arm64-specific implementation for arch_kexec_locate_mem_hole(),
using the resource tree instead of memblock, and respecting
the reservations added by EFI.

This ensures that kexec_file is finally reliable.

Reported-by: Moritz Fischer <mdf@kernel.org>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kernel/kexec_image.c | 45 +++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/arch/arm64/kernel/kexec_image.c b/arch/arm64/kernel/kexec_image.c
index 9ec34690e255..91e852c735df 100644
--- a/arch/arm64/kernel/kexec_image.c
+++ b/arch/arm64/kernel/kexec_image.c
@@ -145,3 +145,48 @@ const struct kexec_file_ops kexec_image_ops = {
 	.verify_sig = image_verify_sig,
 #endif
 };
+
+static int filter_out_reserved_res(struct resource *res, void *arg)
+{
+	return walk_excluding_child_res(res, arg,
+					kexec_locate_mem_hole_callback);
+}
+
+/**
+ * arch_kexec_locate_mem_hole - Find free memory to place the segments.
+ * @kbuf:                       Parameters for the memory search.
+ *
+ * On success, kbuf->mem will have the start address of the memory region found.
+ *
+ * Return: 0 on success, negative errno on error.
+ */
+int arch_kexec_locate_mem_hole(struct kexec_buf *kbuf)
+{
+	int ret;
+
+	/* Arch knows where to place */
+	if (kbuf->mem != KEXEC_BUF_MEM_UNKNOWN)
+		return 0;
+
+	/*
+	 * Crash kernels land in a well known place that has been
+	 * reserved upfront.
+	 *
+	 * Normal kexec kernels can however land anywhere in memory.
+	 * We have to be extra careful not to step over critical
+	 * memory ranges that have been marked as reserved in the
+	 * iomem resource tree (LPI and ACPI tables, among others),
+	 * hence the use of the child-excluding iterator.  This
+	 * matches what the userspace version of kexec does.
+	 */
+	if (kbuf->image->type == KEXEC_TYPE_CRASH)
+		ret = walk_iomem_res_desc(crashk_res.desc,
+					  IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY,
+					  crashk_res.start, crashk_res.end,
+					  kbuf, kexec_locate_mem_hole_callback);
+	else
+		ret = walk_system_ram_res(0, ULONG_MAX, kbuf,
+					  filter_out_reserved_res);
+
+	return ret == 1 ? 0 : -EADDRNOTAVAIL;
+}
-- 
2.30.2

Re: [PATCH 4/4] arm64: kexec_image: Implement arch_kexec_locate_mem_hole()

[Catalin Marinas]

On Wed, May 26, 2021 at 08:05:31PM +0100, Marc Zyngier wrote:
> Provide an arm64-specific implementation for arch_kexec_locate_mem_hole(),
> using the resource tree instead of memblock, and respecting
> the reservations added by EFI.
> 
> This ensures that kexec_file is finally reliable.
> 
> Reported-by: Moritz Fischer <mdf@kernel.org>
> Signed-off-by: Marc Zyngier <maz@kernel.org>

It would have been clearer if __walk_iomem_res_desc() was able to do
such child res excluding callback (if asked via a new flag/arg) directly
but it's too late in the day to figure out if it's possible. It would
save us from another callback in the arch code. But if it's not possible
or you want to stick to this approach, fine by me:

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

-- 
Catalin

Re: [PATCH 4/4] arm64: kexec_image: Implement arch_kexec_locate_mem_hole()

[Marc Zyngier]

On Thu, 27 May 2021 18:37:37 +0100,
Catalin Marinas <catalin.marinas@arm.com> wrote:
> 
> On Wed, May 26, 2021 at 08:05:31PM +0100, Marc Zyngier wrote:
> > Provide an arm64-specific implementation for arch_kexec_locate_mem_hole(),
> > using the resource tree instead of memblock, and respecting
> > the reservations added by EFI.
> > 
> > This ensures that kexec_file is finally reliable.
> > 
> > Reported-by: Moritz Fischer <mdf@kernel.org>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> 
> It would have been clearer if __walk_iomem_res_desc() was able to do
> such child res excluding callback (if asked via a new flag/arg) directly
> but it's too late in the day to figure out if it's possible. It would
> save us from another callback in the arch code.

Yeah, that should be possible with some minor refactoring of the
generic and x86 code, allowing us to get rid of the double arch
callback circus.

It would also make the locking a bit saner, but also change it for all
the callers... I'll have a play with it.

> But if it's not possible or you want to stick to this approach, fine
> by me:
> 
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 0/4] arm64: Make kexec_file_load honor iomem reservations

[Catalin Marinas]

On Wed, May 26, 2021 at 08:05:27PM +0100, Marc Zyngier wrote:
> This series is a complete departure from the approach I initially sent
> almost a month ago[1]. Instead of trying to teach EFI, ACPI and other
> subsystem to use memblock, I've decided to stick with the iomem
> resource tree and use that exclusively for arm64.
> 
> This means that my current approach is (despite what I initially
> replied to both Dave and Catalin) to provide an arm64-specific
> implementation of arch_kexec_locate_mem_hole() which walks the
> resource tree and excludes ranges of RAM that have been registered for
> any odd purpose. This is exactly what the userspace implementation
> does, and I don't really see a good reason to diverge from it.
> 
> Again, this allows my Synquacer board to reliably use kexec_file_load
> with as little as 256M, something that would always fail before as it
> would overwrite most of the reserved tables.
> 
> Obviously, this is now at least 5.14 material. Given how broken
> kexec_file_load is for non-crash kernels on arm64 at the moment,
> should we at least disable it in 5.13 and all previous stable kernels?

I think it makes sense to disable it in the current and earlier kernels.

For this series:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH 0/4] arm64: Make kexec_file_load honor iomem reservations

[Ard Biesheuvel]

On Thu, 27 May 2021 at 19:39, Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Wed, May 26, 2021 at 08:05:27PM +0100, Marc Zyngier wrote:
> > This series is a complete departure from the approach I initially sent
> > almost a month ago[1]. Instead of trying to teach EFI, ACPI and other
> > subsystem to use memblock, I've decided to stick with the iomem
> > resource tree and use that exclusively for arm64.
> >
> > This means that my current approach is (despite what I initially
> > replied to both Dave and Catalin) to provide an arm64-specific
> > implementation of arch_kexec_locate_mem_hole() which walks the
> > resource tree and excludes ranges of RAM that have been registered for
> > any odd purpose. This is exactly what the userspace implementation
> > does, and I don't really see a good reason to diverge from it.
> >
> > Again, this allows my Synquacer board to reliably use kexec_file_load
> > with as little as 256M, something that would always fail before as it
> > would overwrite most of the reserved tables.
> >
> > Obviously, this is now at least 5.14 material. Given how broken
> > kexec_file_load is for non-crash kernels on arm64 at the moment,
> > should we at least disable it in 5.13 and all previous stable kernels?
>
> I think it makes sense to disable it in the current and earlier kernels.
>

Ack to that

> For this series:
>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>

and likewise for the series

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>