Re: [PATCH bpf-next v7 3/4] bpf: Add kfunc bpf_rcu_read_lock/unlock()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 11/21/22 12:01 PM, Yonghong Song wrote:


On 11/21/22 11:41 AM, Martin KaFai Lau wrote:
On 11/21/22 9:05 AM, Yonghong Song wrote:
@@ -4704,6 +4715,15 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
          return -EACCES;
      }
+    /* Access rcu protected memory */
+    if ((reg->type & MEM_RCU) && env->prog->aux->sleepable &&
+        !env->cur_state->active_rcu_lock) {
+        verbose(env,
+            "R%d is ptr_%s access rcu-protected memory with off=%d, not rcu protected\n",
+            regno, tname, off);
+        return -EACCES;
+    }
+
      if (env->ops->btf_struct_access && !type_is_alloc(reg->type)) {
          if (!btf_is_kernel(reg->btf)) {
              verbose(env, "verifier internal error: reg->btf must be kernel btf\n"); @@ -4731,12 +4751,27 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
      if (ret < 0)
          return ret;
+    /* The value is a rcu pointer. The load needs to be in a rcu lock region,
+     * similar to rcu_dereference().
+     */
+    if ((flag & MEM_RCU) && env->prog->aux->sleepable && !env->cur_state->active_rcu_lock) {
+        verbose(env,
+            "R%d is rcu dereference ptr_%s with off=%d, not in rcu_read_lock region\n",
+            regno, tname, off);
+        return -EACCES;
+    }

Would this make the existing rdonly use case fail?

SEC("fentry.s/" SYS_PREFIX "sys_getpgid")
int task_real_parent(void *ctx)
{
     struct task_struct *task, *real_parent;

     task = bpf_get_current_task_btf();
         real_parent = task->real_parent;
         bpf_printk("pid %u\n", real_parent->pid);
         return 0;
}

Right, it will fail. To fix the issue, user can do
    bpf_rcu_read_lock();
    real_parent = task->real_parent;
    bpf_printk("pid %u\n", real_parent->pid);
    bpf_rcu_read_unlock();

But this raised a good question. How do we deal with
legacy sleepable programs with newly-added rcu tagging
capabilities.

My current option is to error out if rcu usage is not right.
But this might break existing sleepable programs.

Another option intends to not break existing, like above,
codes. In this case, MEM_RCU will not tagged if it is
not inside bpf_rcu_read_lock() region.

hmm.... it is to make MEM_RCU to mean a reg is protected by the current active_rcu_lock or not?

In this case, the above non-rcu-protected code should work. And the
following should work as well although it is a little
bit awkward.
    real_parent = task->real_parent; // real_parent not tagged with rcu
    bpf_rcu_read_lock();
    bpf_printk("pid %u\n", real_parent->pid);
    bpf_rcu_read_unlock();

I think it should be fine. bpf_rcu_read_lock() just not useful in this example but nothing break or crash. Also, after bpf_rcu_read_unlock(), real_parent will continue to be readable because the MEM_RCU is not set?

On top of the active_rcu_lock, should MEM_RCU be set only when it is dereferenced from a PTR_TRUSTED ptr (or with ref_obj_id != 0)?
I am thinking about the following more common case:

	/* bpf_get_current_task_btf() may need to be changed
	 * to set PTR_TRUSTED at the retval?
	 */
	/* task: PTR_TO_BTF_ID | PTR_TRUSTED */
	task = bpf_get_current_task_btf();

	bpf_rcu_read_lock();

	/* real_parent: PTR_TO_BTF_ID | PTR_TRUSTED | MEM_RCU */
	real_parent = task->real_parent;

        /* bpf_task_acquire() needs to change to use refcount_inc_not_zero */
	real_parent = bpf_task_acquire(real_parent);

	bpf_rcu_read_unlock();

	/* real_parent is accessible here (after checking NULL) and
	 * can be passed to kfunc
	 */





[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux