From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9D124C07E9D for ; Thu, 29 Sep 2022 06:18:25 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232166AbiI2GSY (ORCPT ); Thu, 29 Sep 2022 02:18:24 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50998 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234953AbiI2GSB (ORCPT ); Thu, 29 Sep 2022 02:18:01 -0400 Received: from out0.migadu.com (out0.migadu.com [94.23.1.103]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5355E12968C; Wed, 28 Sep 2022 23:17:44 -0700 (PDT) Message-ID: <4e6e14e4-7301-9981-c52f-7715f26f63f3@linux.dev> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1664432262; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=leoja3tLYvuGjML3vnPhoqeOaWUjA8ZGSBxgJ7z0bZc=; b=eRkSpbv1JCEMJ29ENuARH50C63WgLKS0RxvCyY/UpeZFd14wvusG9LwyUKOAn1gPpG60Zi QRgaQWNWMoN0cMO08AnVSKfyDhfzIp7hpfPNfExtliLDAQjFudQCGadRlL+sz5Ns4vvOey iCErjpaNxLTnZyQVs9z39qD6+/7oDKo= Date: Wed, 28 Sep 2022 23:17:38 -0700 MIME-Version: 1.0 Subject: Re: [PATCH v2 bpf-next 4/5] bpf: tcp: Stop bpf_setsockopt(TCP_CONGESTION) in init ops to recur itself Content-Language: en-US To: Eric Dumazet Cc: bpf , netdev , Alexei Starovoitov , Andrii Nakryiko , Daniel Borkmann , David Miller , Jakub Kicinski , kernel-team , Paolo Abeni References: <20220923224453.2351753-1-kafai@fb.com> <20220923224518.2353383-1-kafai@fb.com> X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Martin KaFai Lau In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Migadu-Flow: FLOW_OUT Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org On 9/28/22 10:37 PM, Eric Dumazet wrote: > On Wed, Sep 28, 2022 at 10:31 PM Martin KaFai Lau wrote: >> >> On 9/28/22 7:04 PM, Eric Dumazet wrote: >>> On Fri, Sep 23, 2022 at 3:48 PM Martin KaFai Lau wrote: >>>> >>>> From: Martin KaFai Lau >>>> >>>> When a bad bpf prog '.init' calls >>>> bpf_setsockopt(TCP_CONGESTION, "itself"), it will trigger this loop: >>>> >>>> .init => bpf_setsockopt(tcp_cc) => .init => bpf_setsockopt(tcp_cc) ... >>>> ... => .init => bpf_setsockopt(tcp_cc). >>>> >>>> It was prevented by the prog->active counter before but the prog->active >>>> detection cannot be used in struct_ops as explained in the earlier >>>> patch of the set. >>>> >>>> In this patch, the second bpf_setsockopt(tcp_cc) is not allowed >>>> in order to break the loop. This is done by using a bit of >>>> an existing 1 byte hole in tcp_sock to check if there is >>>> on-going bpf_setsockopt(TCP_CONGESTION) in this tcp_sock. >>>> >>>> Note that this essentially limits only the first '.init' can >>>> call bpf_setsockopt(TCP_CONGESTION) to pick a fallback cc (eg. peer >>>> does not support ECN) and the second '.init' cannot fallback to >>>> another cc. This applies even the second >>>> bpf_setsockopt(TCP_CONGESTION) will not cause a loop. >>>> >>>> Signed-off-by: Martin KaFai Lau >>>> --- >>>> include/linux/tcp.h | 6 ++++++ >>>> net/core/filter.c | 28 +++++++++++++++++++++++++++- >>>> 2 files changed, 33 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/include/linux/tcp.h b/include/linux/tcp.h >>>> index a9fbe22732c3..3bdf687e2fb3 100644 >>>> --- a/include/linux/tcp.h >>>> +++ b/include/linux/tcp.h >>>> @@ -388,6 +388,12 @@ struct tcp_sock { >>>> u8 bpf_sock_ops_cb_flags; /* Control calling BPF programs >>>> * values defined in uapi/linux/tcp.h >>>> */ >>>> + u8 bpf_chg_cc_inprogress:1; /* In the middle of >>>> + * bpf_setsockopt(TCP_CONGESTION), >>>> + * it is to avoid the bpf_tcp_cc->init() >>>> + * to recur itself by calling >>>> + * bpf_setsockopt(TCP_CONGESTION, "itself"). >>>> + */ >>>> #define BPF_SOCK_OPS_TEST_FLAG(TP, ARG) (TP->bpf_sock_ops_cb_flags & ARG) >>>> #else >>>> #define BPF_SOCK_OPS_TEST_FLAG(TP, ARG) 0 >>>> diff --git a/net/core/filter.c b/net/core/filter.c >>>> index 96f2f7a65e65..ac4c45c02da5 100644 >>>> --- a/net/core/filter.c >>>> +++ b/net/core/filter.c >>>> @@ -5105,6 +5105,9 @@ static int bpf_sol_tcp_setsockopt(struct sock *sk, int optname, >>>> static int sol_tcp_sockopt_congestion(struct sock *sk, char *optval, >>>> int *optlen, bool getopt) >>>> { >>>> + struct tcp_sock *tp; >>>> + int ret; >>>> + >>>> if (*optlen < 2) >>>> return -EINVAL; >>>> >>>> @@ -5125,8 +5128,31 @@ static int sol_tcp_sockopt_congestion(struct sock *sk, char *optval, >>>> if (*optlen >= sizeof("cdg") - 1 && !strncmp("cdg", optval, *optlen)) >>>> return -ENOTSUPP; >>>> >>>> - return do_tcp_setsockopt(sk, SOL_TCP, TCP_CONGESTION, >>>> + /* It stops this looping >>>> + * >>>> + * .init => bpf_setsockopt(tcp_cc) => .init => >>>> + * bpf_setsockopt(tcp_cc)" => .init => .... >>>> + * >>>> + * The second bpf_setsockopt(tcp_cc) is not allowed >>>> + * in order to break the loop when both .init >>>> + * are the same bpf prog. >>>> + * >>>> + * This applies even the second bpf_setsockopt(tcp_cc) >>>> + * does not cause a loop. This limits only the first >>>> + * '.init' can call bpf_setsockopt(TCP_CONGESTION) to >>>> + * pick a fallback cc (eg. peer does not support ECN) >>>> + * and the second '.init' cannot fallback to >>>> + * another. >>>> + */ >>>> + tp = tcp_sk(sk); >>>> + if (tp->bpf_chg_cc_inprogress) >>>> + return -EBUSY; >>>> + >>> >>> Is the socket locked (and owned by current thread) at this point ? >>> If not, changing bpf_chg_cc_inprogress would be racy. >> >> Yes, the socket is locked and owned. There is a sock_owned_by_me check earlier >> in _bpf_setsockopt(). > > Good to know. Note a listener can be cloned without socket lock being held. > > In order to avoid surprises, I would clear bpf_chg_cc_inprogress in > tcp_create_openreq_child() Ah, make sense. I will re-spin.