[U-Boot] libfdt: make fdt_increase_size() available to everyone

[Timur Tabi]

Wolfgang Denk wrote:

> I reject your patch because it introduces the risk of crashing the
> system and it appears you do not want to be bothered fixing this.

But I believe I have already fixed it by stating that any users of
fdt_increase_size() must ensure that the new size fits in the allocated
area.  The same rules apply to functions like strcat(), so I don't
understand why you are so concerned.

> And I said that this is not sufficient. Static buffers have always
> been a bad idea as there will always be a user who needs more space.
> In this case, where we know the amount needed, we can as well arrange
> for it. You may have to add some code to implemenmt this, butthen
> adding this code has to be part of your patch submission.

But there is no way to ensure that every user of fdt_increase_size() will
follow those rules.

I could, for instance, add an lmb parameter to fdt_increase_size(), but this
will only apply in instances where the fdt exists inside an lmb. There is no
lmb_realloc() function, so the most I can do is return an error if the lmb
isn't big enough.

The problem is that by the time the code needs to resize the fdt, it has
long since forgotten about the lmb.  We would need to modify
ft_board_setup() to take an lmb as a parameter, but that would require
changes to almost 30 boards!

And then, that doesn't help the situation where the fdt is just dumped in
memory somewhere, like this:

tftp 0xc0000 my.dtb
bootm xxx xxx 0xc0000

In this situation, no lmb is created, so there's no way to know if there's
anything at address 0xc4000 that could be overwritten.

So the best I can do is this:

int fdt_increase_size(struct lmb *lmb, void *fdt, int add_len)
{
	int newlen;

	newlen = fdt_totalsize(fdt) + add_len;

	if (lmb) {
		int lmb_size = lmb_size(lmb);

		if (newlen > lmb_size)
			return -ENOMEM;
	}

	/* Open in place with a new len */
	return fdt_open_into(fdt, fdt, newlen);
}

So if an lmb is provided, we can check for overwrites, but we can't require
the lmb.

The only problem with the above function is that lmb_size() doesn't exist,
and I'm not sure how to implement it yet.  There doesn't appear to be any
useful support functions for the lmb code at the moment.

> Right. We don't provide safety belts against doing stupid things. But
> you mentioned your extension is needed to insert firmware blobs which
> can take "significant size" - you cannot estimate in advace (at
> compile time) which size the end user may need. 

Actually, I can.  For firmware, at least, I have macro called
CONFIG_SYS_FSL_FW_MAXSIZE that defines the largest size that firmware can
be.  This is used not only to specify the new size of the lmb (if
applicable), but to also ensure limits on the firmware itself.  This would
be a board-specific setting, since each board knows what kind of firmware it
needs.  And then I have this:

#ifndef CONFIG_SYS_FDT_PAD_DFLT
#define CONFIG_SYS_FDT_PAD_DFLT 0x3000
#endif

#ifndef CONFIG_SYS_FMAN_FW_MAXSIZE
#define CONFIG_SYS_FMAN_FW_MAXSIZE 0
#endif

#ifndef CONFIG_SYS_FDT_PAD
#define CONFIG_SYS_FDT_PAD (CONFIG_SYS_FDT_PAD_DFLT +
CONFIG_SYS_FMAN_FW_MAXSIZE)
#endif

Thus, each board can define its own settings, and boot_relocate_fdt() will
honor them.

> So using a static
> buffer size is a stupid thing too do as is is bound to fail rather
> sooner than later.

AFAIK, lmb regions cannot be resized, so once we create an lmb region for
the fdt, we're stuck with it.  That's why I've enhanced boot_relocate_fdt()
to ensure there's enough room in the lmb region to hold the firmware.  I
just haven't posted *that* code yet, since it's part of the P4080DS board
support that isn't ready yet.

> I will not accept code that relies on such compile time assumptions
> about dynamically inserted data.

I don't see any way around that.

The firmware is inserted after the lmb is created, and the code that creates
the lmb region has no knowledge of the firmware (or any other board-specific
entities that are going to be inserted into the fdt).  The best I can do is
force the firmware to be smaller than a compile-time limit, and ensure the
lmb region is expanded by that value.

> [Note that it does not help that *you* *shout* *at* *me* all the time
> that you need this *now*. This does not make your patch any better.]

Shouting is done with ALL CAPS.  Framing words with asterisks is used to
designate emphasis.

-- 
Timur Tabi
Linux kernel developer at Freescale