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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 6 Nov 2021 02:18:29 +0530
From:   Kumar Kartikeya Dwivedi <memxor@...il.com>
To:     Florian Westphal <fw@...len.de>
Cc:     Toke Høiland-Jørgensen <toke@...hat.com>,
        bpf@...r.kernel.org, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        Maxim Mikityanskiy <maximmi@...dia.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        netdev@...r.kernel.org, netfilter-devel@...r.kernel.org
Subject: Re: [PATCH RFC bpf-next v1 5/6] net: netfilter: Add unstable CT
 lookup helper for XDP and TC-BPF

On Wed, Nov 03, 2021 at 02:13:58AM IST, Florian Westphal wrote:
> Toke Høiland-Jørgensen <toke@...hat.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".
>

FYI, there's also an example in the original series (grep for bpf_ct_lookup_tcp):
https://lore.kernel.org/bpf/20211019144655.3483197-11-maximmi@nvidia.com

> > > 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().

>From my reading, it was safe but not correct to use (as in dereference) without
using nf_conntrack_find_get, since even though freeing of underlying memory is
done using SLAB_DESTROY_BY_RCU, but the nf_conn itself may not correspond to the
same tuple in the rcu read section without taking a reference. So doing what the
example currently does (checking ct->status & IPS_CONFIRMED_BIT) is not safe
without raising the reference, even though the XDP program invocation is under
RCU protection. Returning a PTR_TO_BTF_ID for the nf_conn wouldn't really work
without getting a reference on it, since the object can be recycled.

--
Kartikeya