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) > > 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. struct dynptr on the stack isn't by itself refcounted. E.g., if we have dynptr pointing to PTR_TO_MAP_VALUE there is no refcounting involved and we don't have to do bpf_dynptr_put(). The really refcounted case is malloc()'ed memory pointed to by dynptr. But in this case refcount is stored next to the actual memory, not inside struct bpf_dynptr. So when we do bpf_dynptr_put() on local struct dynptr copy, it decrements refcount of malloc()'ed memory. If it was the last refcnt, then memory is freed. But we can still have other copies (e.g., in another on-the-stack struct bpf_dynptr copy of BPF program that runs on another CPU, or inside the map value) which will keep allocated memory. bpf_dynptr_put() are just saying "we are done with our local instance of struct bpf_dynptr and that slot can be reused for something else". So Clang deciding to reuse that stack slot for something unrelated after bpf_dynptr_put() should be fine.
