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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

> > > 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



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux