Re: [PATCH bpf-next v4 06/24] bpf: Refactor kptr_off_tab into btf_record

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

On Fri, Nov 04, 2022 at 08:30:28AM IST, Alexei Starovoitov wrote:
> On Fri, Nov 04, 2022 at 12:39:55AM +0530, Kumar Kartikeya Dwivedi wrote:
> >  		else
> > @@ -311,11 +344,12 @@ static inline void __copy_map_value(struct bpf_map *map, void *dst, void *src, b
> >  		return;
> >  	}
> >
> > -	for (i = 0; i < map->off_arr->cnt; i++) {
> > -		u32 next_off = map->off_arr->field_off[i];
> > +	for (i = 0; i < map->field_offs->cnt; i++) {
> > +		u32 next_off = map->field_offs->field_off[i];
> > +		u32 sz = next_off - curr_off;
> >
> > -		memcpy(dst + curr_off, src + curr_off, next_off - curr_off);
> > -		curr_off += map->off_arr->field_sz[i];
> > +		memcpy(dst + curr_off, src + curr_off, sz);
> > +		curr_off += map->field_offs->field_sz[i] + sz;
>
> This is a clear bug. The kernel is crashing with this change.
> How did you test this?
>

For me it is crashing at bpf-next now without this.

When for map value with size 48, having fields at:
off: 0, 16, 32
sz:  4, 16, 16

The above produces:

memcpy(dst + 0, src + 0, 0)
memcpy(dst + 4, src + 4, 12)
memcpy(dst + 32, src + 32, 0)
memcpy(dst + 48, src + 48, 0)

Without it, it becomes:

memcpy(dst + 0, src + 0, 0)
memcpy(dst + 4, src + 4, 12)
memcpy(dst + 20, src + 20, 12)
memcpy(dst + 36, src + 36, 12)

I will send a follow up fix.

> >  	}
> >  	memcpy(dst + curr_off, src + curr_off, map->value_size - curr_off);
> >  }
> > @@ -335,16 +369,17 @@ static inline void zero_map_value(struct bpf_map *map, void *dst)
> >  	u32 curr_off = 0;
> >  	int i;
> >
> > -	if (likely(!map->off_arr)) {
> > +	if (likely(!map->field_offs)) {
> >  		memset(dst, 0, map->value_size);
> >  		return;
> >  	}
> >
> > -	for (i = 0; i < map->off_arr->cnt; i++) {
> > -		u32 next_off = map->off_arr->field_off[i];
> > +	for (i = 0; i < map->field_offs->cnt; i++) {
> > +		u32 next_off = map->field_offs->field_off[i];
> > +		u32 sz = next_off - curr_off;
> >
> > -		memset(dst + curr_off, 0, next_off - curr_off);
> > -		curr_off += map->off_arr->field_sz[i];
> > +		memset(dst + curr_off, 0, sz);
> > +		curr_off += map->field_offs->field_sz[i] + sz;
>
> same thing