On Mon, Jul 3, 2023 at 6:47 AM Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote:
>
> Hi,
>
> On 6/30/2023 6:35 PM, Dan Carpenter wrote:
> > Hello Andrii Nakryiko,
> >
> > The patch 517bbe1994a3: "bpf: Enforce BPF ringbuf size to be the
> > power of 2" from Jun 29, 2020, leads to the following Smatch static
> > checker warning:
> >
> > kernel/bpf/ringbuf.c:198 ringbuf_map_alloc()
> > warn: impossible condition '(attr->max_entries > 68719464448)'
> >
> > Also Clang warns:
> >
> > kernel/bpf/ringbuf.c:198:24: warning: result of comparison of constant
> > 68719464448 with expression of type '__u32' (aka 'unsigned int') is
> > always false [-Wtautological-constant-out-of-range-compare]
> > if (attr->max_entries > RINGBUF_MAX_DATA_SZ)
> > ~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~
> >
> > kernel/bpf/ringbuf.c
> > 184 static struct bpf_map *ringbuf_map_alloc(union bpf_attr *attr)
> > 185 {
> > 186 struct bpf_ringbuf_map *rb_map;
> > 187
> > 188 if (attr->map_flags & ~RINGBUF_CREATE_FLAG_MASK)
> > 189 return ERR_PTR(-EINVAL);
> > 190
> > 191 if (attr->key_size || attr->value_size ||
> > 192 !is_power_of_2(attr->max_entries) ||
> > 193 !PAGE_ALIGNED(attr->max_entries))
> > 194 return ERR_PTR(-EINVAL);
> > 195
> > 196 #ifdef CONFIG_64BIT
> > 197 /* on 32-bit arch, it's impossible to overflow record's hdr->pgoff */
> > --> 198 if (attr->max_entries > RINGBUF_MAX_DATA_SZ)
> >
> > This check used to be inside bpf_ringbuf_alloc() and it used be:
> >
> > if (data_sz > RINGBUF_MAX_DATA_SZ)
> >
> > In that context where data_sz is a size_t the condition and the
> > #ifdef CONFIG_64BIT made sense but here it doesn't. Probably just
> > delete the check.
> It seems the check before 517bbe1994a3 is only used for future
> extension. Considering the type of max_entries is u32, I think it is OK
> to remove the check and the macro definition.
I'm fine removing this, given page size is always at least 4096,
ringbuf is capable of addressing all 4GBs easily. Hou, will you be
able to send a patch?
> >
> > 199 return ERR_PTR(-E2BIG);
> > 200 #endif
> > 201
> > 202 rb_map = bpf_map_area_alloc(sizeof(*rb_map), NUMA_NO_NODE);
> > 203 if (!rb_map)
> > 204 return ERR_PTR(-ENOMEM);
> > 205
> > 206 bpf_map_init_from_attr(&rb_map->map, attr);
> > 207
> > 208 rb_map->rb = bpf_ringbuf_alloc(attr->max_entries, rb_map->map.numa_node);
> > 209 if (!rb_map->rb) {
> > 210 bpf_map_area_free(rb_map);
> > 211 return ERR_PTR(-ENOMEM);
> > 212 }
> > 213
> > 214 return &rb_map->map;
> > 215 }
> >
> > regards,
> > dan carpenter
> >
> > .
>
>
