On Mon, Jun 13, 2022 at 9:14 AM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> 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.
