On Thu, Mar 18, 2021 at 08:12:36PM -0700, Yonghong Song wrote:
> -static inline void bpf_cgroup_storage_set(struct bpf_cgroup_storage
> - *storage[MAX_BPF_CGROUP_STORAGE_TYPE])
> +static inline int bpf_cgroup_storage_set(struct bpf_cgroup_storage
> + *storage[MAX_BPF_CGROUP_STORAGE_TYPE])
> {
> enum bpf_cgroup_storage_type stype;
> + int i;
> +
> + preempt_disable();
> + for (i = 0; i < BPF_CGROUP_STORAGE_NEST_MAX; i++) {
> + if (unlikely(this_cpu_read(bpf_cgroup_storage_info[i].task) != NULL))
> + continue;
> +
> + this_cpu_write(bpf_cgroup_storage_info[i].task, current);
> + for_each_cgroup_storage_type(stype)
> + this_cpu_write(bpf_cgroup_storage_info[i].storage[stype],
> + storage[stype]);
> + break;
> + }
> + preempt_enable();
> +
> + if (i == BPF_CGROUP_STORAGE_NEST_MAX) {
> + WARN_ON_ONCE(1);
> + return -EBUSY;
> + }
> + return 0;
The extra 'if' probably will be optimized by the compiler,
but could you write it like this instead:
+ int err = 0;
..
+ for_each_cgroup_storage_type(stype)
+ this_cpu_write(bpf_cgroup_storage_info[i].storage[stype],
+ storage[stype]);
+ goto out;
+ }
+ err = -EBUSY;
+ WARN_ON_ONCE(1);
+ out:
+ preempt_enable();
+ return err;
Also patch 2 should be squashed into patch 1,
since patch 1 alone makes bpf_prog_test_run() broken.
(The WARN_ON_ONCE should trigger right away on test_run without patch 2).
Another nit:
Is title of the patch "fix NULL pointer dereference" actually correct?
It surely was correct before accidental tracing overwrite was fixed.
But the fix is already in bpf tree.
Do you still see it as NULL deref with that 3 min reproducer?
