On Fri, Aug 14, 2020 at 02:15:54PM -0500, YiFei Zhu wrote:
> @@ -3263,6 +3268,7 @@ static int bpf_prog_get_info_by_fd(struct file *file,
> struct bpf_prog_stats stats;
> char __user *uinsns;
> u32 ulen;
> + const struct bpf_used_maps *used_maps;
> int err;
>
> err = bpf_check_uarg_tail_zero(uinfo, sizeof(info), info_len);
> @@ -3284,19 +3290,24 @@ static int bpf_prog_get_info_by_fd(struct file *file,
> memcpy(info.tag, prog->tag, sizeof(prog->tag));
> memcpy(info.name, prog->aux->name, sizeof(prog->aux->name));
>
> + rcu_read_lock();
> + used_maps = rcu_dereference(prog->aux->used_maps);
> +
> ulen = info.nr_map_ids;
> - info.nr_map_ids = prog->aux->used_map_cnt;
> + info.nr_map_ids = used_maps->cnt;
> ulen = min_t(u32, info.nr_map_ids, ulen);
> if (ulen) {
> u32 __user *user_map_ids = u64_to_user_ptr(info.map_ids);
> u32 i;
>
> for (i = 0; i < ulen; i++)
> - if (put_user(prog->aux->used_maps[i]->id,
> + if (put_user(used_maps->arr[i]->id,
put_user() under rcu_read_lock() is not allowed.
You should see a splat like this:
[ 2.297943] BUG: sleeping function called from invalid context at kernel/bpf/syscall.c:3305
[ 2.305554] in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 81, name: test_progs
[ 2.312802] 1 lock held by test_progs/81:
[ 2.316545] #0: ffffffff82265760 (rcu_read_lock){....}-{1:2}, at: bpf_prog_get_info_by_fd.isra.31+0xb9/0xdf0
[ 2.325446] Preemption disabled at:
[ 2.325454] [<ffffffff812aec64>] __fd_install+0x24/0x2b0
[ 2.333571] CPU: 1 PID: 81 Comm: test_progs Not tainted 5.8.0-ga96d3fdf9 #1
[ 2.339818] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
[ 2.347795] Call Trace:
[ 2.350082] dump_stack+0x78/0xab
[ 2.353109] ___might_sleep+0x17d/0x260
[ 2.356554] __might_fault+0x34/0x90
[ 2.359873] bpf_prog_get_info_by_fd.isra.31+0x21b/0xdf0
[ 2.364674] ? __lock_acquire+0x2e4/0x1df0
[ 2.368377] ? bpf_obj_get_info_by_fd+0x20f/0x3e0
[ 2.372621] bpf_obj_get_info_by_fd+0x20f/0x3e0
Please test your patches with kernel debug flags
like CONFIG_DEBUG_ATOMIC_SLEEP=y at a minimum.
kasan and lockdep are good to have as well.
In general I think rcu here is overkill.
Why not to use mutex everywhere? It's not like it's in critical path.
Also I think better name is needed. ADD_MAP is too specific.
When we get around to implement Daniel's suggestion of replacing
one map with another ADD_MAP as command won't fit.
Single purpose commands are ok, but this one feels too narrow.
May be BPF_PROG_BIND_MAP ?
Then later adding target_map_fd field to prog_bind_map struct would do it.
I thought about BPF_PROG_ATTACH_MAP name, but we use the word "attach"
in too many places and it could be confusing.
