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

["Alexis Lothoré" <alexis.lothore@bootlin.com>]

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