Re: [RESEND 2][PATCH v4] hexdump: fix for non-aligned buffers

Andrew Morton
2015-07-09T02:06:46Z

On Thu, 9 Jul 2015 03:36:02 +0200 "H. Mijail" <hmijail@gmail.com> wrote:

>
> > On 09 Jul 2015, at 02:03, Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Thu, 9 Jul 2015 01:44:18 +0200 Horacio Mijail Ant__n Quiles <hmijail@gmail.com> wrote:
> >
> >> An hexdump with a buf not aligned to the groupsize causes
> >> non-naturally-aligned memory accesses. This was causing a kernel panic on
> >> the processor BlackFin BF527, when such an unaligned buffer was fed by the
> >> function ubifs_scanned_corruption in fs/ubifs/scan.c .
> >>
> >> To fix this, if the buffer is not aligned to groupsize in a platform which
> >> does not define CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS, then change the
> >> groupsize to 1, so alignment is no longer a problem.
> >> This behavior is coherent with the way the function currently deals with
> >> inappropriate parameter combinations, which is to fall back to safe
> >> "defaults", even if that means changing the output format and the implicit
> >> access patterns that could have been expected.
> >
> > CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS seems inappropriate for this.
> > Having this unset means "can do unaligned accesses, but they're
> > inefficient". It doesn't mean "unaligned accesses go oops".
> >
> > But I can't the appropriate config knob. There's a
> > CONFIG_CPU_HAS_NO_UNALIGNED, but that's an m68k-private thing.
>
> I'm only a newbie, but I will argue that the lesser evil is checking
> CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS - until a new configure variable
> is defined.
>
> In Documentation/unaligned-memory-access.txt, an undefined
> CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS is taken as if to mean 'the
> hardware isn't able to access memory on arbitrary boundaries'.

hm, yes, OK, CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS is a poor name.

> The other alternative in Documentation/unaligned-memory-access.txt is the
> macro get_unaligned() from asm/unaligned.h. However, using get_unaligned()
> would mean a much more intrusive patch, since each case of the groupsize
> would be changed, and anyway we would still need to check
> CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS to avoid penalising everyone.

Actually, I think using get_unaligned() would be a better solution.
For architectures which have CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS=y,
get_unaligned() should be fast - just one instruction.

This way we avoid having different-appearing output on different
architectures.

> >> Resent on 8 Jul because of no answers.
> >>
> >> Resent on 29 Jun because of no answers.
> >
> > During the merge window. So I have everything sitting there in my
> > patches-to-process pile. The backlog is excessive this time (700+
> > emails) so I'm thinking I'll change things so I'll henceforth be
> > processing patches-for-the-next-cycle during this-cycle, while keeping
> > patches-for-next-cycle out of linux-next.
>
> No problem for me - if I should squelch the next version of this patch
> for some time, please let me know.

The merge window has ended ;)

> >
> > But as mentioned above, that's different from "architectures which do
> > not support them efficently"! Maybe we need a new config variable.
> >
> > Or maybe blackfin should be handling the unaligned access trap and
> > transparently handling it, like sparc?
> >
>
> I'll wait for anyone else to weight in'

Possibly blackfin *could* emulate unaligned accesses. But according to
the documentation, hex_dump_to_buffer() needs to be altered anyway
because we cannot rely on the architecture handling such accesses.