Re: [PATCH RFC bpf-next v1 5/6] net: netfilter: Add unstable CT lookup helper for XDP and TC-BPF

From: Florian Westphal <fw@strlen.de>
To: "Toke Høiland-Jørgensen" <toke@redhat.com>
Cc: Florian Westphal <fw@strlen.de>,
	Kumar Kartikeya Dwivedi <memxor@gmail.com>,
	bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Yonghong Song <yhs@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	Maxim Mikityanskiy <maximmi@nvidia.com>,
	Jesper Dangaard Brouer <brouer@redhat.com>,
	netdev@vger.kernel.org, netfilter-devel@vger.kernel.org
Subject: Re: [PATCH RFC bpf-next v1 5/6] net: netfilter: Add unstable CT lookup helper for XDP and TC-BPF
Date: Tue, 2 Nov 2021 21:43:58 +0100	[thread overview]
Message-ID: <20211102204358.GC11415@breakpoint.cc> (raw)
In-Reply-To: <87y2677j19.fsf@toke.dk>

Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> > I tried to find a use case but I could not.
> > Entry will time out soon once packets stop appearing, so it can't be
> > used for stack bypass.  Is it for something else?  If so, what?
> 
> I think Maxim's use case was to implement a SYN proxy in XDP, where the
> XDP program just needs to answer the question "do I have state for this
> flow already". For TCP flows terminating on the local box this can be
> done via a socket lookup, but for a middlebox, a conntrack lookup is
> useful. Maxim, please correct me if I got your use case wrong.

Looked at
https://netdevconf.info/0x15/slides/30/Netdev%200x15%20Accelerating%20synproxy%20with%20XDP.pdf

seems thats right, its only a "does it exist".

> > For UDP it will work to let a packet pass through classic forward
> > path once in a while, but this will not work for tcp, depending
> > on conntrack settings (lose mode, liberal pickup etc. pp).
> 
> The idea is certainly to follow up with some kind of 'update' helper. At
> a minimum a "keep this entry alive" update, but potentially more
> complicated stuff as well. Details TBD, input welcome :)

Depends on use case.  For bypass infra I'd target the flowtable
infra rather than conntrack because it gets rid of the "early time out"
problem, plus you get the output interface/dst entry.

Not trivial for xdp because existing code assumes sk_buff.
But I think it can be refactored to allow raw buffers, similar
to flow dissector.

> >> +	hash = nf_conntrack_find_get(net, &nf_ct_zone_dflt, &tuple);
> >
> > Ok, so default zone. Depending on meaning of "unstable helper" this
> > is ok and can be changed in incompatible way later.
> 
> I'm not sure about the meaning of "unstable" either, TBH, but in either
> case I'd rather avoid changing things if we don't have to, so I think
> adding the zone as an argument from the get-go may be better...

Another thing I just noted:
The above gives a nf_conn with incremented reference count.

For Maxims use case, thats unnecessary overhead. Existence can be
determined without reference increment.  The caveat is that the pointer
cannot be used after last rcu_read_unlock().