n Mon, Mar 29, 2021 at 1:05 PM Linus Torvalds <torva...@linux-foundation.org> wrote: > > On Sun, Mar 28, 2021 at 7:07 PM Guenter Roeck <li...@roeck-us.net> wrote: > > > > This is not really a new problem. I enabled devicetree unit tests > > in the openrisc kernel and was rewarded with a crash. > > https://lore.kernel.org/lkml/20210327224116.69309-1-li...@roeck-us.net/ > > has all the glorious details. > > Hmm. > > I'm not sure I love that patch. > > I don't think the patch is _wrong_ per se, but if that "require 8 byte > alignment" is a problem, then this seems to be papering over the issue > rather than fixing it. > > So your patch protects from a NULL pointer dereference, but the > underlying issue seems to be a regression, and the fix sounds like the > kernel shouldn't be so strict about alignment requirements.
In the interest of the DT unittests not panicking and halting boot, I think we should handle NULL pointer. > I guess we could make ARCH_SLAB_MINALIGN be at least 8 (perhaps only > if the allocations is >= 8) but honestly, I don't think libfdt merits > making such a big change. Small allocations are actually not uncommon > in the kernel, and on 32-bit architectures I think 4-byte allocations > are normal. > > So I'd be inclined to just remove the new > > /* The device tree must be at an 8-byte aligned address */ > if ((uintptr_t)fdt & 7) > return -FDT_ERR_ALIGNMENT; > > check in scripts/dtc/libfdt/fdt.c which I assume is the source of the > problem. Rob? That is the source, but I'd rather not remove it as we try to avoid any modifications from upstream. And we've found a couple of cases of not following documented alignment requirements. > Your patch to then avoid the NULL pointer dereference seems to be then > an additional safety, but not fixing the actual regression. I think the right fix is not using kmemdup which copies the unittest dtb. Rob
