bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Daniel Borkmann <daniel@iogearbox.net>
To: Martin KaFai Lau <kafai@fb.com>
Cc: ast@kernel.org, john.fastabend@gmail.com, netdev@vger.kernel.org,
	bpf@vger.kernel.org, David Ahern <dsahern@kernel.org>
Subject: Re: [PATCH bpf-next v3 3/6] bpf: add redirect_neigh helper as redirect drop-in
Date: Wed, 30 Sep 2020 09:58:10 +0200	[thread overview]
Message-ID: <750269bb-0b72-9fd9-bfeb-768213c8618a@iogearbox.net> (raw)
In-Reply-To: <20200930064811.mroafbnrrnb77qki@kafai-mbp.dhcp.thefacebook.com>

On 9/30/20 8:48 AM, Martin KaFai Lau wrote:
> On Tue, Sep 29, 2020 at 11:23:03PM +0200, Daniel Borkmann wrote:
[...]
> 
>> +/* Internal, non-exposed redirect flags. */
>> +enum {
>> +	BPF_F_NEIGH = (1ULL << 1),
>> +};
> It will be useful to ensure the future "flags" of BPF_FUNC_redirect
> will not overlap with this.  May be a BUILD_BUG_ON?

I was thinking about this as well, but didn't go for it since typically this would
mean that we need to add a mask of all flags for redirect helper in uapi right next
to where we define BPF_F_INGRESS such that people don't forget to update the mask
whenever they extend the flags there in order for the BUILD_BUG_ON() assertion to be
actually effective (see also RTAX_FEATURE_MASK vs DST_FEATURE_MASK). If the mask sits
in a different location, then developers might forget to update, it might slip through
review (since not included in diff) and the build failure doesn't trigger. So far we
have avoided to extend bpf uapi in such way. That was basically my rationale, another
option could be to just add a comment in the enum right underneath BPF_F_INGRESS that
the (1ULL << 1) bit is currently kernel-internal.

> Others LGTM.
> 
> Acked-by: Martin KaFai Lau <kafai@fb.com>

Thanks!

  reply	other threads:[~2020-09-30  7:58 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-29 21:23 [PATCH bpf-next v3 0/6] Various BPF helper improvements Daniel Borkmann
2020-09-29 21:23 ` [PATCH bpf-next v3 1/6] bpf: add classid helper only based on skb->sk Daniel Borkmann
2020-09-30  1:24   ` Martin KaFai Lau
2020-09-29 21:23 ` [PATCH bpf-next v3 2/6] bpf, net: rework cookie generator as per-cpu one Daniel Borkmann
2020-09-30  2:05   ` Martin KaFai Lau
2020-09-29 21:23 ` [PATCH bpf-next v3 3/6] bpf: add redirect_neigh helper as redirect drop-in Daniel Borkmann
2020-09-30  6:48   ` Martin KaFai Lau
2020-09-30  7:58     ` Daniel Borkmann [this message]
2020-09-29 21:23 ` [PATCH bpf-next v3 4/6] bpf, libbpf: add bpf_tail_call_static helper for bpf programs Daniel Borkmann
2020-09-29 21:23 ` [PATCH bpf-next v3 5/6] bpf, selftests: use bpf_tail_call_static where appropriate Daniel Borkmann
2020-10-04 19:02   ` bpf_program__set_attach_target(prog, 0, "foo") cannot possibly work ? Luigi Rizzo
2020-09-29 21:23 ` [PATCH bpf-next v3 6/6] bpf, selftests: add redirect_neigh selftest Daniel Borkmann

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=750269bb-0b72-9fd9-bfeb-768213c8618a@iogearbox.net \
    --to=daniel@iogearbox.net \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=dsahern@kernel.org \
    --cc=john.fastabend@gmail.com \
    --cc=kafai@fb.com \
    --cc=netdev@vger.kernel.org \
    /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).