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. > > > > 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.
