From: Kui-Feng Lee <sinquersw@gmail.com>
To: Kuniyuki Iwashima <kuniyu@amazon.com>
Cc: andrii@kernel.org, ast@kernel.org, bpf@vger.kernel.org,
daniel@iogearbox.net, davem@davemloft.net, dsahern@kernel.org,
edumazet@google.com, haoluo@google.com, john.fastabend@gmail.com,
jolsa@kernel.org, kpsingh@kernel.org, kuba@kernel.org,
kuni1840@gmail.com, martin.lau@linux.dev, mykolal@fb.com,
netdev@vger.kernel.org, pabeni@redhat.com, sdf@google.com,
song@kernel.org, yonghong.song@linux.dev
Subject: Re: [PATCH v1 bpf-next 00/11] bpf: tcp: Add SYN Cookie generation/validation SOCK_OPS hooks.
Date: Tue, 24 Oct 2023 10:55:22 -0700 [thread overview]
Message-ID: <dd6ef06f-1d6c-4dfc-a7d8-58903c0fe1c8@gmail.com> (raw)
In-Reply-To: <20231024012208.82653-1-kuniyu@amazon.com>
On 10/23/23 18:22, Kuniyuki Iwashima wrote:
>> On 10/23/23 14:35, Martin KaFai Lau wrote:
>>> On 10/20/23 11:48 PM, Kuniyuki Iwashima wrote:
>>>> I think this was doable. With the diff below, I was able to skip
>>>> validation in cookie_v[46]_check() when if skb->sk is not NULL.
>>>>
>>>> The kfunc allocates req and set req->syncookie to 1, which is usually
>>>> set in TX path, so if it's 1 in RX (inet_steal_sock()), we can see
>>>> that req is allocated by kfunc (at least, req->syncookie &&
>>>> req->rsk_listener never be true in the current TCP stack).
>>>>
>>>> The difference here is that req allocated by kfunc holds refcnt of
>>>> rsk_listener (passing true to inet_reqsk_alloc()) to prevent freeing
>>>> the listener until req reaches cookie_v[46]_check().
>>>
>>> The cookie_v[46]_check() holds the listener sk refcnt now?
>>
>> The caller of cookie_v[46]_check() should hold a refcnt of the listener.
>
> No, it need not.
>
> When we handle the default syn cookie, cookie_tcp_reqsk_alloc() passes
> false to inet_reqsk_alloc(), then reqsk does not hold refcnt of the
> listener.
>
> If inet_csk_reqsk_queue_add() in tcp_get_cookie_sock() succeeds, we know
> the listener is still alive
What I said is the callers of cookie_v[46]_check().
For example, tcp_v4_rcv() will make sure the existence of the sk passing
to tcp_v4_do_rcv() -> tcp_v4_cookie_check(). tcp_v4_rcv() gets the sk
from __inet_lookup_skb(). The sk can be refcounted or not.
For the case of not refcounted, it should be rcu protected
(SOCK_RCU_FREE).
AFAIK, tcp_v4_rcv() is called in a rcu_read_lock() section
(far in ip_local_deliver_finish(), even netif_receive_skb_core()).
tcp_v4_rcv() and cookie_v4_check() also access the content of sk
without increase the refcount of sk. That also indicate these function
believe the sk returned by __inet_lookup_skb() is either refcounted
or protected in someway (RCU here).
What I mean protection is that the sk may be closed but not destroyed.
>
>
>> If the listener is destroyed, the callers of cookie_v[46]_check() should
>> fail to lookup a sock for the skb. However, in this case, the kfunc sets
>> a sock to skb->sk, and the lookup function
>> (__inet_lookup_skb()) steals sock from skb. So, there is no guarantee
>> ensuring the listener is still alive.
>>
>> One solution is let the stealing function to lookup the listener if
>> inet_reqsk(skb->sk)->syncookie is true.
>
> kfunc at least guarantees that the listener is not freed until req
> is freed. There's two cases where the listener could be close()d
> after kfunc:
>
> 1. close()d before lookup
> -> kfree_skb(skb) calls reqsk_put() and releases the last
> refcnt of the listener
>
> 2. close()d between lookup and inet_csk_reqsk_queue_add()
> -> inet_csk_reqsk_queue_add() fails and __reqsk_free()
> releases the last refcnt of the listener.
>
> So, we need not look up the listener again in inet_steal_sock().
After thinking about this again, increasing the refcount of the listener
in the kfunc is not necessary. Since the caller of a
bpf program should already hold a refcount of the sk or
rcu protected, we can let inet_csk_reqsk_queue_add() handle it,
just like what you mentioned earlier.
WDYT?
>
>
>>>
>>> >
>>>> The cookie generation at least should be done at tc/xdp. The
>>>> valdation can be done earlier as well on tc/xdp, but it could
>>>> add another complexity, listener's life cycle if we allocate
>>>> req there.
>>>
>>> I think your code below looks pretty close already.
>>>
>>> It seems the only concern/complexity is the extra rsk_listener refcnt (btw the
>>> concern is on performance for the extra refcnt? or there is correctness issue?).
>
> Yes, that's the only concern and I think it's all ok now.
>
> [ I was seeing a weird refcnt warning, but I missed *refcounted was true
> in inet_steal_sock() for reqsk and forgot to flipping it to false :S ]
>
>
>>>
>>> Asking because bpf_sk_assign() can already assign a listener to skb->sk and it
>>> also does not take a refcnt on the listener. The same no refcnt needed on
>>> req->rsk_listener should be doable also. sock_pfree may need to be smarter to
>>> check req->syncookie. What else may need to change?
>
> I was wondering if we are in the same RCU period between tc and
> cookie_v[46]_check(), but yeah, probably sock_pfree() can check
> req->syncookie and set NULL to rsk_listener so that reqsk_put()
> will not touch the listener.
>
>
>>>
>>>>
>>>> I'm wondering which place to add the validation capability, and
>>>> I think SOCK_OPS is simpler than tc.
>>>>
>>>> #1 validate cookie and allocate req at tc, and skip validation
>>>>
>>>> #2 validate cookie (and update bpf map at xdp/tc, and look up bpf
>>>> map) and allocate req at SOCK_OPS hook
>>>>
>>>> Given SYN proxy is usually on the other node and incoming cookie
>>>> is almost always valid, we might need not validate it in the early
>>>> stage in the stack.
>>>>
>>>> What do you think ?
>>>
>>> Yeah, supporting validation in sock_ops is an open option if the tc side is too
>>> hard but I feel you are pretty close on the tc side.
>
> Now I think I can go v2 with tc.
>
> Thanks for your guide!
>
>
>>>
>>>>
>>>> ---8<---
>>>> diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
>>>> index 3ecfeadbfa06..e5e4627bf270 100644
>>>> --- a/include/net/inet_hashtables.h
>>>> +++ b/include/net/inet_hashtables.h
>>>> @@ -462,9 +462,19 @@ struct sock *inet_steal_sock(struct net *net, struct sk_buff *skb, int doff,
>>>> if (!sk)
>>>> return NULL;
>>>>
>>>> - if (!prefetched || !sk_fullsock(sk))
>>>> + if (!prefetched)
>>>> return sk;
>>>>
>>>> + if (!sk_fullsock(sk)) {
>>>> + if (sk->sk_state == TCP_NEW_SYN_RECV && inet_reqsk(sk)->syncookie) {
>>>> + skb->sk = sk;
>>>> + skb->destructor = sock_pfree;
>>>> + sk = inet_reqsk(sk)->rsk_listener;
>>>> + }
>>>> +
>>>> + return sk;
>>>> + }
>>>> +
>>>> if (sk->sk_protocol == IPPROTO_TCP) {
>>>> if (sk->sk_state != TCP_LISTEN)
>>>> return sk;
>>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>>> index cc2e4babc85f..bca491ddf42c 100644
>>>> --- a/net/core/filter.c
>>>> +++ b/net/core/filter.c
>>>> @@ -11800,6 +11800,71 @@ __bpf_kfunc int bpf_sock_addr_set_sun_path(struct bpf_sock_addr_kern *sa_kern,
>>>>
>>>> return 0;
>>>> }
>>>> +
>>>> +__bpf_kfunc int bpf_sk_assign_tcp_reqsk(struct sk_buff *skb, struct sock *sk,
>>>> + struct tcp_options_received *tcp_opt,
>>>> + int tcp_opt__sz, u16 mss)
>>>> +{
>>>> + const struct tcp_request_sock_ops *af_ops;
>>>> + const struct request_sock_ops *ops;
>>>> + struct inet_request_sock *ireq;
>>>> + struct tcp_request_sock *treq;
>>>> + struct request_sock *req;
>>>> +
>>>> + if (!sk)
>>>> + return -EINVAL;
>>>> +
>>>> + if (!skb_at_tc_ingress(skb))
>>>> + return -EINVAL;
>>>> +
>>>> + if (dev_net(skb->dev) != sock_net(sk))
>>>> + return -ENETUNREACH;
>>>> +
>>>> + switch (sk->sk_family) {
>>>> + case AF_INET: /* TODO: MPTCP */
>>>> + ops = &tcp_request_sock_ops;
>>>> + af_ops = &tcp_request_sock_ipv4_ops;
>>>> + break;
>>>> +#if IS_ENABLED(CONFIG_IPV6)
>>>> + case AF_INET6:
>>>> + ops = &tcp6_request_sock_ops;
>>>> + af_ops = &tcp_request_sock_ipv6_ops;
>>>> + break;
>>>> +#endif
>>>> + default:
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + if (sk->sk_type != SOCK_STREAM || sk->sk_state != TCP_LISTEN)
>>>> + return -EINVAL;
>>>> +
>>>> + req = inet_reqsk_alloc(ops, sk, true);
>>>> + if (!req)
>>>> + return -ENOMEM;
>>>> +
>>>> + ireq = inet_rsk(req);
>>>> + treq = tcp_rsk(req);
>>>> +
>>>> + refcount_set(&req->rsk_refcnt, 1);
>>>> + req->syncookie = 1;
>>>> + req->mss = mss;
>>>> + req->ts_recent = tcp_opt->saw_tstamp ? tcp_opt->rcv_tsval : 0;
>>>> +
>>>> + ireq->snd_wscale = tcp_opt->snd_wscale;
>>>> + ireq->sack_ok = tcp_opt->sack_ok;
>>>> + ireq->wscale_ok = tcp_opt->wscale_ok;
>>>> + ireq->tstamp_ok = tcp_opt->saw_tstamp;
>>>> +
>>>> + tcp_rsk(req)->af_specific = af_ops;
>>>> + tcp_rsk(req)->ts_off = tcp_opt->rcv_tsecr - tcp_ns_to_ts(tcp_clock_ns());
>>>> +
>>>> + skb_orphan(skb);
>>>> + skb->sk = req_to_sk(req);
>>>> + skb->destructor = sock_pfree;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> __diag_pop();
>>>>
>>>> int bpf_dynptr_from_skb_rdonly(struct sk_buff *skb, u64 flags,
>>>> @@ -11828,6 +11893,10 @@ BTF_SET8_START(bpf_kfunc_check_set_sock_addr)
>>>> BTF_ID_FLAGS(func, bpf_sock_addr_set_sun_path)
>>>> BTF_SET8_END(bpf_kfunc_check_set_sock_addr)
>>>>
>>>> +BTF_SET8_START(bpf_kfunc_check_set_tcp_reqsk)
>>>> +BTF_ID_FLAGS(func, bpf_sk_assign_tcp_reqsk)
>>>> +BTF_SET8_END(bpf_kfunc_check_set_tcp_reqsk)
>>>> +
>>>> static const struct btf_kfunc_id_set bpf_kfunc_set_skb = {
>>>> .owner = THIS_MODULE,
>>>> .set = &bpf_kfunc_check_set_skb,
>>>> @@ -11843,6 +11912,11 @@ static const struct btf_kfunc_id_set bpf_kfunc_set_sock_addr = {
>>>> .set = &bpf_kfunc_check_set_sock_addr,
>>>> };
>>>>
>>>> +static const struct btf_kfunc_id_set bpf_kfunc_set_tcp_reqsk = {
>>>> + .owner = THIS_MODULE,
>>>> + .set = &bpf_kfunc_check_set_tcp_reqsk,
>>>> +};
>>>> +
>>>> static int __init bpf_kfunc_init(void)
>>>> {
>>>> int ret;
>>>> @@ -11858,8 +11932,10 @@ static int __init bpf_kfunc_init(void)
>>>> ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_LWT_SEG6LOCAL, &bpf_kfunc_set_skb);
>>>> ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_NETFILTER, &bpf_kfunc_set_skb);
>>>> ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP, &bpf_kfunc_set_xdp);
>>>> - return ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
>>>> - &bpf_kfunc_set_sock_addr);
>>>> + ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_CGROUP_SOCK_ADDR,
>>>> + &bpf_kfunc_set_sock_addr);
>>>> + ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &bpf_kfunc_set_tcp_reqsk);
>>>> + return ret;
>>>> }
>>>> late_initcall(bpf_kfunc_init);
>>>>
>>>> ---8<---
prev parent reply other threads:[~2023-10-24 17:55 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-13 22:04 [PATCH v1 bpf-next 00/11] bpf: tcp: Add SYN Cookie generation/validation SOCK_OPS hooks Kuniyuki Iwashima
2023-10-13 22:04 ` [PATCH v1 bpf-next 01/11] tcp: Clean up reverse xmas tree in cookie_v[46]_check() Kuniyuki Iwashima
2023-10-13 22:04 ` [PATCH v1 bpf-next 02/11] tcp: Cache sock_net(sk) " Kuniyuki Iwashima
2023-10-13 22:04 ` [PATCH v1 bpf-next 03/11] tcp: Clean up goto labels " Kuniyuki Iwashima
2023-10-17 0:00 ` Kui-Feng Lee
2023-10-17 0:30 ` Kuniyuki Iwashima
2023-10-13 22:04 ` [PATCH v1 bpf-next 04/11] tcp: Don't initialise tp->tsoffset in tcp_get_cookie_sock() Kuniyuki Iwashima
2023-10-13 22:04 ` [PATCH v1 bpf-next 05/11] bpf: tcp: Add SYN Cookie generation SOCK_OPS hook Kuniyuki Iwashima
2023-10-18 0:54 ` Martin KaFai Lau
2023-10-18 17:00 ` Kuniyuki Iwashima
2023-10-13 22:04 ` [PATCH v1 bpf-next 06/11] bpf: tcp: Add SYN Cookie validation " Kuniyuki Iwashima
2023-10-16 20:38 ` Stanislav Fomichev
2023-10-16 22:02 ` Kuniyuki Iwashima
2023-10-17 16:52 ` Kuniyuki Iwashima
2023-10-13 22:04 ` [PATCH v1 bpf-next 07/11] bpf: Make bpf_sock_ops.replylong[1] writable Kuniyuki Iwashima
2023-10-13 22:04 ` [PATCH v1 bpf-next 08/11] bpf: tcp: Make TS available for SYN Cookie storage Kuniyuki Iwashima
2023-10-13 22:04 ` [PATCH v1 bpf-next 09/11] tcp: Split cookie_ecn_ok() Kuniyuki Iwashima
2023-10-13 22:04 ` [PATCH v1 bpf-next 10/11] bpf: tcp: Make WS, SACK, ECN configurable from BPF SYN Cookie Kuniyuki Iwashima
2023-10-18 1:08 ` Martin KaFai Lau
2023-10-18 17:02 ` Kuniyuki Iwashima
2023-10-13 22:04 ` [PATCH v1 bpf-next 11/11] selftest: bpf: Test BPF_SOCK_OPS_(GEN|CHECK)_SYNCOOKIE_CB Kuniyuki Iwashima
2023-10-17 5:50 ` Martin KaFai Lau
2023-10-17 16:29 ` Kuniyuki Iwashima
2023-10-16 13:05 ` [PATCH v1 bpf-next 00/11] bpf: tcp: Add SYN Cookie generation/validation SOCK_OPS hooks Daniel Borkmann
2023-10-16 16:11 ` Kuniyuki Iwashima
2023-10-16 14:19 ` Willem de Bruijn
2023-10-16 16:46 ` Kuniyuki Iwashima
2023-10-16 18:41 ` Willem de Bruijn
2023-10-17 5:53 ` Martin KaFai Lau
2023-10-17 16:48 ` Kuniyuki Iwashima
2023-10-18 6:19 ` Martin KaFai Lau
2023-10-18 8:02 ` Eric Dumazet
2023-10-18 17:20 ` Kuniyuki Iwashima
2023-10-18 21:47 ` Kui-Feng Lee
2023-10-18 22:31 ` Kuniyuki Iwashima
2023-10-19 7:25 ` Martin KaFai Lau
2023-10-19 18:01 ` Kuniyuki Iwashima
2023-10-20 19:59 ` Martin KaFai Lau
2023-10-20 23:10 ` Kuniyuki Iwashima
2023-10-21 6:48 ` Kuniyuki Iwashima
2023-10-23 21:35 ` Martin KaFai Lau
2023-10-24 0:37 ` Kui-Feng Lee
2023-10-24 1:22 ` Kuniyuki Iwashima
2023-10-24 17:55 ` Kui-Feng Lee [this message]
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=dd6ef06f-1d6c-4dfc-a7d8-58903c0fe1c8@gmail.com \
--to=sinquersw@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=dsahern@kernel.org \
--cc=edumazet@google.com \
--cc=haoluo@google.com \
--cc=john.fastabend@gmail.com \
--cc=jolsa@kernel.org \
--cc=kpsingh@kernel.org \
--cc=kuba@kernel.org \
--cc=kuni1840@gmail.com \
--cc=kuniyu@amazon.com \
--cc=martin.lau@linux.dev \
--cc=mykolal@fb.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=sdf@google.com \
--cc=song@kernel.org \
--cc=yonghong.song@linux.dev \
/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).