On Wed, Apr 27, 2022 at 4:28 PM Joanne Koong <joannelkoong@xxxxxxxxx> wrote: > > On Tue, Apr 26, 2022 at 8:53 PM Andrii Nakryiko > <andrii.nakryiko@xxxxxxxxx> wrote: > > > > On Tue, Apr 26, 2022 at 6:26 PM Alexei Starovoitov > > <alexei.starovoitov@xxxxxxxxx> wrote: > > > > > > On Tue, Apr 26, 2022 at 4:45 PM Joanne Koong <joannelkoong@xxxxxxxxx> wrote: > > > > > > > > > > I guess it's ok to treat refcnted dynptr special like above. > > > > > I wonder whether we can reuse check_reference_leak logic? > > > > I like this idea! My reason for not storing dynptr reference ids in > > > > state->refs was because it's costly (eg we realloc_array every time we > > > > acquire a reference). But thinking about this some more, I like the > > > > idea of keeping everything unified by having all reference ids reside > > > > within state->refs and checking for leaks the same way. Perhaps we can > > > > optimize acquire_reference_state() as well where we upfront allocate > > > > more space for state->refs instead of having to do a realloc_array > > > > every time. > > > > > > realloc is decently efficient underneath. > > > Probably not worth micro optimizing for it. > > > As far as ref state... Looks like dynptr patch is trying > > > hard to prevent writes into the stack area where dynptr > > > was allocated. Then cleans it up after dynptr_put. > > > For other pointers on stack we just mark the area as stack_misc > > > only when the stack slot was overwritten. > > > We don't mark the slot as 'misc' after the pointer was read from stack. > > > We can use the same approach with dynptr as long as dynptr > > > leaking is tracking through ref state > > > (instead of for(each stack slot) at the time of bpf_exit) > I think the trade-off with this is that the verifier error message > will be more ambiguous (eg if you try to call bpf_dynptr_put, the > message would be something like "arg 1 is an unacquired reference" vs. > a more clear-cut message like "direct write into dynptr is not > permitted" at the erring instruction). But I think that's fine. I will > change it to mark the slot as misc for v3. I'm trying to say that "direct write into dynptr is not permitted" could be just as confusing to users, because the store instruction into that stack slot was generated by the compiler because of some optimization and the user has no idea why that code was generated.
