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:
> > >
> > > 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.
> [...]
> > 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.
>
> > 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.
> > 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
