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

[Laurent Vivier <lvivier@redhat.com>]

On 2/16/24 15:54, Stefano Brivio wrote:
> On Fri, 16 Feb 2024 15:17:13 +0100
> Laurent Vivier <lvivier@redhat.com> wrote:
>
>> On 2/16/24 10:08, Stefano Brivio wrote:
>>> On Wed, 14 Feb 2024 09:56:26 +0100
>>> Laurent Vivier <lvivier@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