10.0-RC1: net/mpd5 crashes in NgMkSockNode due to stack alignment on ARM EABI

[Guy Yur]

On Sat, Dec 21, 2013 at 9:15 PM, John-Mark Gurney <jmg at funkthat.com> wrote:
> Guy Yur wrote this message on Sat, Dec 21, 2013 at 19:24 +0200:
>> I am running 10.0-RC1 on the BeagleBone Black and the net/mpd5 port is
>> crashing in libnetgraph NgMkSockNode due to stack alignment.
>>
>> 10.0-RC1 World and kernel were compiled in a VirtualBox VM running
>> 9.2-RELEASE-p2 i386.
>> clang and ARM_EABI used as the default make options.
>>
>> Added prints in NgMkSockNode show rbuf is aligned on 2-byte and not
>> 4-byte which is needed to access ni->id (a uint32_t).
>>
>> ni = 0xbfffe87a
>> rbuf = 0xbfffe842
>> sizeof(resp->header) = 56
>>
>>
>> (gdb) bt
>> #0  0x201529a0 in NgMkSockNode (name=<value optimized out>, csp=0xbfffe95c,
>>     dsp=0xbfffe958) at /usr/src/lib/libnetgraph/sock.c:134
>> #1  0x00037b9c in MppcTestCap () at ccp_mppc.c:754
>> #2  0x0007c1f4 in main (ac=4, av=0xbfffeb90) at main.c:248
>> #3  0x0000d1b0 in __start (argc=4, argv=0xbfffeb90, env=0xbfffeba4,
>>     ps_strings=<value optimized out>, obj=<value optimized out>,
>>     cleanup=<value optimized out>) at /usr/src/lib/csu/arm/crt1.c:115
>> #4  0x203e9dc0 in _thr_ast (curthread=0x200fd000)
>>     at /usr/src/lib/libthr/thread/thr_sig.c:265
>>
>>
>> Putting rbuf in a union with struct ng_mesg sorted the alignment to
>> 4-byte and mpd5 didn't crash.
>> I attached the changes I used to test mpd5 doesn't crash with correct alignment.
>
> The patch looks correct, but lets make sure that the -net people don't
> have an issue with it...
>
> I've reattached Guy's patch for review.
>
> Guy, bug me in a week or so if I haven't committed it, and I will...
>
> Thanks for tracking this down.
>
> --
>   John-Mark Gurney                              Voice: +1 415 225 5579
>
>      "All that I will do, has been done, All that I have, has not."

Hi John,

Should I still file a PR?

1. I noticed my patch causes a style bug with the rbuf line now taking
87 columns.
2. Since the union now has a ng_mesg struct, the casting to resp can
be skipped and the union member used directly.

Attached new patch breaking the rbuf line and swapping resp usage with
res.res and &res.res
Maybe a different name than res should be used for the union or the member.

I only tested the new patch compiles for arm.armv6, haven't verified it.

Thanks,
Guy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sock-NgMkSockNode-2.patch
Type: application/octet-stream
Size: 1017 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-net/attachments/20131222/2db48a30/attachment.obj>