netdev - Re: [PATCH v4 bpf-next 00/14] net: netfilter: add kfunc helper to update ct timeout

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAADnVQKk9LPm=4OeosxLZCmv+_PnowPZdz9QP4f-H8Vd4HSLVw@mail.gmail.com>
Date:   Mon, 13 Jun 2022 15:15:00 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Kumar Kartikeya Dwivedi <memxor@...il.com>
Cc:     Lorenzo Bianconi <lorenzo@...nel.org>, bpf <bpf@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Pablo Neira Ayuso <pablo@...filter.org>,
        Florian Westphal <fw@...len.de>,
        netfilter-devel <netfilter-devel@...r.kernel.org>,
        Lorenzo Bianconi <lorenzo.bianconi@...hat.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Yonghong Song <yhs@...com>
Subject: Re: [PATCH v4 bpf-next 00/14] net: netfilter: add kfunc helper to
 update ct timeout

On Mon, Jun 13, 2022 at 9:14 AM Kumar Kartikeya Dwivedi
<memxor@...il.com> wrote:
>
> On Sun, Jun 12, 2022 at 01:41:17AM IST, Alexei Starovoitov wrote:
> > On Thu, May 26, 2022 at 11:34:48PM +0200, Lorenzo Bianconi wrote:
> > > Changes since v3:
> > > - split bpf_xdp_ct_add in bpf_xdp_ct_alloc/bpf_skb_ct_alloc and
> > >   bpf_ct_insert_entry
> > > - add verifier code to properly populate/configure ct entry
> > > - improve selftests
> >
> > Kumar, Lorenzo,
> >
> > are you planning on sending v5 ?
> > The patches 1-5 look good.
> > Patch 6 is questionable as Florian pointed out.
>
> Yes, it is almost there.
>
> > What is the motivation to allow writes into ct->status?
>
> It will only be allowed for ct from alloc function, after that ct = insert(ct)
> releases old one with new read only ct. I need to recheck once again with the
> code what other bits would cause problems on insert before I rework and reply.

I still don't understand why you want to allow writing after alloc.

> > The selftest doesn't do that anyway.
>
> Yes, it wasn't updated, we will do that in v5.
>
> > Patch 7 (acquire-release pairs) is too narrow.
> > The concept of a pair will not work well. There could be two acq funcs and one release.
>
> That is already handled (you define two pairs: acq1, rel and acq2, rel).
> There is also an example: bpf_ct_insert_entry -> bpf_ct_release,
> bpf_xdp_ct_lookup -> ct_release.

If we can represent that without all these additional btf_id sets
it would be much better.

> > Please think of some other mechanism. Maybe type based? BTF?
> > Or encode that info into type name? or some other way.
>
> Hmm, ok. I kinda dislike this solution too. The other idea that comes to mind is
> encapsulating nf_conn into another struct and returning pointer to that:
>
>         struct nf_conn_alloc {
>                 struct nf_conn ct;
>         };
>
>         struct nf_conn_alloc *bpf_xdp_ct_alloc(...);
>         struct nf_conn *bpf_ct_insert_entry(struct nf_conn_alloc *act, ...);
>
> Then nf_conn_alloc gets a different BTF ID, and hence the type can be matched in
> the prototype. Any opinions?

Yes. Or maybe typedef ?
typedef struct nf_conn nf_conn__alloc;
typedef struct nf_conn nf_conn__ro;

C will accept silent type casts from one type to another,
but BTF type checking can be strict?
Not sure. wrapping a struct works too, but extra '.ct' accessor
might be annoying? Unless you only use it with container_of().
I would prefer double or triple underscore to highlight a flavor.
struct nf_conn___init {...}
The main benefit, of course, is no need for extra btf_id sets.
Different types take care of correct arg passing.
In that sense typedef idea doesn't quite work,
since BTF checks with typedef would be unnecessarily strict
compared to regular C type checking rules. That difference
in behavior might bite us later.
So let's go with struct wrapping.