On Fri, Nov 04, 2022 at 12:32:41PM IST, Kumar Kartikeya Dwivedi wrote:
> 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.
>
I think this is broken in the original code, I didn't realise this at first but
it's effectively the same in that commit. So it should have Fixes: tag for that.
