Message ID | 20240207-fix_sparse_errors_checksum_tests-v6-0-4caa9629705b@rivosinc.com |
---|---|
Headers |
Return-Path: <linux-kernel+bounces-57337-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7301:168b:b0:106:860b:bbdd with SMTP id ma11csp2590294dyb; Wed, 7 Feb 2024 16:23:57 -0800 (PST) X-Forwarded-Encrypted: i=3; AJvYcCXq5HPNHLEmMZA/q2DtCI3U+ptZ+Xn/rE3AuKhLykUqQEHRlGHacJ8nZ1J/Sw1VWCdGQpq7Ygh0x4UHraXkJBb/4yWqkQ== X-Google-Smtp-Source: AGHT+IFhfO5ybrdDv4rx8gkze4mgkCU8b786d0i0M4vR3qSduKyopFRq6JEHmmyRmAI+R9bPm215 X-Received: by 2002:a17:906:dfe8:b0:a3a:510f:97c0 with SMTP id lc8-20020a170906dfe800b00a3a510f97c0mr333969ejc.61.1707351837085; Wed, 07 Feb 2024 16:23:57 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1707351837; cv=pass; d=google.com; s=arc-20160816; b=ikZWl0FgLZzxWu+bpdytctvfEHbsDxsk4WmeoUVn3Q4Iu630ni/w0JDKAI5Te3UHfb rVPuxRf23M+dJTFKyRsHdaR3chYvy7Ne7wI079mLIuUPoV//SJuWdVO+webVrmSAD1Gs YaRqhgOleE50b1d6KDWw8k20N2qPVbwgk49yPXxC1rxYtGxmcZ206+B0XSQqym7pskoX D+J4HusnWMS0L5W0ubCPrbMAapxcNKjnYvZK4reBfWUyOHfJ8iiK2YqNuVR4tBw3P1DY R7U0rP9LwZ+a1SPEPOAt4KzKaMv7fQPeSinKf+WVzs50rlMIqcI92PzZh3DoRNZ15e3t umJg== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=cc:to:content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:message-id:date:subject:from :dkim-signature; bh=iGy7BRl7kPJKYeaAvt+TgLXvSAxcG9Nq9NPnIaMKfaI=; fh=mzeOH7Mol7vHvkHaCofLhc9N8s7p5MNd8fxB/Pxwu4o=; b=D3DoNRCaEoLJZ0QFMW70JCVDpHTsoKH/35eDNiVM6OcrljBaP61NEbTmv8gqLn/DFs EgUmk/6IXt5xN/EmRqzsl3/5hFjJ5t//+QBtWja0AQj37r0pPohOYgccKrzw/TyibHrQ HVR/7XNlgMnEpBWK1uqLHlDurmjeog4LneBMWsyzX+gSmxhZI9/00vD36Dbi37d2JE7b LXaNazolp6G5q8fLDG/7turrRWA+25mgc6eSKGCp6otuLe4M/RiTtJr6ppTLisQI4bJv EzvQfF5BNkWXhIp9+2su1C2JaY0kGI+AFRpZvmHfXD868KfxFyW8ppM23xcjNtgbYyWP hcNg==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@rivosinc-com.20230601.gappssmtp.com header.s=20230601 header.b=zcuTJ8H3; arc=pass (i=1 spf=pass spfdomain=rivosinc.com dkim=pass dkdomain=rivosinc-com.20230601.gappssmtp.com); spf=pass (google.com: domain of linux-kernel+bounces-57337-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) smtp.mailfrom="linux-kernel+bounces-57337-ouuuleilei=gmail.com@vger.kernel.org" X-Forwarded-Encrypted: i=2; AJvYcCV/H9HUl15FZxk7pnFXHRIX6gA8p8IF2S1T28rrgouzaUU7GhFSLUR69XKS50E0ifD3sRSf37yieQa1h8fEBUxLLPdJxQ== Received: from am.mirrors.kernel.org (am.mirrors.kernel.org. [147.75.80.249]) by mx.google.com with ESMTPS id s21-20020a1709060c1500b00a38273c1419si1469265ejf.699.2024.02.07.16.23.56 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Feb 2024 16:23:57 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-57337-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249; Authentication-Results: mx.google.com; dkim=pass header.i=@rivosinc-com.20230601.gappssmtp.com header.s=20230601 header.b=zcuTJ8H3; arc=pass (i=1 spf=pass spfdomain=rivosinc.com dkim=pass dkdomain=rivosinc-com.20230601.gappssmtp.com); spf=pass (google.com: domain of linux-kernel+bounces-57337-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) smtp.mailfrom="linux-kernel+bounces-57337-ouuuleilei=gmail.com@vger.kernel.org" Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by am.mirrors.kernel.org (Postfix) with ESMTPS id 9CCB31F22D8C for <ouuuleilei@gmail.com>; Thu, 8 Feb 2024 00:23:56 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id F3F263611A; Thu, 8 Feb 2024 00:22:57 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=rivosinc-com.20230601.gappssmtp.com header.i=@rivosinc-com.20230601.gappssmtp.com header.b="zcuTJ8H3" Received: from mail-pg1-f175.google.com (mail-pg1-f175.google.com [209.85.215.175]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 28D762C857 for <linux-kernel@vger.kernel.org>; Thu, 8 Feb 2024 00:22:52 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.215.175 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1707351775; cv=none; b=tSD4K9jLccGpcCChEqTG16l4Qrr8/ePc3+57o4JADLfBnSJfyM58THhLy1zN22qUc1CljmJp1rnK0hwP1cEUp+yZG425nrwiXoIBWgVrxS2dmGw6BNV9eitKxHdzVfY3I0HL8DbDPg3mXxYv1YESbqfrdWL8uK8gocznQwsUlJA= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1707351775; c=relaxed/simple; bh=3a1VyrDlpdg9fZq+LBjebNs62E5K5JdZfIQyohmyq3A=; h=From:Subject:Date:Message-Id:MIME-Version:Content-Type:To:Cc; b=h6XUuvqPcY6w2lIC8tOUXZZViA8/j/n5Ggb/3Ha0CZ++9B475ZOvdhJg9u+F+ddBr9klUyj+gqrvZ3BCOQGjy5rbDM2BQvURvB5Z6kuHtm6iNrvOnjM31dHSoaKfquDByl1zakHs9RNUotg+ivHXbMs+Xc0MDmGD/46PwZXjJp4= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=rivosinc.com; spf=pass smtp.mailfrom=rivosinc.com; dkim=pass (2048-bit key) header.d=rivosinc-com.20230601.gappssmtp.com header.i=@rivosinc-com.20230601.gappssmtp.com header.b=zcuTJ8H3; arc=none smtp.client-ip=209.85.215.175 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=rivosinc.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=rivosinc.com Received: by mail-pg1-f175.google.com with SMTP id 41be03b00d2f7-5dbcfa0eb5dso1152684a12.3 for <linux-kernel@vger.kernel.org>; Wed, 07 Feb 2024 16:22:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rivosinc-com.20230601.gappssmtp.com; s=20230601; t=1707351772; x=1707956572; darn=vger.kernel.org; h=cc:to:content-transfer-encoding:mime-version:message-id:date :subject:from:from:to:cc:subject:date:message-id:reply-to; bh=iGy7BRl7kPJKYeaAvt+TgLXvSAxcG9Nq9NPnIaMKfaI=; b=zcuTJ8H3o63n+oW9j9etHNo9MAT8JaGz+XGRxzx5loy11nYCrsX2B1uGjWeZyHteVx HU+B+PRZcTzlSKTOOXC53qk4S/UOjnMlBJlZvvN0BD2cDSKdLJ3b9mpkICk0cZLn+U+g M+AW/7sp0srXuI3ft6ZGh7LuNRfzX4lGH6A96OmRZUrWSp1mI+Q1vNJs8cDhtiRt7RMi O6b8hIxR+4PHbZGiYOFWtjR9Nr6++/xo4GrQYLVYEs5tK3ooBR0CFP84ObfJ44wM9fRV 1yiUN9pCRZQybHsaIRm5DA0rjYHMgDk+hVyKvbo9G6qr4ace2VQsrhuAYFVsVTwoghmU k6Yg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707351772; x=1707956572; h=cc:to:content-transfer-encoding:mime-version:message-id:date :subject:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=iGy7BRl7kPJKYeaAvt+TgLXvSAxcG9Nq9NPnIaMKfaI=; b=tnQTgZuGeR6r8avWofvbKfcVxQZXo16D/GSgWqPwdZvCmQpMy1/ibr/8O6m6180VNq RNKvkKUSaJJQeOzoz7SG7GBqV+YCVJ13tnRx98kbziMeKJuAgIG35xdAEWUmHRRUSkW5 rfSViq1wg3D2VgIMLjRgxCCp+rjdAH5smcH07UCEyoeHWIRBiLmbbgHtBDNmOw697cDW jkiiu7HxuG2oEllPfSWd8AQbMqzhaijxLakWMJJlpL1Xw80Bj3cWVOtYYCRp1igvBwGy rYxHbpCFDCviICoYL5jyVqhm8qZq27H2TsZxtMUMdfweN34XazRjZMIIBQTkiRSq3VIy AjoA== X-Gm-Message-State: AOJu0YysURsWD6TYO6TqSZuW7+d34ClC6hDylkEakV+Z0782fWJ7DykZ QUBma0NvRFrpqyCBPgii2H2JRgIhVusHqyMi2TqDiMwxB6aBw7N2+wECbPkDeJoZaxpPosWP0Fu t X-Received: by 2002:a05:6a20:e113:b0:19e:48e7:e664 with SMTP id kr19-20020a056a20e11300b0019e48e7e664mr6974827pzb.31.1707351772103; Wed, 07 Feb 2024 16:22:52 -0800 (PST) X-Forwarded-Encrypted: i=1; AJvYcCV11Zo3sUCJMWPgya8d1jog0P15+VyUxaj/KmMlKoiKTEMcra3+QrtE0JGKijQFjRUDII/dYJ1RALGMAg+FfF5JSZCnkYuF22JIw4wf0iaFpi2aiyy0A9P7OxKGkY4D0gxczGpySs2wEvfht5bbpyU4hCCjT1tHMWwwJJzU4l/J2wAF6S8zcEYG+WsR+R3xu7KXeUWHq1a2eNs6PA== Received: from charlie.ba.rivosinc.com ([64.71.180.162]) by smtp.gmail.com with ESMTPSA id g13-20020aa79dcd000000b006e03a640007sm2272630pfq.71.2024.02.07.16.22.51 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Feb 2024 16:22:51 -0800 (PST) From: Charlie Jenkins <charlie@rivosinc.com> Subject: [PATCH v6 0/2] lib: checksum: Fix issues with checksum tests Date: Wed, 07 Feb 2024 16:22:49 -0800 Message-Id: <20240207-fix_sparse_errors_checksum_tests-v6-0-4caa9629705b@rivosinc.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-B4-Tracking: v=1; b=H4sIANkexGUC/5XOQW6DMBAF0KtEXteV8djGZNV7VBEy9lCsKhB5i NUq4u41WaGKBVn+L83782CEKSKx8+nBEuZIcRpLMG8n5gc3fiGPoWQmhVSiqhrex5+Wbi4Rtpj SlKj1A/pvul/bGWkmLk1nTQcq1FaxwtwSlpvnxOel5CHSPKXf52Ku1vYFPFe84jJA6K0Jtkx8p JgniqN/99OVrX6WG1PCAVNywTsQpjMe6+D0jgmvmlBM7NH7um+UbOodU21NdcBU659edjZIcKD 9jqk3JogDpi6mCtYJB1Cjxn/msix/VgL6WyMCAAA= To: Guenter Roeck <linux@roeck-us.net>, David Laight <David.Laight@aculab.com>, Palmer Dabbelt <palmer@dabbelt.com>, Andrew Morton <akpm@linux-foundation.org> Cc: linux-kernel@vger.kernel.org, Charlie Jenkins <charlie@rivosinc.com>, kernel test robot <lkp@intel.com> X-Mailer: b4 0.12.3 X-Developer-Signature: v=1; a=ed25519-sha256; t=1707351771; l=1496; i=charlie@rivosinc.com; s=20231120; h=from:subject:message-id; bh=3a1VyrDlpdg9fZq+LBjebNs62E5K5JdZfIQyohmyq3A=; b=0I9ryIC5Ga5w8HPBbIqElbjVp+XA6/fjwe70Gsazr9tqCITHUm8cwHlsBi5qQ4lTpxhVcPdbY 5modatIdWBIDil1HWcRJdQTcat8ii7hJJ6jR2iap/A/7AfBebQqGxGp X-Developer-Key: i=charlie@rivosinc.com; a=ed25519; pk=t4RSWpMV1q5lf/NWIeR9z58bcje60/dbtxxmoSfBEcs= X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1790288159815219089 X-GMAIL-MSGID: 1790288159815219089 |
Series |
lib: checksum: Fix issues with checksum tests
|
|
Message
Charlie Jenkins
Feb. 8, 2024, 12:22 a.m. UTC
The ip_fast_csum and csum_ipv6_magic tests did not have the data
types properly casted, and improperly misaligned data.
Signed-off-by: Charlie Jenkins <charlie@rivosinc.com>
---
Changes in v6:
- Fix for big endian (Guenter)
- Link to v5: https://lore.kernel.org/r/20240130-fix_sparse_errors_checksum_tests-v5-0-4d8a0a337e5e@rivosinc.com
Changes in v5:
- Add Guenter's tested-by
- CC Andrew Morton
- Link to v4: https://lore.kernel.org/r/20240124-fix_sparse_errors_checksum_tests-v4-0-bc2b8d23a35c@rivosinc.com
Changes in v4:
- Pad test values with zeroes (David)
- Link to v3: https://lore.kernel.org/r/20240123-fix_sparse_errors_checksum_tests-v3-0-efecc7f94297@rivosinc.com
Changes in v3:
- Don't read memory out of bounds
- Link to v2: https://lore.kernel.org/r/20240123-fix_sparse_errors_checksum_tests-v2-0-b306b6ce7da5@rivosinc.com
Changes in v2:
- Add additional patch to fix alignment issues
- Link to v1: https://lore.kernel.org/r/20240119-fix_sparse_errors_checksum_tests-v1-1-2d3df86d8d78@rivosinc.com
---
Charlie Jenkins (2):
lib: checksum: Fix type casting in checksum kunits
lib: checksum: Use aligned accesses for ip_fast_csum and csum_ipv6_magic tests
lib/checksum_kunit.c | 409 +++++++++++++++++++--------------------------------
1 file changed, 149 insertions(+), 260 deletions(-)
---
base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
change-id: 20240119-fix_sparse_errors_checksum_tests-26b86b34d784
Comments
On Wed, 07 Feb 2024 16:22:49 -0800 Charlie Jenkins <charlie@rivosinc.com> wrote: > The ip_fast_csum and csum_ipv6_magic tests did not have the data > types properly casted, and improperly misaligned data. Neither this nor the two patch's changelogs describe *why* these changes are needed. They merely assert that we need to do this. IOW, when fixing a bug please always describe the user-visible effects of that bug.
On Wed, Feb 07, 2024 at 05:45:22PM -0800, Andrew Morton wrote: > On Wed, 07 Feb 2024 16:22:49 -0800 Charlie Jenkins <charlie@rivosinc.com> wrote: > > > The ip_fast_csum and csum_ipv6_magic tests did not have the data > > types properly casted, and improperly misaligned data. > > Neither this nor the two patch's changelogs describe *why* these changes > are needed. They merely assert that we need to do this. > > IOW, when fixing a bug please always describe the user-visible effects > of that bug. > I can add a description that says that the tests are being fixed because they caused failures on systems without misaligned access support. As for the casting patch it's a bit less pertinent but I can add that it allows the code to pass sparse checks. - Charlie
On 2/7/24 18:09, Charlie Jenkins wrote: > On Wed, Feb 07, 2024 at 05:45:22PM -0800, Andrew Morton wrote: >> On Wed, 07 Feb 2024 16:22:49 -0800 Charlie Jenkins <charlie@rivosinc.com> wrote: >> >>> The ip_fast_csum and csum_ipv6_magic tests did not have the data >>> types properly casted, and improperly misaligned data. >> >> Neither this nor the two patch's changelogs describe *why* these changes >> are needed. They merely assert that we need to do this. >> >> IOW, when fixing a bug please always describe the user-visible effects >> of that bug. >> > > I can add a description that says that the tests are being fixed because > they caused failures on systems without misaligned access support. As > for the casting patch it's a bit less pertinent but I can add that it > allows the code to pass sparse checks. > > - Charlie > I don't know exactly what Andrew is asking for, but maybe including the error log from the failed selftests and/or the sparse errors would be sufficient ? Not sure though if any of those count as "user visible". Guenter
On Wed, 07 Feb 2024 18:44:42 PST (-0800), linux@roeck-us.net wrote: > On 2/7/24 18:09, Charlie Jenkins wrote: >> On Wed, Feb 07, 2024 at 05:45:22PM -0800, Andrew Morton wrote: >>> On Wed, 07 Feb 2024 16:22:49 -0800 Charlie Jenkins <charlie@rivosinc.com> wrote: >>> >>>> The ip_fast_csum and csum_ipv6_magic tests did not have the data >>>> types properly casted, and improperly misaligned data. >>> >>> Neither this nor the two patch's changelogs describe *why* these changes >>> are needed. They merely assert that we need to do this. >>> >>> IOW, when fixing a bug please always describe the user-visible effects >>> of that bug. >>> >> >> I can add a description that says that the tests are being fixed because >> they caused failures on systems without misaligned access support. As >> for the casting patch it's a bit less pertinent but I can add that it >> allows the code to pass sparse checks. >> >> - Charlie >> > I don't know exactly what Andrew is asking for, but maybe including the > error log from the failed selftests and/or the sparse errors would be > sufficient ? > > Not sure though if any of those count as "user visible". Ya, for compiler warning/error workarounds I usually just include something like "without this, I get $ERROR". Something like 28ea54bade76 ("RISC-V: Don't rely on positional structure initialization"). For the aligned access on there was a fairly interesting discussion on why this hadn't tripped up before, I forget if it was on LKML or IRC (or Slack or just in the office). That's worth including...
On Wed, Feb 07, 2024 at 04:22:49PM -0800, Charlie Jenkins wrote: > The ip_fast_csum and csum_ipv6_magic tests did not have the data > types properly casted, and improperly misaligned data. > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> I still get: Failed unit tests: mips:malta:checksum mips64:malta:checksum mipsel:malta:checksum mipsel64:malta:checksum parisc:B160L:checksum parisc:C3700:checksum parisc64:C3700:checksum sh:rts7751r2dplus_defconfig:checksum on parisc/parisc64: # test_ip_fast_csum: ASSERTION FAILED at lib/checksum_kunit.c:463 Expected ( u64)csum_result == ( u64)expected, but ( u64)csum_result == 33754 (0x83da) ( u64)expected == 10946 (0x2ac2) not ok 4 test_ip_fast_csum ok 5 test_csum_ipv6_magic # checksum: pass:4 fail:1 skip:0 total:5 # Totals: pass:4 fail:1 skip:0 total:5 mipsel/mipsel64 (little endian): # test_csum_ipv6_magic: ASSERTION FAILED at lib/checksum_kunit.c:506 Expected ( u64)csum_result == ( u64)expected, but ( u64)csum_result == 18588 (0x489c) ( u64)expected == 12357 (0x3045) not ok 5 test_csum_ipv6_magic # checksum: pass:4 fail:1 skip:0 total:5 # Totals: pass:4 fail:1 skip:0 total:5 mips (big endian): # test_csum_ipv6_magic: ASSERTION FAILED at lib/checksum_kunit.c:506 Expected ( u64)csum_result == ( u64)expected, but ( u64)csum_result == 59728 (0xe950) ( u64)expected == 12357 (0x3045) not ok 5 test_csum_ipv6_magic # checksum: pass:4 fail:1 skip:0 total:5 # Totals: pass:4 fail:1 skip:0 total:5 I noticed that csum_result varies across tests for some reason. On parisc/parisc64 the value is unexpected but always the same. sh (little endian; ok, this isn't entirely fair, this test wasn't enabled before): KTAP version 1 # Subtest: checksum # module: checksum_kunit 1..5 # test_csum_fixed_random_inputs: ASSERTION FAILED at lib/checksum_kunit.c:370 Expected ( u64)result == ( u64)expec, but ( u64)result == 53378 (0xd082) ( u64)expec == 33488 (0x82d0) not ok 1 test_csum_fixed_random_inputs # test_csum_all_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:395 Expected ( u64)result == ( u64)expec, but ( u64)result == 65281 (0xff01) ( u64)expec == 65280 (0xff00) not ok 2 test_csum_all_carry_inputs # test_csum_no_carry_inputs: ASSERTION FAILED at lib/checksum_kunit.c:443 Expected ( u64)result == ( u64)expec, but ( u64)result == 65535 (0xffff) ( u64)expec == 65534 (0xfffe) not ok 3 test_csum_no_carry_inputs ok 4 test_ip_fast_csum ok 5 test_csum_ipv6_magic # checksum: pass:2 fail:3 skip:0 total:5 # Totals: pass:2 fail:3 skip:0 total:5 The result/expected values are always the same in the sh4 tests. I'd take the test results for sh4 with a grain of salt; there is at least some possibility that this is an emulation problem. Guenter
Hi, On 2/7/24 16:22, Charlie Jenkins wrote: > The ip_fast_csum and csum_ipv6_magic tests did not have the data > types properly casted, and improperly misaligned data. > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> I sorted out most of the problems with this version, but I still get: # test_csum_ipv6_magic: ASSERTION FAILED at lib/checksum_kunit.c:513 Expected ( u64)csum_result == ( u64)expected, but ( u64)csum_result == 16630 (0x40f6) ( u64)expected == 65535 (0xffff) not ok 5 test_csum_ipv6_magic on m68k:q800. This is suspicious because there is no 0xffff in expected_csum_ipv6_magic[]. With some debugging information: ####### num_tests=86 i=84 expect array size=84 ####### MAX_LEN=512 WORD_ALIGNMENT=4 magic data size=42 That means the loop for (int i = 0; i < num_tests; i++) { ... expected = (__force __sum16)expected_csum_ipv6_magic[i]; ... } will access data beyond the end of the expected_csum_ipv6_magic[] array, possibly because m68k doesn't pad struct csum_ipv6_magic_data to 44 bytes. In this context, is the comment about proto having to be 0 really true ? It seems to me that the calculated checksum must be identical on both little and big endian systems. After all, they need to be able to talk to each other. Thanks, Guenter
On Sun, Feb 11, 2024 at 11:18:36AM -0800, Guenter Roeck wrote: > Hi, > > On 2/7/24 16:22, Charlie Jenkins wrote: > > The ip_fast_csum and csum_ipv6_magic tests did not have the data > > types properly casted, and improperly misaligned data. > > > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> > > I sorted out most of the problems with this version, but I still get: > > # test_csum_ipv6_magic: ASSERTION FAILED at lib/checksum_kunit.c:513 > Expected ( u64)csum_result == ( u64)expected, but > ( u64)csum_result == 16630 (0x40f6) > ( u64)expected == 65535 (0xffff) > not ok 5 test_csum_ipv6_magic > > on m68k:q800. This is suspicious because there is no 0xffff in > expected_csum_ipv6_magic[]. With some debugging information: > > ####### num_tests=86 i=84 expect array size=84 > ####### MAX_LEN=512 WORD_ALIGNMENT=4 magic data size=42 > > That means the loop > > for (int i = 0; i < num_tests; i++) { > ... > expected = (__force __sum16)expected_csum_ipv6_magic[i]; > ... > } > > will access data beyond the end of the expected_csum_ipv6_magic[] array, > possibly because m68k doesn't pad struct csum_ipv6_magic_data to 44 bytes. Okay I will check that out. > > In this context, is the comment about proto having to be 0 really true ? > It seems to me that the calculated checksum must be identical on both > little and big endian systems. After all, they need to be able to talk > to each other. I agree, but I couldn't find a solution other than setting it to zero. Maybe I am missing something simple... - Charlie > > Thanks, > Guenter >
Hi Günter, On Sun, Feb 11, 2024 at 8:18 PM Guenter Roeck <linux@roeck-us.net> wrote: > On 2/7/24 16:22, Charlie Jenkins wrote: > > The ip_fast_csum and csum_ipv6_magic tests did not have the data > > types properly casted, and improperly misaligned data. > > > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> > > I sorted out most of the problems with this version, but I still get: > > # test_csum_ipv6_magic: ASSERTION FAILED at lib/checksum_kunit.c:513 > Expected ( u64)csum_result == ( u64)expected, but > ( u64)csum_result == 16630 (0x40f6) > ( u64)expected == 65535 (0xffff) > not ok 5 test_csum_ipv6_magic > > on m68k:q800. This is suspicious because there is no 0xffff in > expected_csum_ipv6_magic[]. With some debugging information: > > ####### num_tests=86 i=84 expect array size=84 > ####### MAX_LEN=512 WORD_ALIGNMENT=4 magic data size=42 > > That means the loop > > for (int i = 0; i < num_tests; i++) { > ... > expected = (__force __sum16)expected_csum_ipv6_magic[i]; > ... > } > > will access data beyond the end of the expected_csum_ipv6_magic[] array, > possibly because m68k doesn't pad struct csum_ipv6_magic_data to 44 bytes. Exactly, sizeof(struct csum_ipv6_magic_data) = 42 on m68k. Hence struct csum_ipv6_magic_data needs an extra field "unsigned char pad[3];" at the end. Gr{oetje,eeting}s, Geert
On Mon, Feb 12, 2024 at 12:26:08AM -0500, Charlie Jenkins wrote: > On Sun, Feb 11, 2024 at 11:18:36AM -0800, Guenter Roeck wrote: > > Hi, > > > > On 2/7/24 16:22, Charlie Jenkins wrote: > > > The ip_fast_csum and csum_ipv6_magic tests did not have the data > > > types properly casted, and improperly misaligned data. > > > > > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> > > > > I sorted out most of the problems with this version, but I still get: > > > > # test_csum_ipv6_magic: ASSERTION FAILED at lib/checksum_kunit.c:513 > > Expected ( u64)csum_result == ( u64)expected, but > > ( u64)csum_result == 16630 (0x40f6) > > ( u64)expected == 65535 (0xffff) > > not ok 5 test_csum_ipv6_magic > > > > on m68k:q800. This is suspicious because there is no 0xffff in > > expected_csum_ipv6_magic[]. With some debugging information: > > > > ####### num_tests=86 i=84 expect array size=84 > > ####### MAX_LEN=512 WORD_ALIGNMENT=4 magic data size=42 > > > > That means the loop > > > > for (int i = 0; i < num_tests; i++) { > > ... > > expected = (__force __sum16)expected_csum_ipv6_magic[i]; > > ... > > } > > > > will access data beyond the end of the expected_csum_ipv6_magic[] array, > > possibly because m68k doesn't pad struct csum_ipv6_magic_data to 44 bytes. > > Okay I will check that out. > > > > > In this context, is the comment about proto having to be 0 really true ? > > It seems to me that the calculated checksum must be identical on both > > little and big endian systems. After all, they need to be able to talk > > to each other. > > I agree, but I couldn't find a solution other than setting it to zero. > Maybe I am missing something simple... > Try the patch below on top of yours. It should work on both big and little endian systems. Key changes: - use random_buf directly instead of copying anything - no need to convert source / destination addresses - csum in the buffer is in network byte order and needs to stay that way - len in the buffer is in network byte order and needs to be converted to host byte order since that is expected by csum_ipv6_magic() - the expected value is in host byte order and needs to be converted to network byte order for comparison - protocol is just fine and converted by csum_ipv6_magic() as needed Hope this helps, Guenter --- diff --git a/lib/checksum_kunit.c b/lib/checksum_kunit.c index acce285a4959..dec60e97e87a 100644 --- a/lib/checksum_kunit.c +++ b/lib/checksum_kunit.c @@ -217,16 +217,19 @@ static const u32 init_sums_no_overflow[] = { }; static const u16 expected_csum_ipv6_magic[] = { - 0x3045, 0xb681, 0xc210, 0xe77b, 0x41cc, 0xa904, 0x4367, 0xe8d8, 0x5809, - 0x5901, 0x5a40, 0xd3f4, 0xe467, 0xddde, 0xa609, 0xae05, 0xed14, 0x9133, - 0x8007, 0x89b6, 0x97b0, 0x8927, 0x1e43, 0x7903, 0x4772, 0xd3a4, 0x457b, - 0x83d0, 0x4ce1, 0x3656, 0x2dfc, 0xb678, 0x9a83, 0xd523, 0x61db, 0x379e, - 0x3880, 0xbb23, 0xa38b, 0xd2eb, 0x991a, 0xcf73, 0x13ea, 0x890f, 0x20ce, - 0x60ad, 0x5688, 0x4b13, 0x9399, 0x8a36, 0x75ae, 0x513a, 0xb222, 0xf3bb, - 0x80d4, 0x1f98, 0xc2bc, 0xf566, 0x796a, 0x268a, 0xe67f, 0xb917, 0xd716, - 0x3a16, 0x1858, 0x9d5b, 0x6fb4, 0x72b4, 0x1196, 0xb3d0, 0x89dc, 0xdd07, - 0x1a8d, 0xfa6d, 0x1507, 0xafab, 0x7d37, 0xa623, 0x72dd, 0x2083, 0xdfc4, - 0xa267, 0x92c9, 0xc8ad, + 0x2fbd, 0xb5d0, 0xc16e, 0xe6c1, 0x412e, 0xa836, 0x433b, 0xe87c, 0x57ea, + 0x5875, 0x5a21, 0xd361, 0xe422, 0xdd50, 0xa57f, 0xad6b, 0xecd1, 0x90b5, + 0x7fda, 0x88db, 0x979e, 0x8916, 0x1df0, 0x7853, 0x473e, 0xd2b3, 0x4526, + 0x8304, 0x4c34, 0x363d, 0x2dc1, 0xb66c, 0x9a28, 0xd4f2, 0x615d, 0x36dd, + 0x3784, 0xbadd, 0xa2c6, 0xd293, 0x9830, 0xcea8, 0x1349, 0x886d, 0x20a3, + 0x6001, 0x5607, 0x4a30, 0x9365, 0x893c, 0x7505, 0x50a1, 0xb200, 0xf3ad, + 0x80bf, 0x1f48, 0xc1d9, 0xf55d, 0x7871, 0x262a, 0xe606, 0xb894, 0xd6cd, + 0x39ed, 0x1817, 0x9d20, 0x6f93, 0x722d, 0x1116, 0xb3b4, 0x88ec, 0xdcb5, + 0x1a46, 0xfa1d, 0x141e, 0xaef7, 0x7cee, 0xa583, 0x72ad, 0x201b, 0xdece, + 0xa1ee, 0x92bd, 0xc7ba, 0x403e, 0x50a9, 0xcb5a, 0xff3b, 0x1b41, 0xa06b, + 0xcf2d, 0xcc9a, 0xf42a, 0x0c61, 0x7654, 0xf3d4, 0xcc25, 0x4985, 0x7606, + 0xc8a2, 0x6636, 0x610e, 0xc454, 0xefa4, 0xf3a6, 0x43a7, 0xd8e2, 0xe31e, + 0x150b, 0x445d, 0x57d5, 0x253c, 0x6adf, 0x3c53, 0x502c, 0x9ee5, 0x8422, }; static const u16 expected_fast_csum[] = { @@ -465,44 +468,36 @@ static void test_ip_fast_csum(struct kunit *test) } } +#define IPV6_NUM_TESTS ((MAX_LEN - sizeof(struct in6_addr) * 2 - sizeof(int) * 3) / WORD_ALIGNMENT) + static void test_csum_ipv6_magic(struct kunit *test) { #if defined(CONFIG_NET) + const struct in6_addr *saddr; + const struct in6_addr *daddr; + unsigned int len; __sum16 csum_result, expected; - struct csum_ipv6_magic_data { - const struct in6_addr saddr; - const struct in6_addr daddr; - unsigned int len; - __wsum csum; - unsigned char proto; - } data, *data_ptr; - int num_tests = MAX_LEN / WORD_ALIGNMENT - sizeof(struct csum_ipv6_magic_data); + unsigned char proto; + unsigned int csum; - for (int i = 0; i < num_tests; i++) { - data_ptr = (struct csum_ipv6_magic_data *)(random_buf + (i * WORD_ALIGNMENT)); + const int daddr_offset = sizeof(struct in6_addr); + const int len_offset = sizeof(struct in6_addr) + sizeof(struct in6_addr); + const int csum_offset = sizeof(struct in6_addr) + sizeof(struct in6_addr) + + sizeof(int); + const int proto_offset = sizeof(struct in6_addr) + sizeof(struct in6_addr) + + sizeof(int) * 2; - cpu_to_be32_array((__be32 *)&data.saddr, (const u32 *)&data_ptr->saddr, - sizeof_field(struct csum_ipv6_magic_data, saddr) / 4); - cpu_to_be32_array((__be32 *)&data.daddr, (const u32 *)&data_ptr->daddr, - sizeof_field(struct csum_ipv6_magic_data, daddr) / 4); - data.len = data_ptr->len; - data.csum = (__force __wsum)htonl((__force u32)data_ptr->csum); - /* - * proto must be zero to be compatible between big-endian and - * little-endian CPUs. On little-endian CPUs, proto is - * converted to a big-endian 32-bit value before the checksum - * operation. This causes proto to be in the most significant - * 8 bits on a little-endian CPU. On big-endian CPUs proto will - * remain in the least significant 8 bits. There does not exist - * a transformation to an arbitrary proto that will allow - * csum_ipv6_magic to return the same value on a big-endian and - * little-endian CPUs. - */ - data.proto = 0; - csum_result = csum_ipv6_magic(&data.saddr, &data.daddr, - data.len, data.proto, - data.csum); - expected = (__force __sum16)expected_csum_ipv6_magic[i]; + for (int i = 0; i < IPV6_NUM_TESTS; i++) { + int index = i * WORD_ALIGNMENT; + + saddr = (const struct in6_addr *)(random_buf + index); + daddr = (const struct in6_addr *)(random_buf + index + daddr_offset); + len = ntohl(*(unsigned int *)(random_buf + index + len_offset)); + csum = *(unsigned int *)(random_buf + index + csum_offset); + proto = *(random_buf + index + proto_offset); + + csum_result = csum_ipv6_magic(saddr, daddr, len, proto, csum); + expected = (__force __sum16)htons(expected_csum_ipv6_magic[i]); CHECK_EQ(csum_result, expected); } #endif /* !CONFIG_NET */
Hi Günter, On Mon, Feb 12, 2024 at 6:13 PM Guenter Roeck <linux@roeck-us.net> wrote: > On Mon, Feb 12, 2024 at 12:26:08AM -0500, Charlie Jenkins wrote: > > On Sun, Feb 11, 2024 at 11:18:36AM -0800, Guenter Roeck wrote: > > > On 2/7/24 16:22, Charlie Jenkins wrote: > > > > The ip_fast_csum and csum_ipv6_magic tests did not have the data > > > > types properly casted, and improperly misaligned data. > > > > > > > > Signed-off-by: Charlie Jenkins <charlie@rivosinc.com> > > > > > > I sorted out most of the problems with this version, but I still get: > > > > > > # test_csum_ipv6_magic: ASSERTION FAILED at lib/checksum_kunit.c:513 > > > Expected ( u64)csum_result == ( u64)expected, but > > > ( u64)csum_result == 16630 (0x40f6) > > > ( u64)expected == 65535 (0xffff) > > > not ok 5 test_csum_ipv6_magic > > > > > > on m68k:q800. This is suspicious because there is no 0xffff in > > > expected_csum_ipv6_magic[]. With some debugging information: > > > > > > ####### num_tests=86 i=84 expect array size=84 > > > ####### MAX_LEN=512 WORD_ALIGNMENT=4 magic data size=42 > > > > > > That means the loop > > > > > > for (int i = 0; i < num_tests; i++) { > > > ... > > > expected = (__force __sum16)expected_csum_ipv6_magic[i]; > > > ... > > > } > > > > > > will access data beyond the end of the expected_csum_ipv6_magic[] array, > > > possibly because m68k doesn't pad struct csum_ipv6_magic_data to 44 bytes. > > > > Okay I will check that out. > > > > > > > > In this context, is the comment about proto having to be 0 really true ? > > > It seems to me that the calculated checksum must be identical on both > > > little and big endian systems. After all, they need to be able to talk > > > to each other. > > > > I agree, but I couldn't find a solution other than setting it to zero. > > Maybe I am missing something simple... > > > > Try the patch below on top of yours. It should work on both big and little > endian systems. > > Key changes: > - use random_buf directly instead of copying anything > - no need to convert source / destination addresses > - csum in the buffer is in network byte order and needs > to stay that way > - len in the buffer is in network byte order and needs to be > converted to host byte order since that is expected by > csum_ipv6_magic() > - the expected value is in host byte order and needs to be > converted to network byte order for comparison > - protocol is just fine and converted by csum_ipv6_magic() > as needed Thanks for your patch! > --- a/lib/checksum_kunit.c > +++ b/lib/checksum_kunit.c > @@ -465,44 +468,36 @@ static void test_ip_fast_csum(struct kunit *test) > } > } > > +#define IPV6_NUM_TESTS ((MAX_LEN - sizeof(struct in6_addr) * 2 - sizeof(int) * 3) / WORD_ALIGNMENT) > + > static void test_csum_ipv6_magic(struct kunit *test) > { > #if defined(CONFIG_NET) > + const struct in6_addr *saddr; > + const struct in6_addr *daddr; > + unsigned int len; > __sum16 csum_result, expected; > - struct csum_ipv6_magic_data { > - const struct in6_addr saddr; > - const struct in6_addr daddr; > - unsigned int len; > - __wsum csum; > - unsigned char proto; > - } data, *data_ptr; > - int num_tests = MAX_LEN / WORD_ALIGNMENT - sizeof(struct csum_ipv6_magic_data); > + unsigned char proto; > + unsigned int csum; > > - for (int i = 0; i < num_tests; i++) { > - data_ptr = (struct csum_ipv6_magic_data *)(random_buf + (i * WORD_ALIGNMENT)); > + const int daddr_offset = sizeof(struct in6_addr); > + const int len_offset = sizeof(struct in6_addr) + sizeof(struct in6_addr); > + const int csum_offset = sizeof(struct in6_addr) + sizeof(struct in6_addr) + > + sizeof(int); > + const int proto_offset = sizeof(struct in6_addr) + sizeof(struct in6_addr) + > + sizeof(int) * 2; Please no manual offset calculations. Please fix the csum_ipv6_magic_data structure definition instead. > > - cpu_to_be32_array((__be32 *)&data.saddr, (const u32 *)&data_ptr->saddr, > - sizeof_field(struct csum_ipv6_magic_data, saddr) / 4); > - cpu_to_be32_array((__be32 *)&data.daddr, (const u32 *)&data_ptr->daddr, > - sizeof_field(struct csum_ipv6_magic_data, daddr) / 4); > - data.len = data_ptr->len; > - data.csum = (__force __wsum)htonl((__force u32)data_ptr->csum); > - /* > - * proto must be zero to be compatible between big-endian and > - * little-endian CPUs. On little-endian CPUs, proto is > - * converted to a big-endian 32-bit value before the checksum > - * operation. This causes proto to be in the most significant > - * 8 bits on a little-endian CPU. On big-endian CPUs proto will > - * remain in the least significant 8 bits. There does not exist > - * a transformation to an arbitrary proto that will allow > - * csum_ipv6_magic to return the same value on a big-endian and > - * little-endian CPUs. > - */ > - data.proto = 0; > - csum_result = csum_ipv6_magic(&data.saddr, &data.daddr, > - data.len, data.proto, > - data.csum); > - expected = (__force __sum16)expected_csum_ipv6_magic[i]; > + for (int i = 0; i < IPV6_NUM_TESTS; i++) { > + int index = i * WORD_ALIGNMENT; > + > + saddr = (const struct in6_addr *)(random_buf + index); > + daddr = (const struct in6_addr *)(random_buf + index + daddr_offset); > + len = ntohl(*(unsigned int *)(random_buf + index + len_offset)); > + csum = *(unsigned int *)(random_buf + index + csum_offset); > + proto = *(random_buf + index + proto_offset); > + > + csum_result = csum_ipv6_magic(saddr, daddr, len, proto, csum); > + expected = (__force __sum16)htons(expected_csum_ipv6_magic[i]); > CHECK_EQ(csum_result, expected); > } > #endif /* !CONFIG_NET */ Gr{oetje,eeting}s, Geert