On Fri, 16 Feb 2024 19:05:39 +0100 Laurent Vivier <lvivier(a)redhat.com> wrote:On 2/16/24 15:54, Stefano Brivio wrote:...in every case, actually.On Fri, 16 Feb 2024 15:17:13 +0100 Laurent Vivier <lvivier(a)redhat.com> wrote:So I think in the worst case iph is aligned on 2.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.On Wed, 14 Feb 2024 09:56:26 +0100 Laurent Vivier <lvivier(a)redhat.com> wrote: > ... > /** > * 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.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)))).Do you know which architectures don't support this alignment?I couldn't find a table, from experience / memory it's not a good idea to do this especially on several MIPS flavours and 32-bit ARM. From a kernel tree: $ grep -rn "select HAVE_EFFICIENT_UNALIGNED_ACCESS" arch/ arch/arc/Kconfig:352: select HAVE_EFFICIENT_UNALIGNED_ACCESS arch/x86/Kconfig:216: select HAVE_EFFICIENT_UNALIGNED_ACCESS arch/arm64/Kconfig:204: select HAVE_EFFICIENT_UNALIGNED_ACCESS arch/s390/Kconfig:174: select HAVE_EFFICIENT_UNALIGNED_ACCESS arch/loongarch/Kconfig:114: select HAVE_EFFICIENT_UNALIGNED_ACCESS if !ARCH_STRICT_ALIGN arch/powerpc/Kconfig:237: select HAVE_EFFICIENT_UNALIGNED_ACCESS arch/m68k/Kconfig:30: select HAVE_EFFICIENT_UNALIGNED_ACCESS if !CPU_HAS_NO_UNALIGNED arch/arm/Kconfig:98: select HAVE_EFFICIENT_UNALIGNED_ACCESS if (CPU_V6 || CPU_V6K || CPU_V7) && MMU these are the architectures on which, at least under some conditions or on some CPUs, unaligned access are generally okay. It could be problematic on everything else (again, from my experience, it will actually be).Do you know if we will support this architecture?I think we should try to be nice to all architectures currently supported by the Linux kernel. We have some tests for a number of architectures (currently disabled, but I give some a run from time to time). And Debian packages are built for these architectures: https://buildd.debian.org/status/package.php?p=passtI think I will send the v3 of my series without fixing that because I don't have enough time this week. I will address the problem later.No problem! I will also try to spend a moment and see if there's some reasonable solution I can suggest. Thanks, -- Stefano