On Wed, Jan 29, 2025 at 02:42:41PM -0800, Eduard Zingerman wrote: > On Wed, 2025-01-29 at 22:04 +0000, Peilin Ye wrote: > > Thanks a lot for that; would you mind sharing a bit more on how you > > reasoned about it (i.e., why is it OK to save_aux_ptr_type() > > unconditionally) ? > > Well, save_aux_ptr_type() does two things: > - if there is no env->insn_aux_data[env->insn_idx].ptr_type associated > with the instruction it saves one; > - if there is .ptr_type, it checks if a new one is compatible and > errors out if it's not. > > The .ptr_type is used in convert_ctx_accesses() to rewrite access > instruction (STX/LDX, atomic or not) in a way specific to pointer > type. > > So, doing save_aux_ptr_type() conditionally is already sketchy, > as there is a risk to miss if some instruction is used in a context > where pointer type requires different rewrites. > > convert_ctx_accesses() rewrites instruction for pointer following > types: > - PTR_TO_CTX > - PTR_TO_SOCKET > - PTR_TO_SOCK_COMMON > - PTR_TO_TCP_SOCK > - PTR_TO_XDP_SOCK > - PTR_TO_BTF_ID > - PTR_TO_ARENA > > atomic_ptr_type_ok() allows the following pointer types: > - CONST_PTR_TO_MAP > - PTR_TO_MAP_VALUE > - PTR_TO_MAP_KEY > - PTR_TO_STACK > - PTR_TO_BTF_ID > - PTR_TO_MEM > - PTR_TO_ARENA > - PTR_TO_BUF > - PTR_TO_FUNC > - CONST_PTR_TO_DYNPTR > > One has to check rewrites applied by convert_ctx_accesses() to atomic > instructions to reason about correctness of the conditional > save_aux_ptr_type() call. > > If is_arena_reg() guard is removed from save_aux_ptr_type() we risk to > reject programs that do atomic load/store where same instruction is > used to modify a pointer that can be either of the above types. > I speculate that this is not the problem, as do_check() processing for > BPF_STX/BPF_LDX already calls save_aux_ptr_type() unconditionally. I see, thanks for the explanation! Peilin Ye
