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) iirc we've debugged the case where clang reused stack area with a scalar that was previously used for stack spill. The dynptr on stack won't be seen as stack spill from compiler pov but I worry about the case: struct bpf_dynptr t; bpf_dynptr_alloc(&t,..); bpf_dynptr_put(&t); // compiler thinks the stack area of 't' is dead and reuses // it for something like scalar. Even without dynptr_put above the compiler might see that dynptr_alloc or another function stored something into dynptr, but if nothing is using that dynptr later it might consider the stack area as dead. We cannot mark every dynptr variable as volatile. Another point to consider... This patch unconditionally tells the verifier to unmark_stack_slots_dynptr() after bpf_dynptr_put(). But that's valid only for refcnt=1 -> 0 transition. I'm not sure that will be forever the case even for dynptr-s on stack. If we allows refcnt=2,3,... on stack then the verifier won't be able to clear stack slots after bpf_dynptr_put and we will face the stack reuse issue. I guess the idea is that refcnt-ed dynptr will be only in a map? That might be inconvenient. We allow refcnt-ed kptrs to be in a map, in a register, and spilled to the stack. Surely, dynptr are more complex in that sense.
