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
*/
