Re: [B.A.T.M.A.N.] [PATCH v4] batman-adv: Snoop DHCPACKs for DAT

[Sven Eckelmann <sven@narfation.org>]

[-- Attachment #1: Type: text/plain, Size: 4007 bytes --]

On Montag, 21. Mai 2018 12:49:23 CEST Linus Lüssing wrote:
[...]
> > And what about the basically duplicated bootp_pkt definition in net/ipv4/
> > ipconfig.c?
> 
> Would it make sense to first add it to distributed-arp-table.c and
> then later I'd make a second patch directly for netdev@ to unify those?

I think this could work.

On Montag, 21. Mai 2018 13:04:40 CEST Linus Lüssing wrote:

> Hm, I guess there's nothing I could do about that unaligned
> access other than suppressing any potential warnings? I guess
> since DHCP ACKs are relatively rare compared to other traffic in
> fast path I think the performance impact should be negligible.

No, it triggers the unaligned acccess exception and some architectures will 
not even be able to work around them [1].

> What was the magic command to check unaligned accesses during
> runtime again?

I think the debugfs infrastructure can be used to debug this [2]

    echo 2 >/sys/kernel/debug/mips/unaligned_action

neoraider used vxlan between outer ethernet header and batman-adv to 
"misalign" such fields by two bytes.

> With the preceding "#pragma pack(2)" any warnings should be
> suppressed already, shouldn't they?

No, you cast these fields to __be32 * and then access them. The pointer will 
therefore be accessed with the assumption that it is aligned correctly (4 
bytes boundary for MIPS). This will cause an exception on those machines.

Let me show you some examples. Let us first start with an direct access of a 
struct member:

    $MIPS_GCC/mips-openwrt-linux-musl-gcc -O1 -x c -S - -o - << EOF
    #include <stdint.h>
    struct test {
            uint32_t test1;
            uint32_t test2;
    };
    
    uint32_t test(struct test *t)
    {
            return t->test2;
    }
    EOF

You will get a simple function with a single (32 bit) load which also requires 
the 4 byte alignment:

    test:
            .frame  $sp,0,$31
            .mask   0x00000000,0
            .fmask  0x00000000,0
            .set    noreorder
            .set    nomacro
            lw      $2,4($4)
            j       $31
            nop


Now let us add the pragma which to reduce it to a 2 byte alignment 
requirement:

    $MIPS_GCC/mips-openwrt-linux-musl-gcc -O1 -x c -S - -o - << EOF
    #include <stdint.h>
    #pragma pack(2)
    struct test {
            uint32_t test1;
            uint32_t test2;
    };
    #pragma pack()
    
    uint32_t test(struct test *t)
    {
            return t->test2;
    }
    EOF

This will create two different loads to get the complete 32 bit word when only 
2 byte alignment is guaranteed

    test:
            .frame  $sp,0,$31
            .mask   0x00000000,0
            .fmask  0x00000000,0
            .set    noreorder
            .set    nomacro
            lwl     $2,4($4)
            nop
            lwr     $2,7($4)
            j       $31
            nop

And now we convert all this to your method of indirect access (uint32_t 
pointer):

    $MIPS_GCC/mips-openwrt-linux-musl-gcc -O1 -x c -S - -o - << EOF
    #include <stdint.h>
    #pragma pack(2)
    struct test {
            uint32_t test1;
            uint32_t test2;
    };
    #pragma pack()
    
    uint32_t test(struct test *t)
    {
            uint32_t *t2 = &t->test2;
            return *t2;
    }
    EOF

You will see that this will again only be a single 32 bit load which requires 
4 byte alignment (and thus will cause an CPU exception when it is only 2 byte 
aligned):

    test:
            .frame  $sp,0,$31
            .mask   0x00000000,0
            .fmask  0x00000000,0
            .set    noreorder
            .set    nomacro
            lw      $2,4($4)
            j       $31
            nop

Kind regards,
	Sven

[1] https://www.kernel.org/doc/Documentation/unaligned-memory-access.txt
[2] https://www.linux-mips.org/wiki/Alignment#Transparent_fixing_by_the_kernel

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]