| Message ID | 20200415140151.439943-1-toke@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf] cpumap: Avoid warning when CONFIG_DEBUG_PER_CPU_MAPS is enabled | expand |
On Wed, 15 Apr 2020 16:01:51 +0200 Toke Høiland-Jørgensen <toke@redhat.com> wrote: > When the kernel is built with CONFIG_DEBUG_PER_CPU_MAPS, the cpumap code > can trigger a spurious warning if CONFIG_CPUMASK_OFFSTACK is also set. This > happens because in this configuration, NR_CPUS can be larger than > nr_cpumask_bits, so the initial check in cpu_map_alloc() is not sufficient > to guard against hitting the warning in cpumask_check(). > > Fix this by using the nr_cpumask_bits variable in the map creation code > instead of the NR_CPUS constant. Shouldn't you use 'nr_cpu_ids' instead of 'nr_cpumask_bits' ? Else this will still fail on systems with CONFIG_CPUMASK_OFFSTACK=n. > > Fixes: 6710e1126934 ("bpf: introduce new bpf cpu map type BPF_MAP_TYPE_CPUMAP") > Cc: Jesper Dangaard Brouer <brouer@redhat.com> > Reported-by: Xiumei Mu <xmu@redhat.com> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > --- > kernel/bpf/cpumap.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c > index 70f71b154fa5..23902afb3bba 100644 > --- a/kernel/bpf/cpumap.c > +++ b/kernel/bpf/cpumap.c > @@ -99,8 +99,8 @@ static struct bpf_map *cpu_map_alloc(union bpf_attr *attr) > > bpf_map_init_from_attr(&cmap->map, attr); > > - /* Pre-limit array size based on NR_CPUS, not final CPU check */ > - if (cmap->map.max_entries > NR_CPUS) { > + /* Pre-limit array size based on nr_cpumask_bits, not final CPU check */ > + if (cmap->map.max_entries > nr_cpumask_bits) { Shouldn't you use 'nr_cpu_ids' instead of 'nr_cpumask_bits' ? > err = -E2BIG; > goto free_cmap; > }
Jesper Dangaard Brouer <brouer@redhat.com> writes: > On Wed, 15 Apr 2020 16:01:51 +0200 > Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >> When the kernel is built with CONFIG_DEBUG_PER_CPU_MAPS, the cpumap code >> can trigger a spurious warning if CONFIG_CPUMASK_OFFSTACK is also set. This >> happens because in this configuration, NR_CPUS can be larger than >> nr_cpumask_bits, so the initial check in cpu_map_alloc() is not sufficient >> to guard against hitting the warning in cpumask_check(). >> >> Fix this by using the nr_cpumask_bits variable in the map creation code >> instead of the NR_CPUS constant. > > Shouldn't you use 'nr_cpu_ids' instead of 'nr_cpumask_bits' ? > > Else this will still fail on systems with CONFIG_CPUMASK_OFFSTACK=n. Jesper and I had an offlist discussion about this, and decided that it's actually better to keep the check in cpu_map_alloc as-is, and instead just check against nr_cpumask_bits in cpu_map_update_elem(). Otherwise, the max-size of the map can vary between machines which can affect BPF program portability. I'll send a v2. -Toke
diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c index 70f71b154fa5..23902afb3bba 100644 --- a/kernel/bpf/cpumap.c +++ b/kernel/bpf/cpumap.c @@ -99,8 +99,8 @@ static struct bpf_map *cpu_map_alloc(union bpf_attr *attr) bpf_map_init_from_attr(&cmap->map, attr); - /* Pre-limit array size based on NR_CPUS, not final CPU check */ - if (cmap->map.max_entries > NR_CPUS) { + /* Pre-limit array size based on nr_cpumask_bits, not final CPU check */ + if (cmap->map.max_entries > nr_cpumask_bits) { err = -E2BIG; goto free_cmap; }
When the kernel is built with CONFIG_DEBUG_PER_CPU_MAPS, the cpumap code can trigger a spurious warning if CONFIG_CPUMASK_OFFSTACK is also set. This happens because in this configuration, NR_CPUS can be larger than nr_cpumask_bits, so the initial check in cpu_map_alloc() is not sufficient to guard against hitting the warning in cpumask_check(). Fix this by using the nr_cpumask_bits variable in the map creation code instead of the NR_CPUS constant. Fixes: 6710e1126934 ("bpf: introduce new bpf cpu map type BPF_MAP_TYPE_CPUMAP") Cc: Jesper Dangaard Brouer <brouer@redhat.com> Reported-by: Xiumei Mu <xmu@redhat.com> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> --- kernel/bpf/cpumap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)