Re: [PATCH v1 bpf-next 2/7] bpf: Consider non-owning refs trusted

[Yonghong Song <yonghong.song@linux.dev>]

On 8/1/23 1:36 PM, Dave Marchevsky wrote:
> Recent discussions around default kptr "trustedness" led to changes such
> as commit 6fcd486b3a0a ("bpf: Refactor RCU enforcement in the
> verifier."). One of the conclusions of those discussions, as expressed
> in code and comments in that patch, is that we'd like to move away from
> 'raw' PTR_TO_BTF_ID without some type flag or other register state
> indicating trustedness. Although PTR_TRUSTED and PTR_UNTRUSTED flags mark
> this state explicitly, the verifier currently considers trustedness
> implied by other register state. For example, owning refs to graph
> collection nodes must have a nonzero ref_obj_id, so they pass the
> is_trusted_reg check despite having no explicit PTR_{UN}TRUSTED flag.
> This patch makes trustedness of non-owning refs to graph collection
> nodes explicit as well.
> 
> By definition, non-owning refs are currently trusted. Although the ref
> has no control over pointee lifetime, due to non-owning ref clobbering
> rules (see invalidate_non_owning_refs) dereferencing a non-owning ref is
> safe in the critical section controlled by bpf_spin_lock associated with
> its owning collection.
> 
> Note that the previous statement does not hold true for nodes with shared
> ownership due to the use-after-free issue that this series is
> addressing. True shared ownership was disabled by commit 7deca5eae833
> ("bpf: Disable bpf_refcount_acquire kfunc calls until race conditions are fixed"),
> though, so the statement holds for now. Further patches in the series will change
> the trustedness state of non-owning refs before re-enabling
> bpf_refcount_acquire.
> 
> Let's add NON_OWN_REF type flag to BPF_REG_TRUSTED_MODIFIERS such that a
> non-owning ref reg state would pass is_trusted_reg check. Somewhat
> surprisingly, this doesn't result in any change to user-visible
> functionality elsewhere in the verifier: graph collection nodes are all
> marked MEM_ALLOC, which tends to be handled in separate codepaths from
> "raw" PTR_TO_BTF_ID. Regardless, let's be explicit here and document the
> current state of things before changing it elsewhere in the series.
> 
> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>

Acked-by: Yonghong Song <yonghong.song@linux.dev>