netdev - Re: [PATCH bpf-next 4/6] selftests/bpf: add ipv4 and dual ipv4/ipv6 support in btf_skc_cls

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <b9f7bf76-d58f-4e65-82e7-551464b6b7df@bootlin.com>
Date: Sat, 19 Oct 2024 14:43:19 +0200
From: Alexis Lothoré <alexis.lothore@...tlin.com>
To: Martin KaFai Lau <martin.lau@...ux.dev>
Cc: Alexei Starovoitov <ast@...nel.org>,
 Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>,
 Eduard Zingerman <eddyz87@...il.com>, Song Liu <song@...nel.org>,
 Yonghong Song <yonghong.song@...ux.dev>,
 John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>,
 Stanislav Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>,
 Jiri Olsa <jolsa@...nel.org>, Mykola Lysenko <mykolal@...com>,
 Shuah Khan <shuah@...nel.org>, "David S. Miller" <davem@...emloft.net>,
 Jakub Kicinski <kuba@...nel.org>, Jesper Dangaard Brouer <hawk@...nel.org>,
 ebpf@...uxfoundation.org, Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
 Lorenz Bauer <lmb@...udflare.com>, bpf@...r.kernel.org,
 linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
 netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next 4/6] selftests/bpf: add ipv4 and dual ipv4/ipv6
 support in btf_skc_cls_ingress

On 10/19/24 02:30, Martin KaFai Lau wrote:
> On 10/16/24 11:35 AM, Alexis Lothoré (eBPF Foundation) wrote:

[...]

>>   +    switch (ip_mode) {
>> +    case TEST_MODE_IPV4:
>> +        sock_family = AF_INET;
>> +        srv_addr = SERVER_ADDR_IPV4;
>> +        addr = (struct sockaddr_storage *)&srv_sa4;
>> +        addr_len = sizeof(srv_sa4);
>> +        break;
>> +    case TEST_MODE_IPV6:
>> +        opts.post_socket_cb = v6only_true;
>> +        sock_family = AF_INET6;
>> +        srv_addr = SERVER_ADDR_IPV6;
>> +        addr = (struct sockaddr_storage *)&srv_sa6;
>> +        addr_len = sizeof(srv_sa6);
>> +        break;
>> +    case TEST_MODE_DUAL:
>> +        opts.post_socket_cb = v6only_false;
>> +        sock_family = AF_INET6;
>> +        srv_addr = SERVER_ADDR_DUAL;
>> +        addr = (struct sockaddr_storage *)&srv_sa6;
>> +        addr_len = sizeof(srv_sa6);
>> +        break;
>> +    default:
>> +            break;
> 
> nit. indentation is off.

True, and for some reason checkpatch does not raise any warning here. This will
be fixed in v2 for all switch cases

> better directly "return;", in case future something complains vars are not init.

ACK. I'll also add a PRINT_FAIL call in those default statements.

> 
>> +    }
>> +
>>       if (write_sysctl("/proc/sys/net/ipv4/tcp_syncookies", tcp_syncookies))
>>           return;
>>   -    listen_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0);
>> +    listen_fd = start_server_str(sock_family, SOCK_STREAM, srv_addr,  0,
>> +                     &opts);
>>       if (!ASSERT_OK_FD(listen_fd, "start server"))
>>           return;
>>   -    err = getsockname(listen_fd, (struct sockaddr *)&srv_sa6, &addrlen);
>> +    err = getsockname(listen_fd, (struct sockaddr *)addr, &addr_len);
>>       if (!ASSERT_OK(err, "getsockname(listen_fd)"))
>>           goto done;
>> -    memcpy(&skel->bss->srv_sa6, &srv_sa6, sizeof(srv_sa6));
>> -    srv_port = ntohs(srv_sa6.sin6_port);
>> +
>> +    switch (ip_mode) {
>> +    case TEST_MODE_IPV4:
>> +        memcpy(&skel->bss->srv_sa4, &srv_sa4, sizeof(srv_sa4));
>> +        srv_port = ntohs(srv_sa4.sin_port);
>> +        break;
>> +    case TEST_MODE_IPV6:
>> +    case TEST_MODE_DUAL:
>> +        memcpy(&skel->bss->srv_sa6, &srv_sa6, sizeof(srv_sa6));
>> +        srv_port = ntohs(srv_sa6.sin6_port);
>> +        break;
>> +    default:
>> +            break;
> 
> indentation off. also "goto done;"

I can add the goto done as a safety net (eg in case someone modifies the test
later and miss some details), but with the return added on the switch above
(also done on ip_mode), the goto should never be executed.

[...]

>>   -static int handle_ip6_tcp(struct ipv6hdr *ip6h, struct __sk_buff *skb)
>> +static int handle_ip_tcp(struct ethhdr *eth, struct __sk_buff *skb)
>>   {
>> -    struct bpf_sock_tuple *tuple;
>> +    struct bpf_sock_tuple *tuple = NULL;
>> +    unsigned int tuple_len = 0;
>>       struct bpf_sock *bpf_skc;
>> -    unsigned int tuple_len;
>> +    struct ipv6hdr *ip6h;
>> +    void *iphdr = NULL;
>> +    int iphdr_size = 0;
>> +    struct iphdr *ip4h;
> 
> nit. All new "= 0;" and "= NULL;" init should not be needed.

I added those "by default" because my last series raised some build errors with
clang while building fine with gcc (but in the end, there was indeed code paths
possibly using those variables uninitialized). ACK, I'll remove those initializers.

[...]

>> +    }
>>   -    /* Is it the testing traffic? */
>> -    if (th->dest != srv_sa6.sin6_port)
>> +    if (!tuple) {
> 
> !tuple should not be possible. can be removed.

I initially added that check because of the verifier complained about tuple
being scalar. But I have checked and removed it again and it is now... accepted
? I guess I was still missing some other parts when I got this error, very
likely the `return TC_ACT_OK` in the default case. I'll then remove this check
in v2.

[...]

>>   -    if (ip6h->nexthdr == IPPROTO_TCP)
>> -        return handle_ip6_tcp(ip6h, skb);
>> +    return handle_ip_tcp(eth, skb);
>>         return TC_ACT_OK;
> 
> The last double return should have been removed also.

That's indeed a miss, I'll remove it.

Thanks,

Alexis


-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com