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.
>
> 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
>
> .
