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

[Stefano Brivio <sbrivio@redhat.com>]

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.

-- 
Stefano