On Tue, Aug 01, 2023 at 01:36:30PM -0700, Dave Marchevsky wrote:
> +SEC("?fentry.s/bpf_testmod_test_read")
> +__failure __msg("sleepable progs may only spin_{lock,unlock} in RCU CS")
> +int BPF_PROG(rbtree_fail_sleepable_lock_no_rcu,
> + struct file *file, struct kobject *kobj,
> + struct bin_attribute *bin_attr, char *buf, loff_t off, size_t len)
> +{
> + struct node_acquire *n;
> +
> + n = bpf_obj_new(typeof(*n));
> + if (!n)
> + return 0;
> +
> + /* no bpf_rcu_read_{lock,unlock} */
> + bpf_spin_lock(&glock);
> + bpf_rbtree_add(&groot, &n->node, less);
> + bpf_spin_unlock(&glock);
Continuing the discussion in the other patch...
I don't see anything wrong with above.
bpf_spin_lock will disable preemption and will call rbtree_add.
Everything looks safe.
RCU/Design/Requirements/Requirements.rst
"
In addition, anything that disables
preemption also marks an RCU-sched read-side critical section,
including preempt_disable() and preempt_enable(), local_irq_save()
and local_irq_restore(), and so on.
"
In practice it's always better to enforce explicit bpf_rcu_read_lock/unlock,
but seems overkill in this case. Sleepable prog holding spin_lock will
not cause a UAF for non-owned references with refcnt == 0.
