On Thu, 29 Feb 2024 09:56:25 +0100 Stefano Brivio <sbrivio(a)redhat.com> wrote:On Thu, 29 Feb 2024 19:49:09 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:...no, that's not the case. Dereferencing 'iph' from struct tcp[46]_l2_buf_t is fine: struct tcp4_l2_buf_t { uint8_t pad[2]; /* 0 2 */ struct tap_hdr taph; /* 2 18 */ struct iphdr iph; /* 20 20 */ [...] } __attribute__((__packed__)); struct tcp6_l2_buf_t { uint8_t pad[2]; /* 0 2 */ struct tap_hdr taph; /* 2 18 */ struct ipv6hdr ip6h; /* 20 40 */ [...] } __attribute__((__packed__)); The problematic structures are the UDP buffers: struct udp4_l2_buf_t { struct sockaddr_in s_in; /* 0 16 */ struct tap_hdr taph; /* 16 18 */ struct iphdr iph; /* 34 20 */ [...] } __attribute__((__aligned__(4))); and for UDP, this patch is dereferencing buffer pointers only, not pointers to headers. Inconsistent if you want, but it's quite simple and it works, plus if you had half a mind (at least for UDP) to split buffers into header and payloads iovec parts... this doesn't need to be exceedingly elegant. -- StefanoOn Thu, Feb 29, 2024 at 08:05:09AM +0100, Stefano Brivio wrote:Oh, oops, I didn't realise this was the case (I haven't reviewed the patch yet).On Thu, 29 Feb 2024 11:38:53 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:Right, that's kind of what I'm getting at. Assuming this value starts in an unaligned buffer, then in order to pass this by value the caller will need to load from that unaligned pointer. AFAIK, the compiler will base the type of loads only on the pointed to type, which isn't changed whether we dereference in the caller or the callee.On Wed, Feb 28, 2024 at 02:26:18PM +0100, Laurent Vivier wrote: > On 2/19/24 04:08, David Gibson wrote: > > On Sat, Feb 17, 2024 at 04:07:23PM +0100, Laurent Vivier wrote: > > > > [...] > > > > > +/** > > > + * proto_ipv6_header_psum() - Calculates the partial checksum of an > > > + * IPv6 header for UDP or TCP > > > + * @payload_len: Payload length > > > + * @proto: Protocol number > > > + * @saddr: Source address > > > + * @daddr: Destination address > > > + * Returns: Partial checksum of the IPv6 header > > > + */ > > > +uint32_t proto_ipv6_header_psum(uint16_t payload_len, uint8_t protocol, > > > + struct in6_addr saddr, struct in6_addr daddr) > > > > Hrm, this is passing 2 16-byte IPv6 addresses by value, which might > > not be what we want. > > The idea here is to avoid the pointer alignment problem (&ip6h->saddr and > &ip6h->daddr can be misaligned). Ah, right. That's a neat idea, but I'm not sure it really helps: I think it will just move the misaligned access from inside the function to the call site, where we try to marshal the parameter from something unaligned.I haven't tested this yet, but note that this is generally okay: the problem is *dereferencing* an unaligned pointer. But if you load memory from an aligned pointer, and extract a value from this memory, it's all fine.Speaking MIPS, this is not safe on all CPU models: la $1, 1002 # s1 now contains the value 1002 lw $2, 0($1) # load word from memory at 1002 + 0 into s2 but this is: la $1, 1000 # s1 now contains the value 1000 la $2, 1004 # s3 now contains the value 1004 lw $3, 0($1) # load word from memory at 1000 + 0 into s3 lw $4, 0($3) # load word from memory at 1004 + 0 into s4 sll $5, $3, 16 # 16-bit shift left s3 into s5 srl $6, $4, 16 # 16-bit shift right s4 into s6 or $2, $5, $6 # OR s5 and s6 into s2Right, but I don't think merely moving the dereference to the caller will necessarily induce the compiler to generate this rather than the former.