On Fri, 16 Feb 2024 15:17:13 +0100 Laurent Vivier <lvivier(a)redhat.com> wrote:On 2/16/24 10:08, Stefano Brivio wrote:That's because of the size of struct tap_hdr (18 bytes). On, at least, x86_64, armhf, and i686: $ pahole passt [...] struct udp4_l2_buf_t { struct sockaddr_in s_in; /* 0 16 */ struct tap_hdr taph; /* 16 18 */ struct iphdr iph; /* 34 20 */ [...] ...we could align the start of 'taph' by adding 2 bytes of padding before it, note that the size of struct sockaddr_in doesn't depend on the architecture. But then you can't dereference 'taph', which is probably even worse. -- StefanoOn Wed, 14 Feb 2024 09:56:26 +0100 Laurent Vivier <lvivier(a)redhat.com> wrote:I don't understand how &b->iph cannot be aligned as b should be aligned and b is defined using udp4_l2_buf_t structure with _attribute__ ((packed, aligned(__alignof__(unsigned int)))).... /** * udp_update_l2_buf() - Update L2 buffers with Ethernet and IPv4 addresses * @eth_d: Ethernet destination address, NULL if unchanged @@ -579,6 +562,9 @@ static void udp_splice_sendfrom(const struct ctx *c, unsigned start, unsigned n, * * Return: size of tap frame with headers */ +#pragma GCC diagnostic push +/* ignore unaligned pointer value warning for &b->iph */ +#pragma GCC diagnostic ignored "-Waddress-of-packed-member" static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport, const struct timespec *now) { @@ -614,13 +600,14 @@ static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport, b->iph.saddr = b->s_in.sin_addr.s_addr; } - udp_update_check4(b); + b->iph.check = csum_ip4_header(&b->iph);Similar comment as I had on v1: I don't think this is safe. If &b->iph is, say, 0x2000, it's all fine: when csum_ip4_header() needs to access, say, ip4h->tot_len, it will dereference 0x2000 and look at 16 bits, 2 bytes into it. If &b->iph is 0x2001, though, csum_ip4_header() will dereference 0x2001 and, on some architectures, boom.