Re: [PATCH v2 6/8] checksum: use csum_ip4_header() in udp.c and tcp.c

On 2/16/24 15:54, Stefano Brivio wrote:
...
  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:
  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)))). 
 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.
 
So I think in the worst case iph is aligned on 2.

Do you know which architectures don't support this alignment?

Do you know if we will support this architecture?

I 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.

Thanks,
Laurent