From: Yonghong Song <yonghong.song@linux.dev>
To: Martin KaFai Lau <martin.lau@linux.dev>,
Kuniyuki Iwashima <kuniyu@amazon.com>
Cc: Kuniyuki Iwashima <kuni1840@gmail.com>,
bpf@vger.kernel.org, netdev@vger.kernel.org,
Eric Dumazet <edumazet@google.com>,
Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Andrii Nakryiko <andrii@kernel.org>,
Paolo Abeni <pabeni@redhat.com>
Subject: Re: [PATCH v7 bpf-next 6/6] selftest: bpf: Test bpf_sk_assign_tcp_reqsk().
Date: Tue, 2 Jan 2024 11:17:19 -0800 [thread overview]
Message-ID: <9b251840-7cb8-4d17-bd23-1fc8071d8eef@linux.dev> (raw)
In-Reply-To: <bd21939e-c6c8-4fb2-a4b6-e085a2230c8e@linux.dev>
On 12/20/23 10:35 PM, Martin KaFai Lau wrote:
> On 12/20/23 5:28 PM, Kuniyuki Iwashima wrote:
>> +static int tcp_validate_header(struct tcp_syncookie *ctx)
>> +{
>> + s64 csum;
>> +
>> + if (tcp_reload_headers(ctx))
>> + goto err;
>> +
>> + csum = bpf_csum_diff(0, 0, (void *)ctx->tcp, ctx->tcp->doff * 4,
>> 0);
>> + if (csum < 0)
>> + goto err;
>> +
>> + if (ctx->ipv4) {
>> + /* check tcp_v4_csum(csum) is 0 if not on lo. */
>> +
>> + csum = bpf_csum_diff(0, 0, (void *)ctx->ipv4, ctx->ipv4->ihl
>> * 4, 0);
>> + if (csum < 0)
>> + goto err;
>> +
>> + if (csum_fold(csum) != 0)
>> + goto err;
>> + } else if (ctx->ipv6) {
>> + /* check tcp_v6_csum(csum) is 0 if not on lo. */
>> + }
>> +
>> + return 0;
>> +err:
>> + return -1;
>> +}
>> +
>> +static int tcp_parse_option(__u32 index, struct tcp_syncookie *ctx)
>> +{
>> + char opcode, opsize;
>> +
>> + if (ctx->ptr + 1 > ctx->data_end)
>> + goto stop;
>> +
>> + opcode = *ctx->ptr++;
>> +
>> + if (opcode == TCPOPT_EOL)
>> + goto stop;
>> +
>> + if (opcode == TCPOPT_NOP)
>> + goto next;
>> +
>> + if (ctx->ptr + 1 > ctx->data_end)
>> + goto stop;
>> +
>> + opsize = *ctx->ptr++;
>> +
>> + if (opsize < 2)
>> + goto stop;
>> +
>> + switch (opcode) {
>> + case TCPOPT_MSS:
>> + if (opsize == TCPOLEN_MSS && ctx->tcp->syn &&
>> + ctx->ptr + (TCPOLEN_MSS - 2) < ctx->data_end)
>> + ctx->attrs.mss = get_unaligned_be16(ctx->ptr);
>> + break;
>> + case TCPOPT_WINDOW:
>> + if (opsize == TCPOLEN_WINDOW && ctx->tcp->syn &&
>> + ctx->ptr + (TCPOLEN_WINDOW - 2) < ctx->data_end) {
>> + ctx->attrs.wscale_ok = 1;
>> + ctx->attrs.snd_wscale = *ctx->ptr;
>> + }
>> + break;
>> + case TCPOPT_TIMESTAMP:
>> + if (opsize == TCPOLEN_TIMESTAMP &&
>> + ctx->ptr + (TCPOLEN_TIMESTAMP - 2) < ctx->data_end) {
>> + ctx->attrs.rcv_tsval = get_unaligned_be32(ctx->ptr);
>> + ctx->attrs.rcv_tsecr = get_unaligned_be32(ctx->ptr + 4);
>> +
>> + if (ctx->tcp->syn && ctx->attrs.rcv_tsecr)
>> + ctx->attrs.tstamp_ok = 0;
>> + else
>> + ctx->attrs.tstamp_ok = 1;
>> + }
>> + break;
>> + case TCPOPT_SACK_PERM:
>> + if (opsize == TCPOLEN_SACK_PERM && ctx->tcp->syn &&
>> + ctx->ptr + (TCPOLEN_SACK_PERM - 2) < ctx->data_end)
>> + ctx->attrs.sack_ok = 1;
>> + break;
>> + }
>> +
>> + ctx->ptr += opsize - 2;
>> +next:
>> + return 0;
>> +stop:
>> + return 1;
>> +}
>> +
>> +static void tcp_parse_options(struct tcp_syncookie *ctx)
>> +{
>> + ctx->ptr = (char *)(ctx->tcp + 1);
>> +
>> + bpf_loop(40, tcp_parse_option, ctx, 0);
>> +}
>> +
>> +static int tcp_validate_sysctl(struct tcp_syncookie *ctx)
>> +{
>> + if ((ctx->ipv4 && ctx->attrs.mss != MSS_LOCAL_IPV4) ||
>> + (ctx->ipv6 && ctx->attrs.mss != MSS_LOCAL_IPV6))
>> + goto err;
>> +
>> + if (!ctx->attrs.wscale_ok || ctx->attrs.snd_wscale != 7)
>> + goto err;
>> +
>> + if (!ctx->attrs.tstamp_ok)
>
> The bpf-ci reported error in cpuv4. The email from
> bot+bpf-ci@kernel.org has the link.
>
> I tried the following:
>
> if (!ctx->attrs.tstamp_ok) {
> bpf_printk("ctx->attrs.tstamp_ok %u",
> ctx->attrs.tstamp_ok);
> goto err;
> }
>
>
> The above prints tstamp_ok as 1 while there is a "if
> (!ctx->attrs.tstamp_ok)" test before it.
>
> Yonghong and I debugged it quite a bit. verifier concluded the
> ctx->attrs.tstamp_ok is 0. We knew some red herring like cpuv4 has
> fewer register spilling but not able to root cause it yet.
>
> In the mean time, there are existing selftests parsing the tcp header.
> For example, the test_parse_tcp_hdr_opt[_dynptr].c. Not as complete as
> your tcp_parse_option() but should be pretty close. It does not use
> bpf_loop. It uses a bounded loop + a subprog (the parse_hdr_opt in the
> selftests) instead. You can consider a similar construct to see if it
> works around the cpuv4 CI issue for the time being.
I did some investigation and found some issues in verifier. I need to dig more into details.
On the other hand, with the following patch
https://lore.kernel.org/bpf/20240102190726.2017424-1-yonghong.song@linux.dev/
the selftest can run successfully with cpuv2/v3/v4.
>
> pw-bot: cr
>
>> + goto err;
>> +
>> + if (!ctx->attrs.sack_ok)
>> + goto err;
>> +
>> + if (!ctx->tcp->ece || !ctx->tcp->cwr)
>> + goto err;
>> +
>> + return 0;
>> +err:
>> + return -1;
>> +}
>> +
>
> [ ... ]
>
>> +static int tcp_handle_syn(struct tcp_syncookie *ctx)
>> +{
>> + s64 csum;
>> +
>> + if (tcp_validate_header(ctx))
>> + goto err;
>> +
>> + tcp_parse_options(ctx);
>> +
>> + if (tcp_validate_sysctl(ctx))
>> + goto err;
>> +
>
next prev parent reply other threads:[~2024-01-02 19:17 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-21 1:28 [PATCH v7 bpf-next 0/6] bpf: tcp: Support arbitrary SYN Cookie at TC Kuniyuki Iwashima
2023-12-21 1:28 ` [PATCH v7 bpf-next 1/6] tcp: Move tcp_ns_to_ts() to tcp.h Kuniyuki Iwashima
2023-12-21 1:28 ` [PATCH v7 bpf-next 2/6] tcp: Move skb_steal_sock() to request_sock.h Kuniyuki Iwashima
2023-12-21 1:28 ` [PATCH v7 bpf-next 3/6] bpf: tcp: Handle BPF SYN Cookie in skb_steal_sock() Kuniyuki Iwashima
2023-12-21 1:28 ` [PATCH v7 bpf-next 4/6] bpf: tcp: Handle BPF SYN Cookie in cookie_v[46]_check() Kuniyuki Iwashima
2023-12-21 1:28 ` [PATCH v7 bpf-next 5/6] bpf: tcp: Support arbitrary SYN Cookie Kuniyuki Iwashima
2024-01-12 1:44 ` Martin KaFai Lau
2024-01-15 20:13 ` Kuniyuki Iwashima
2023-12-21 1:28 ` [PATCH v7 bpf-next 6/6] selftest: bpf: Test bpf_sk_assign_tcp_reqsk() Kuniyuki Iwashima
2023-12-21 6:35 ` Martin KaFai Lau
2023-12-21 7:04 ` Kuniyuki Iwashima
2023-12-21 8:43 ` Kuniyuki Iwashima
2024-01-02 19:17 ` Yonghong Song [this message]
2023-12-21 16:44 ` Matthieu Baerts
2024-01-12 6:20 ` [PATCH v7 bpf-next 0/6] bpf: tcp: Support arbitrary SYN Cookie at TC Martin KaFai Lau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9b251840-7cb8-4d17-bd23-1fc8071d8eef@linux.dev \
--to=yonghong.song@linux.dev \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=edumazet@google.com \
--cc=kuni1840@gmail.com \
--cc=kuniyu@amazon.com \
--cc=martin.lau@linux.dev \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).