About atags_proc buffer size

PATCH Vojtech Bocek 3 participants 6 messages Sep 27, 2013 Arm (32-bit)

Share & tools

Vojtech Bocek

author

2013-09-27 20:25 UTC #0 lore raw

Hello,
I want to ask something about atags_proc.c implementation. Currently, it uses a buffer to temporarily store atags. The buffer size is set to 1.5kb for some reason, but as far as I know, atag list's size is not limited in any way. I've got a device (HTC One) which uses about 12kb of tags, that means it panics during boot if CONFIG_ATAGS_PROC is enabled, because the buffer contains only part of the tag list without an end tag.

Why is only 1.5kb used? Is it some specific size the device should not exceed?

I don't know much about the way ARM boot process works, but I tried to store just the pointer to atag list, and it works fine (quick patch attached). Do atags get erased later in boot process on some platforms, is that the reason why buffer had to be used?
This requires modifications in kexec-tools, because it uses that 1.5kb buffer, too. Again, here's my version of such modification[1]. If this is okay, I can create a proper patch and submit it.

Yours,
Vojtech Bocek

[1]: https://github.com/Tasssadar/kexec-tools/commit/c6844e1ddb13a6b60cfefcb01c3843da97d6174c

---

arch/arm/kernel/atags_proc.c +4 -5

index c7ff807..12cc483 100644
@@ -21,12 +21,11 @@ static const struct file_operations atags_fops = {
 	.llseek = default_llseek,
 };
 
-#define BOOT_PARAMS_SIZE 1536
-static char __initdata atags_copy[BOOT_PARAMS_SIZE];
+static const struct tag* __initdata atags_copy;
 
 void __init save_atags(const struct tag *tags)
 {
-	memcpy(atags_copy, tags, sizeof(atags_copy));
+	atags_copy = tags;
 }
 
 static int __init init_atags_procfs(void)

@@ -40,7 +39,7 @@ static int __init init_atags_procfs(void)
 	struct buffer *b;
 	size_t size;
 
-	if (tag->hdr.tag != ATAG_CORE) {
+	if (!atags_copy || tag->hdr.tag != ATAG_CORE) {
 		printk(KERN_INFO "No ATAGs?");
 		return -EINVAL;
 	}

@@ -49,7 +48,7 @@ static int __init init_atags_procfs(void)
 		;
 
 	/* include the terminating ATAG_NONE */
-	size = (char *)tag - atags_copy + sizeof(struct tag_header);
+	size = (char *)tag - (char *)atags_copy + sizeof(struct tag_header);
 
 	WARN_ON(tag->hdr.tag != ATAG_NONE);

Russell King - ARM Linux

2013-09-27 20:47 UTC #1 lore raw

Quoted 7 lines · Vojtech Bocek

I want to ask something about atags_proc.c implementation. Currently, it uses a buffer to temporarily store atags. The buffer size is set to 1.5kb for some reason, but as far as I know, atag list's size is not limited in any way. I've got a device (HTC One) which uses about 12kb of tags, that means it panics during boot if CONFIG_ATAGS_PROC is enabled, because the buffer contains only part of the tag list without an end tag.

The tags are supposed to be a short-lived structure which gets used to pass barest minimum of details to the kernel, and the data stored in them almost certainly gets overwritten before the kernels memory allocators are up and running.

So, we need to statically allocate some space to save these things - it can't be done dynamically.

The problem is this: for the vast majority of platforms, they pass no more than 1.5kB (lower case b is *bits* not *bytes*) to the kernel in their tagged list. Having a static allocation of 12k would be wasteful for the majority of users.

Quoted 4 lines

I don't know much about the way ARM boot process works, but I tried to store just the pointer to atag list, and it works fine (quick patch attached). Do atags get erased later in boot process on some platforms, is that the reason why buffer had to be used?

This may appear to work, but check it after you've been running for some time and have exercised the memory systems. You'll probably find that your tags have vanished!

Vojtech Bocek

author

2013-09-27 21:09 UTC #2 lore raw

It only needs to survive until init_atags_procfs is called, because it is copied to another buffer for procfs entry. Can I be sure it survives until that? My guess is that it is likely to survive, but not certain.

I suppose it is possible to somehow protect that bit of ram until it is read by init_atags_procfs, but I wonder if you even want to do that in mainline - if majority of devices doesn't use such big tag lists, then it is probably that device's vendor problem. I've met this problem on two devices so far though, both of them Android phones, one is the HTC One (that is MSM APQ8064 SoC) and I unfortunately can't remember the first one - I discarded it as usual Android kernel's mess.

Quoted 30 lines · Russell King - ARM Linux

Quoted 7 lines · Vojtech Bocek
I want to ask something about atags_proc.c implementation. Currently, it uses a buffer to temporarily store atags. The buffer size is set to 1.5kb for some reason, but as far as I know, atag list's size is not limited in any way. I've got a device (HTC One) which uses about 12kb of tags, that means it panics during boot if CONFIG_ATAGS_PROC is enabled, because the buffer contains only part of the tag list without an end tag.
The tags are supposed to be a short-lived structure which gets used to pass barest minimum of details to the kernel, and the data stored in them almost certainly gets overwritten before the kernels memory allocators are up and running.
So, we need to statically allocate some space to save these things - it can't be done dynamically.
The problem is this: for the vast majority of platforms, they pass no more than 1.5kB (lower case b is *bits* not *bytes*) to the kernel in their tagged list. Having a static allocation of 12k would be wasteful for the majority of users.
Quoted 4 lines
I don't know much about the way ARM boot process works, but I tried to store just the pointer to atag list, and it works fine (quick patch attached). Do atags get erased later in boot process on some platforms, is that the reason why buffer had to be used?
This may appear to work, but check it after you've been running for some time and have exercised the memory systems. You'll probably find that your tags have vanished!

Russell King - ARM Linux

2013-09-27 21:15 UTC #3 lore raw

Quoted 11 lines · Vojtech Bocek

It only needs to survive until init_atags_procfs is called, because it is copied to another buffer for procfs entry. Can I be sure it survives until that? My guess is that it is likely to survive, but not certain.
I suppose it is possible to somehow protect that bit of ram until it is read by init_atags_procfs, but I wonder if you even want to do that in mainline - if majority of devices doesn't use such big tag lists, then it is probably that device's vendor problem. I've met this problem on two devices so far though, both of them Android phones, one is the HTC One (that is MSM APQ8064 SoC) and I unfortunately can't remember the first one - I discarded it as usual Android kernel's mess.

We've been through several early allocators - particularly one which allocates from the bottom of memory upwards, which would overwrite the ATAGs long before init_atags_procfs gets called.

If we rely on the behaviour of the current early allocator not to touch that, and it changes in the future, that's going to be rather too fragile.

Vojtech Bocek

author

2013-09-27 21:25 UTC #4 lore raw

Okay then, I suppose there is no nice way around that, or at least none that I can find. I'll just make that initial buffer 12kb big on my kernel for that device and be done with it.

Anyway, thanks for the information and help, it is much appreciated.

Quoted 20 lines · Russell King - ARM Linux

Quoted 11 lines · Vojtech Bocek
It only needs to survive until init_atags_procfs is called, because it is copied to another buffer for procfs entry. Can I be sure it survives until that? My guess is that it is likely to survive, but not certain.
I suppose it is possible to somehow protect that bit of ram until it is read by init_atags_procfs, but I wonder if you even want to do that in mainline - if majority of devices doesn't use such big tag lists, then it is probably that device's vendor problem. I've met this problem on two devices so far though, both of them Android phones, one is the HTC One (that is MSM APQ8064 SoC) and I unfortunately can't remember the first one - I discarded it as usual Android kernel's mess.
We've been through several early allocators - particularly one which allocates from the bottom of memory upwards, which would overwrite the ATAGs long before init_atags_procfs gets called.
If we rely on the behaviour of the current early allocator not to touch that, and it changes in the future, that's going to be rather too fragile.

Geert Uytterhoeven

2013-09-27 21:39 UTC #5 lore raw

In reply to Russell King - ARM Linux

Quoted 14 lines · Russell King - ARM Linux

Quoted 7 lines · Vojtech Bocek
I want to ask something about atags_proc.c implementation. Currently, it uses a buffer to temporarily store atags. The buffer size is set to 1.5kb for some reason, but as far as I know, atag list's size is not limited in any way. I've got a device (HTC One) which uses about 12kb of tags, that means it panics during boot if CONFIG_ATAGS_PROC is enabled, because the buffer contains only part of the tag list without an end tag.
The tags are supposed to be a short-lived structure which gets used to pass barest minimum of details to the kernel, and the data stored in them almost certainly gets overwritten before the kernels memory allocators are up and running.

Quoted 7 lines

So, we need to statically allocate some space to save these things - it can't be done dynamically.
The problem is this: for the vast majority of platforms, they pass no more than 1.5kB (lower case b is *bits* not *bytes*) to the kernel in their tagged list. Having a static allocation of 12k would be wasteful for the majority of users.

It's __initdata memory, right?
So it would waste this 12 KiB only temporarily.
Stll, it enlarges the kernel image by 12 KiB, as there's no such thing as __initbss.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds