Re: [RFC PATCH v9 01/11] bpf: Support getting referenced kptr from struct_ops argument

On 7/24/24 10:00 AM, Amery Hung wrote:

On Tue, Jul 23, 2024 at 5:32 PM Martin KaFai Lau [off-list ref] wrote:

quoted

On 7/14/24 10:51 AM, Amery Hung wrote:

quoted

@@ -21004,6 +21025,13 @@ static int do_check_common(struct bpf_verifier_env *env, int subprog)
               mark_reg_known_zero(env, regs, BPF_REG_1);
       }

+     if (env->prog->type == BPF_PROG_TYPE_STRUCT_OPS) {
+             ctx_arg_info = (struct bpf_ctx_arg_aux *)env->prog->aux->ctx_arg_info;
+             for (i = 0; i < env->prog->aux->ctx_arg_info_size; i++)
+                     if (ctx_arg_info[i].refcounted)
+                             ctx_arg_info[i].ref_obj_id = acquire_reference_state(env, 0);
+     }
+

I think this will miss a case when passing the struct_ops prog ctx (i.e. "__u64
*ctx") to a global subprog. Something like this:

__noinline int subprog_release(__u64 *ctx __arg_ctx)
{
         struct task_struct *task = (struct task_struct *)ctx[1];
         int dummy = (int)ctx[0];

         bpf_task_release(task);

         return dummy + 1;
}

SEC("struct_ops/subprog_ref")
__failure
int test_subprog_ref(__u64 *ctx)
{
         struct task_struct *task = (struct task_struct *)ctx[1];

         bpf_task_release(task);

         return subprog_release(ctx);;
}

SEC(".struct_ops.link")
struct bpf_testmod_ops subprog_ref = {
         .test_refcounted = (void *)test_subprog_ref,
};

Thanks for pointing this out. The test did failed.

quoted

A quick thought is, I think tracking the ctx's ref id in the env->cur_state may
not be the correct place.

I think it is a bit tricky because subprogs are checked independently
and their state is folded (i.e., there can be multiple edges from the
main program to a subprog).

Maybe the verifier can rewrite the program: set the refcounted ctx to
NULL when releasing reference. Then, in do_check_common(), if it is a
global subprog, we mark refcounted ctx as PTR_MAYBE_NULL to force a
runtime check. How does it sound?

don't know how to get the ctx pointer to patch the code. It is not always in r1.

A case like this should still break even with the PTR_MAYBE_NULL marking in all 
main and subprog (I haven't tried this one myself):

SEC("struct_ops/subprog_ref")
int test_subprog_ref(__u64 *ctx)
{
	struct task_struct *task = (struct task_struct *)ctx[1];

	if (task) {
		subprog_release(ctx);
		bpf_task_release(task);
	}

	return;
}

afaik, the global subprog is checked independently from the main prog and it 
does not know the state of the main prog. Take a look at the subprog_is_global() 
case in the check_func_call().

How about only acquire_reference_state() for the main prog? Yes, the global 
subprog cannot do the bpf_kptr_xchg() and bpf_qdisc_skb_drop() but it can still 
read the skb. The non-global subprog (static) should work though (please test).

I don't have other better idea. May be Alexei can provide some guidance here?