On Thu, Nov 18, 2021 at 7:32 PM Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote: > > On Tue, Nov 16, 2021 at 05:17:27PM -0800, Andrii Nakryiko wrote: > > > + > > > + rec_size = attr->core_relo_rec_size; > > > + if (rec_size != sizeof(struct bpf_core_relo)) > > > + return -EINVAL; > > > > For func_info we allow trailing zeroes (in check_btf_func, we check > > MIN_BPF_FUNCINFO_SIZE and MAX_FUNCINFO_REC_SIZE). Shouldn't we do > > something like that here? > > For both func_info and line_info the verifier indeed uses 252 as max rec_size. > I believe the logic is trying to help in the following case: > If llvm starts to produce a larger records the libbpf will test for the > new kernel and will sanize the newer records with zeros _in place_. > > In case of bpf_core_relo the implementation of this patch doesn't > take llvm bytes as-is. The relo records saved and copied potentially > many times with every libbpf call relocations. > Then later they are prepared in the final form. > > I don't mind doing the same 252 logic here, but when I wrote above > check it felt more natural to be strict and simple, but... > > > > + > > > + u_core_relo = make_bpfptr(attr->core_relo, uattr.is_kernel); > > > + expected_size = sizeof(struct bpf_core_relo); > > > + ncopy = min_t(u32, expected_size, rec_size); > > > > I'm confused, a few lines above you errored out if expected_size != rec_size... > > but then I kept this part and below bpf_check_uarg_tail_zero(). > Clearly I couldn't make up my mind :) > Heh :) > So drop all future proofing ? or do 252 ? I think allowing bigger record sizes than the kernel knows about (assuming zeroes in the tail) is a bit better, because then sanitization won't require unnecessary memory copying. I'm actually not sure why we restrict record size, if kernel can just copy just the right amount of first X bytes of each record (we do the copy anyways), so it feels like kernel should just make sure that the tail is zeroed, like we do for bpf_attr, for example. So I guess I vote for future proofing.
