From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTP id 0002B5A0280 for ; Fri, 16 Feb 2024 19:25:21 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1708107921; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=rj8qNDMRKK3qnX3XyZWGD73dBGvkcZ/oB4NLNVjix1Y=; b=TkU51Y4s6+iDybZZWEC8hM+NS35rwROIY11SPT4yzKilS1LmWRmS4sKNnrPM8P6C/dA1pU PBwX6SF8ZmBKTYynH0pDmDexkM5pWovj2Bg9YgsU94/aHoPKRomydw7FnAZvfmwsxIBE/e 5//i+FrjLpF5gVT3c/QhZoacoErSufI= Received: from mail-ej1-f72.google.com (mail-ej1-f72.google.com [209.85.218.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-531-GVhCzaZsMAe8rvhVmU3QkA-1; Fri, 16 Feb 2024 13:25:19 -0500 X-MC-Unique: GVhCzaZsMAe8rvhVmU3QkA-1 Received: by mail-ej1-f72.google.com with SMTP id a640c23a62f3a-a3d38947c35so139037666b.3 for ; Fri, 16 Feb 2024 10:25:19 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708107917; x=1708712717; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=rj8qNDMRKK3qnX3XyZWGD73dBGvkcZ/oB4NLNVjix1Y=; b=Ur5cXXbG24CGErGyD5V88IJUZpwx098yaF/0x38GzhW7n4JVCn9sFyD9xOW/akmtn9 /c+0oJ3Ds4xw1zuHbozy7FBQ9WVb74YuQhFO+SUzcb/R1t/TN4KzQg0LySAahahYl43Z 5/2J7wcwufyo48WD89gc+VFdn4wFi8XT0/Xhfh8ptlkK9gQ0w8R0TIbXpOU9CqvNZbOo YFb3lWb+WZzcB2yHksc0BECedicyOstUzSuaWDAjA3PTsFk0kodZHkFHbNhnO51FhB1v riCZC96axe20f9scJCNUUDuVdso2Qcyv+RgKz+l4qONoM4S36rK/cgZdws/JHF8duI0L hmyA== X-Gm-Message-State: AOJu0YzFDl38/pkAxMg8proE3CMNjiV4tFjvlqaxoImukVhA2tMhf7mM OUAp2OLteB6yXgc1IL+mJ3VhbmzBVpXMi67ZFPdruKYTAa5d1DQLXOnYq/LCV4xmhz3zPLsrTSR KRsiaNiPZPURi0qnhv+R4tWCDqI7UmEH9rVpo/66jOfM0h204WMo+qCVz6ZeywI9ez156xt/Fvo LVIhC3Aj/cbPFvnuAqFqCDtYqHTBizRZXBSac= X-Received: by 2002:a17:906:ad8b:b0:a3d:991c:5152 with SMTP id la11-20020a170906ad8b00b00a3d991c5152mr4179294ejb.59.1708107916975; Fri, 16 Feb 2024 10:25:16 -0800 (PST) X-Google-Smtp-Source: AGHT+IGoMwbo0mEEDHoVWRkayt4oxIxtfUROK5RjrMTZdIB8O3KsDtXF20PQaRzVNfbuQFJZzCpGpw== X-Received: by 2002:a17:906:ad8b:b0:a3d:991c:5152 with SMTP id la11-20020a170906ad8b00b00a3d991c5152mr4179279ejb.59.1708107916550; Fri, 16 Feb 2024 10:25:16 -0800 (PST) Received: from maya.cloud.tilaa.com (maya.cloud.tilaa.com. [164.138.29.33]) by smtp.gmail.com with ESMTPSA id jg28-20020a170907971c00b00a3d346d5b0fsm214843ejc.42.2024.02.16.10.25.15 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Fri, 16 Feb 2024 10:25:15 -0800 (PST) Date: Fri, 16 Feb 2024 19:24:40 +0100 From: Stefano Brivio To: Laurent Vivier Subject: Re: [PATCH v2 6/8] checksum: use csum_ip4_header() in udp.c and tcp.c Message-ID: <20240216192440.6031e2d2@elisabeth> In-Reply-To: <7a9915cf-f004-453a-b328-80d086c14a80@redhat.com> References: <20240214085628.210783-1-lvivier@redhat.com> <20240214085628.210783-7-lvivier@redhat.com> <20240216100805.040826b3@elisabeth> <53d4a403-0d3f-4aa0-b980-27c2026a468b@redhat.com> <20240216155400.1d26ae17@elisabeth> <7a9915cf-f004-453a-b328-80d086c14a80@redhat.com> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.36; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: BQNXFBVTT6D536D6WWL37YPB6POVTB2H X-Message-ID-Hash: BQNXFBVTT6D536D6WWL37YPB6POVTB2H X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Fri, 16 Feb 2024 19:05:39 +0100 Laurent Vivier wrote: > On 2/16/24 15:54, Stefano Brivio wrote: > > On Fri, 16 Feb 2024 15:17:13 +0100 > > Laurent Vivier wrote: > > > >> On 2/16/24 10:08, Stefano Brivio wrote: > >>> On Wed, 14 Feb 2024 09:56:26 +0100 > >>> Laurent Vivier 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