Re: [PATCH] udp: create and send ICMPv4 to local peer when applicable

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 13 Feb 2025 14:07:22 -0500
Jon Maloy <jmaloy@redhat.com> wrote:

> On 2025-02-12 16:39, Jon Maloy wrote:
> > On 2025-02-10 19:55, David Gibson wrote:  
> >> On Sun, Feb 09, 2025 at 12:00:56PM -0500, Jon Maloy wrote:  
> >>> +    const struct flowside *toside = flowside_at_sidx(tosidx);
> >>> +    struct in_addr oaddr = toside->oaddr.v4mapped.a4;
> >>> +    struct in_addr eaddr = toside->eaddr.v4mapped.a4;
> >>> +    struct iov_tail data = IOV_TAIL(iov, 1, 0);
> >>> +    struct {
> >>> +        struct icmphdr icmp4h;
> >>> +        struct iphdr ip4h;
> >>> +        struct udphdr uh;
> >>> +        char data[8];
> >>> +    } msg;  
> >>
> >> Needs a ((packed)) attribute to ensure we don't get compiler padding.  
> 
> I get
> udp.c:437:23: warning: taking address of packed member of ‘struct 
> <anonymous>’ may result in an unaligned pointer value 
> [-Waddress-of-packed-member]
>    437 |         tap_push_ip4h(&msg.ip4h, eaddr, oaddr, udplen, 
> IPPROTO_UDP);
> 
> Also, the sizes of these members are 8+20+8+8, so I cannot see how
> this struct can possibly be packed or how there can be an unaligned
> pointer.

Unaligned pointer because it's on the stack, and it can happily be at a
semi-random point of it. Packed needs to be packed because you could
have 8 + 20 + ("oops we are at 28 let's make it 32") 4 + 8 + 8.

> In short: ((packed)) seems unnecessary, and only causes problems.

...well, but it still should be ((packed)) because it goes on the wire,
and ((aligned)) because you need to access the single fields:

  __attribute__((packed, aligned(__alignof__(max_align_t))));

meaning: align it to whatever any kind of machine might possibly need.

-- 
Stefano