On Fri, Nov 18, 2022 at 11:14:03PM -0600, David Vernet wrote: > On Fri, Nov 18, 2022 at 08:13:37PM -0800, Alexei Starovoitov wrote: > > On Fri, Nov 18, 2022 at 03:44:42PM -0600, David Vernet wrote: > > > > > if it's a release arg it should always have a refcount on it. > > > > > PTR_UNTRUSTED | PTR_TRUSTED would also make no sense. MEM_FIXED_SIZE > > > > > though seems fine? In general, I thought it was prudent for us to take > > > > > the most conservative possible approach here, which is that PTR_TRUSTED > > > > > only applies when no other modifiers are present, and it applies for all > > > > > obj_ptr types (other than PTR_TO_CTX which does its own thing). > > > > > > > > Probably worth refining when PTR_TRUSTED is cleared. > > > > For example adding PTR_UNTRUSTED should definitely clear it. > > > > > > > > > That makes sense for PTR_UNTRUSTED, what about the other type modifiers > > > like PTR_MAYBE_NULL? We set and unset if a ptr is NULL throughout a > > > function, so we'd have to record if it was previously trusted in order > > > to properly re-OR after a NULL check. > > > > PTR_MAYBE_NULL is another bit and I don't think it conflicts with PTR_TRUSTED. > > PTR_TO_BTF_ID | PTR_TRUSTED is a valid pointer. > > PTR_TO_BTF_ID | PTR_TRUSTED | PTR_MAYBE_NULL is a valid pointer or NULL. > > > > PTR_TO_BTF_ID | PTR_MAYBE_NULL is a legacy "valid pointer" or NULL. > > That legacy pointer cannot be passed to KF_TRUSTED_ARGS kfuncs. > > > > KF_TRUSTED_ARGS kfuncs should not accept PTR_TO_BTF_ID | PTR_TRUSTED | PTR_MAYBE_NULL. > > Indeed -- my point was that I don't think e.g. clearing PTR_TRUSTED when > we set PTR_UNTRUSTED will work, at least not yet. It's still too tricky > to find all the places where we'd have to &= ~PTR_TRUSTED or |= > PTR_TRUSTED when setting specific type modifiers. As described below, we > first have to clarify the general workflow to enable the presence of > PTR_TRUSTED to be the single source of truth for trust. Agree. A reg->type with both PTR_TRUSTED and PTR_UNTRUSTED would be a bug, but let's fix it when we get there. Even if such bug hits us we can protect from it by make sure that we treat PTR_UNTRUSTED as logically stronger flag. > > It's a job of the prog to do != NULL check. > > Otherwise all such != NULL checks would need to move inside kfuncs which is not good. > > > > > > MEM_ALLOC flag is probably equivalent to PTR_TRUSTED. > > > > Maybe the bit: > > > > regs[BPF_REG_0].type = PTR_TO_BTF_ID | MEM_ALLOC; > > > > should set PTR_TRUSTED as well? > > > > > > We could, but that changes the meaning of PTR_TRUSTED and IMO makes it > > > harder to reason about. Before it was just "the kernel passed this arg > > > to the program and promises the program that it was trusted when it was > > > first passed". Now it's that plus it could mean that it points to an > > > allocated object from bpf_obj_new()". IMO we should keep all of these > > > modifiers separate so that the presence of a modifier has a well-defined > > > meaning that we can interpret in each context as needed. In this case, > > > we can make trust opt-in, so a KF_TRUSTED_ARGS BTF pointer either of the > > > following: > > > > > > 1. reg->ref_obj_id > 0 > > > 2. Either one of PTR_TRUSTED | MEM_ALLOC type modifiers are set, and no > > > others. > > > > I don't think MEM_ALLOC conflicts with PTR_TRUSTED. > > MEM_ALLOC flags means that it came from bpf_obj_new() and that's what > > bpf_spin_lock and bpf_obj_drop() want to see. > > > > Adding PTR_TRUSTED to MEM_ALLOC looks necessary to me. > > It doesn't have to be done right now, but eventually feels right. > > I think I agree. MEM_ALLOC should always imply PTR_TRUSTED. Ideally we > shouldn't have to check MEM_ALLOC for KF_TRUSTED_ARGS at all, and > PTR_TRUSTED should be the only modifier representing if something is > safe. exactly. > For now I'd prefer to keep them separate until we have a clear > plan, especially with respect to clearing PTR_TRUSTED for when something > unsafe happens like PTR_UNTRUSTED or PTR_MAYBE_NULL. It's all too > muddied still. sure. we can do that in the follow up. A bit more technical debt to address later. > > > I've been thinking whether reg->ref_obj_id > 0 condition should be converted > > to PTR_TRUSTED too... > > On one side it will simplify the check for KF_TRUSTED_ARGS. > > The only thing check_kfunc_args() would need to do is: > > is_kfunc_trusted_args(meta) > > && type_flag(reg->type) & PTR_TRUSTED > > && !(type_flag(reg->type) & PTR_MAYBE_NULL) > > > > On the other side fixing all places where we set ref_obj_id > > and adding |= PTR_TRUSTED may be too cumbersome ? > > I think it's probably too cumbersome now, but yeah, as mentioned above, > I think it's the right direction. I think it will require a lot of > thought to do it right, though. With the code the way that it is now, I > can't convince myself that we wouldn't do something like |= PTR_TRUSTED > when we observe ref_obj_id > 0, and then later &= ~PTR_TRUSTED when > setting PTR_MAYBE_NULL. I think Kumar's latest patch set is a nice step > towards achieving this clearer state. Hopefully we can continue to > improve. > > > Right now we're saying PTR_TO_CTX is implicitly trusted, but we can OR > > PTR_TO_CTX with PTR_TRUSTED to make it explicit and truly generalize the check. > > Further agreed, this is the correct longer-term direction. > > > > Agreed that after the rebase this would no longer be correct. I think we > > > should make it opt-in, though. PTR_TRUSTED | MEM_ALLOC is fine. > > > PTR_TRUSTED | MEM_ALLOC | PTR_MAYBE_NULL would not be. > > > > to pass into KF_TRUSTED_ARGS kfunc? Agree. > > I guess we can tighten the check a bit: > > is_kfunc_trusted_args(meta) > > && type_flag(reg->type) & PTR_TRUSTED > > && !(type_flag(reg->type) & ~(PTR_TRUSTED | MEM_ALLOC)) > > > > In english: the pointer should have PTR_TRUSTED flag and > > no other flags other than PTR_TRUSTED and MEM_ALLOC should be set. > > Yeah, I think this is the correct way to model this for now. Later on > once we refactor things, the presence of PTR_TRUSTED on its own should > be sufficient. A good north star to aim towards. > > I'll send this out as v8 tomorrow. Perfect. Looking forward.
