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 19:05:39 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

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

...in every case, actually.

> 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=passt

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

No problem! I will also try to spend a moment and see if there's some
reasonable solution I can suggest. Thanks,

-- 
Stefano