On Wed, Jul 5, 2023 at 4:49 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> 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?
>
Never mind, I see that you already did, thanks! Catching up :)
>
> > >
> > > 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
> > >
> > > .
> >
> >
