Messages in this thread | | | From | Geert Uytterhoeven <> | Date | Fri, 16 Feb 2024 10:01:05 +0100 | Subject | Re: [PATCH v6 0/2] lib: checksum: Fix issues with checksum tests |
| |
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
-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
| |