On Mon, Dec 20, 2021 at 02:56:45AM IST, Alexei Starovoitov wrote:

On Mon, Dec 20, 2021 at 01:26:03AM +0530, Kumar Kartikeya Dwivedi wrote:

quoted

quoted

The goal is clear now, but look at it differently:
struct nf_conn *ct = bpf_xdp_ct_lookup(...);
if (ct) {
  struct nf_conn *master = ct->master;
  struct net *net = ct->ct_net.net;

  bpf_ct_release(ct);
  master->status; // prevent this ?
  net->ifindex;   // but allow this ?

I think both will be prevented with the current logic, no?
net will be ct + offset, so if mark_btf_ld_reg writes PTR_TO_BTF_ID to dst_reg
for net, it will copy ct's reg's ref_obj_id to parent_ref_obj_id of dst_reg (net).
Then on release of ct, net's reg gets killed too since reg[ct]->ref_obj_id
matches its parent_ref_obj_id.

Excatly, but it should be allowed.
There is nothing wrong with 'net' access after ct_release.

Ok, I see your point. I'll just drop this patch in v5, and we'll revisit the
other pkt pointer thing when the patch is posted.

[...]

quoted

Very interesting idea! I'm guessing we'll need something akin to bpf_timer
support, i.e. a dedicated type verified using BTF which can be embedded in
map_value? I'll be happy to work on enabling this.

Thanks! Would be awesome.

quoted

One thought though (just confirming):
If user does map_value->saved_ct = ct, we have to ignore reference leak check
for ct's ref_id, but if they rewrite saved_ct, we would also have to unignore
it, correct?

We cannot just ignore it :)
I was thinking to borrow std::unique_ptr like semanitcs.

struct nf_conn *ct = bpf_xdp_ct_lookup(...); // here ref checking logic tracks it as normal
map_value->saved_ct = ct; // here it trasnfers the ref from Rx into map_value
ct->status; // cannot be access here.

It could look unnatural to typical C programmer, so we might need
explicit std::move-like helper, so the assignment will be:
bpf_move_ptr(&map_value->saved_ct, &ct); // same as map_value->saved_ct = ct; ct = NULL;
...
bpf_move_ptr(&ct, &map_value->saved_ct); // would take the ownership back from the map
// and the ref checking logic tracks 'ct' again as normal

Agreed, normal assignment syntax having those side effects is indeed awkward.

quoted

I think we can make this tracking easier by limiting to one bpf_ptr_to_btf
struct in map_value, then it can simply be part of ptr_to_map_value's reg_state.

Possible. Hopefully such limitiation will not be needed.

Thanks for your review and feedback, Alexei! I'll address all points.

--
Kartikeya