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 X-Spam-Level: X-Spam-Status: No, score=-5.5 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_PASS,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id AD6FEC43381 for ; Tue, 26 Feb 2019 03:32:30 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 6FC7621848 for ; Tue, 26 Feb 2019 03:32:30 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=fomichev-me.20150623.gappssmtp.com header.i=@fomichev-me.20150623.gappssmtp.com header.b="mS12y3Vw" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726356AbfBZDc3 (ORCPT ); Mon, 25 Feb 2019 22:32:29 -0500 Received: from mail-pg1-f193.google.com ([209.85.215.193]:34369 "EHLO mail-pg1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726294AbfBZDc2 (ORCPT ); Mon, 25 Feb 2019 22:32:28 -0500 Received: by mail-pg1-f193.google.com with SMTP id i130so5519755pgd.1 for ; Mon, 25 Feb 2019 19:32:28 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fomichev-me.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=7Jl3NVzbeEjlbtTFAHMnCd1uKATR7by2vUGV4kB0OVQ=; b=mS12y3VwRK/u5fw5e0Cfcfo6I/xbomVcFo3ePHpAEL/314aEX6BDCG7Dh7cK1UWJ0L O96o2828pusnMjlFCXj5XAbItePznWOpuoXBFoeKdYBEURHVNgEhVjDCIEt4Mrz5qIrN xuy0AQlRDOQ5gjmVyBZ1GcE7SHRcph8+MMCM9ZpppwT9JyJnpWGIjbgFggiwG69JkeF1 /kjwYbnwZ+0Cpgg+0IQ6LCbHOVwIwmywfhFrqemqM3xUmUCTRPjEKNpztl08I2BUqprG 9z2DTLq9rvwWnM6272DCU5AoXK0EYcZgZDAaKuqSA6JBB0dUs4eOxpW97HTMdLYU7LvP /F/A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=7Jl3NVzbeEjlbtTFAHMnCd1uKATR7by2vUGV4kB0OVQ=; b=BwIz9y1935xiAGMTxfKlU1xeeVj1mS8sAfVW1x9O35pt7m4SRTnI65ynnrQo1p1ySZ mN48M6IDspy8WvXjlT8axXHfQEqkI6b+sKUYrhT+jvsEFQyVTdZK1UrAj0y8KCPExh1Q FbNCEyBEkltYxlRpaxVgHk7K1cLw4T+vxDYm8bnyPsHy0MM0vJ4WWmuXEWxwhysvuZ/T XjiHPLtiNaqosLvGVjcCLEW6mWOFRXYfmyre5NYqmbXW3Qf2bbq+Kalx9WsfxlyvFd3o DZTVl2A8kd34xMP5zVZa8IjPRVNw7AmP+tfim53Y0MnTkpuzsucBjbA+dTCRYECvEh2A GOGg== X-Gm-Message-State: AHQUAubvaLPJFXmvN+6ESuiStF04QgzUdMvHACSpe54mQvZ0Vbz+DBRC MMxoDeFzGtsmLSAkNycS6Jbm4A== X-Google-Smtp-Source: AHgI3IaesYhwBfXWuoAvmzxWaJaP347aaZBHcrIGfrZOFgdqeXb7nXkvbAW0UMXdLf2F3aY81ZF1Zw== X-Received: by 2002:a63:a510:: with SMTP id n16mr3241606pgf.443.1551151947476; Mon, 25 Feb 2019 19:32:27 -0800 (PST) Received: from localhost ([2601:646:8f00:18d9:d0fa:7a4b:764f:de48]) by smtp.gmail.com with ESMTPSA id t12sm21640372pgq.68.2019.02.25.19.32.26 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 25 Feb 2019 19:32:26 -0800 (PST) Date: Mon, 25 Feb 2019 19:32:25 -0800 From: Stanislav Fomichev To: Martin Lau Cc: Lawrence Brakmo , netdev , Alexei Starovoitov , Daniel Borkmann , Eric Dumazet , Kernel Team Subject: Re: [PATCH v2 bpf-next 2/9] bpf: Add bpf helper bpf_tcp_enter_cwr Message-ID: <20190226033225.GE32115@mini-arch> References: <20190223010703.678070-1-brakmo@fb.com> <20190223010703.678070-3-brakmo@fb.com> <20190225231438.GC32115@mini-arch> <20190226012959.4gttysrj3khqumc5@kafai-mbp.dhcp.thefacebook.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190226012959.4gttysrj3khqumc5@kafai-mbp.dhcp.thefacebook.com> User-Agent: Mutt/1.11.3 (2019-02-01) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On 02/26, Martin Lau wrote: > On Mon, Feb 25, 2019 at 03:14:38PM -0800, Stanislav Fomichev wrote: > [ ... ] > > > > > > > To ensure it is only called from BPF_CGROUP_INET_EGRESS, the > > > attr->expected_attach_type must be specified as BPF_CGROUP_INET_EGRESS > > > during load time if the prog uses this new helper. > > > The newly added prog->enforce_expected_attach_type bit will also be set > > > if this new helper is used. This bit is for backward compatibility reason > > > because currently prog->expected_attach_type has been ignored in > > > BPF_PROG_TYPE_CGROUP_SKB. During attach time, > > > prog->expected_attach_type is only enforced if the > > > prog->enforce_expected_attach_type bit is set. > > > i.e. prog->expected_attach_type is only enforced if this new helper > > > is used by the prog. > > > > [ ... ] > > > > @@ -1725,6 +1733,10 @@ static int bpf_prog_attach_check_attach_type(const struct bpf_prog *prog, > > > case BPF_PROG_TYPE_CGROUP_SOCK: > > > case BPF_PROG_TYPE_CGROUP_SOCK_ADDR: > > > return attach_type == prog->expected_attach_type ? 0 : -EINVAL; > > > + case BPF_PROG_TYPE_CGROUP_SKB: > > > + return prog->enforce_expected_attach_type && > > > + prog->expected_attach_type != attach_type ? > > > + -EINVAL : 0; > > > default: > > > return 0; > > > } > [ ... ] > > > > diff --git a/net/core/filter.c b/net/core/filter.c > > > index 97916eedfe69..ca57ef25279c 100644 > > > --- a/net/core/filter.c > > > +++ b/net/core/filter.c > > > @@ -5426,6 +5426,24 @@ static const struct bpf_func_proto bpf_tcp_sock_proto = { > > > .arg1_type = ARG_PTR_TO_SOCK_COMMON, > > > }; > > > > > > +BPF_CALL_1(bpf_tcp_enter_cwr, struct tcp_sock *, tp) > > > +{ > > > + struct sock *sk = (struct sock *)tp; > > > + > > > + if (sk->sk_state == TCP_ESTABLISHED) { > > > + tcp_enter_cwr(sk); > > > + return 0; > > > + } > > > + > > > + return -EINVAL; > > > +} > > > + > > > +static const struct bpf_func_proto bpf_tcp_enter_cwr_proto = { > > > + .func = bpf_tcp_enter_cwr, > > > + .gpl_only = false, > > > + .ret_type = RET_INTEGER, > > > + .arg1_type = ARG_PTR_TO_TCP_SOCK, > > > +}; > > > #endif /* CONFIG_INET */ > > > > > > bool bpf_helper_changes_pkt_data(void *func) > > > @@ -5585,6 +5603,13 @@ cg_skb_func_proto(enum bpf_func_id func_id, struct bpf_prog *prog) > > > #ifdef CONFIG_INET > > > case BPF_FUNC_tcp_sock: > > > return &bpf_tcp_sock_proto; > > > > [...] > > > + case BPF_FUNC_tcp_enter_cwr: > > > + if (prog->expected_attach_type == BPF_CGROUP_INET_EGRESS) { > > > + prog->enforce_expected_attach_type = 1; > > > + return &bpf_tcp_enter_cwr_proto; > > Instead of this back and forth with enforce_expected_attach_type, can we > > just do here: > > > > if (prog->expected_attach_type == BPF_CGROUP_INET_EGRESS) > > return &bpf_tcp_enter_cwr_proto; > > else > > return null; > > > > Wouldn't it have the same effect? > The attr->expected_attach_type is currently ignored (i.e. not checked) > during the bpf load time. But nothing stops you form checking prog->expected_attach_type in the cg_skb_func_proto, right? That is done at the time of loading. So depending on prog->expected_attach_type just return null or non-null and the verifier will take care of the rest. Then, at attach time just make sure we are attaching it to the expected_attach_type. We also should not have any existing use cases for BPF_FUNC_tcp_enter_cwr I suppose. In other words, why something like below won't work? Am I missing something? --- diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index b155cd17c1bd..86dc7cd00f34 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -1678,6 +1678,10 @@ static int bpf_prog_attach_check_attach_type(const struct bpf_prog *prog, case BPF_PROG_TYPE_CGROUP_SOCK: case BPF_PROG_TYPE_CGROUP_SOCK_ADDR: return attach_type == prog->expected_attach_type ? 0 : -EINVAL; + case BPF_PROG_TYPE_CGROUP_SKB: + if (prog->expected_attach_type) + return attach_type == prog->expected_attach_type ? 0 : -EINVAL; + return 0; default: return 0; } diff --git a/net/core/filter.c b/net/core/filter.c index 7559d6835ecb..56f70468fc7a 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -5396,6 +5396,11 @@ cg_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) switch (func_id) { case BPF_FUNC_get_local_storage: return &bpf_get_local_storage_proto; + case BPF_FUNC_tcp_enter_cwr: + if (prog->expected_attach_type == BPF_CGROUP_INET_EGRESS) + return &bpf_tcp_enter_cwr_proto; + else + return NULL; default: return sk_filter_func_proto(func_id, prog); } > > How to avoid breaking backward compatibility without selectively > enforcing prog->expected_attach_type during attach time?