On Tue, Oct 25, 2022 at 09:57:58PM IST, Dave Marchevsky wrote:
> On 10/19/22 8:48 PM, Kumar Kartikeya Dwivedi wrote:
> > On Wed, Oct 19, 2022 at 10:45:22PM IST, Dave Marchevsky wrote:
> >> On 10/13/22 2:22 AM, Kumar Kartikeya Dwivedi wrote:
> >>> Introduce the idea of local kptrs, i.e. PTR_TO_BTF_ID that point to a
> >>> type in program BTF. This is indicated by the presence of MEM_TYPE_LOCAL
> >>> type tag in reg->type to avoid having to check btf_is_kernel when trying
> >>> to match argument types in helpers.
> >>>
> >>> For now, these local kptrs will always be referenced in verifier
> >>> context, hence ref_obj_id == 0 for them is a bug. It is allowed to write
> >>> to such objects, as long fields that are special are not touched
> >>> (support for which will be added in subsequent patches).
> >>>
> >>> No PROBE_MEM handling is hence done since they can never be in an
> >>> undefined state, and their lifetime will always be valid.
> >>>
> >>> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> >>> ---
>
> [...]
>
> >>
> >>
> >> more re: passing entire reg state to btf_struct access:
> >>
> >> In the next patch in the series ("bpf: Recognize bpf_{spin_lock,list_head,list_node} in local kptrs")
> >> you do btf_find_struct_meta(btf, reg->btf_id). I see why you couldn't use 't'
> >> that's passed in here / elsewhere since you need the btf_id for meta lookup.
> >> Perhaps 'btf_type *t' param can be changed to btf_id, eliminating the need
> >> to pass 'reg'.
> >>
> >> Alternatively, since we're already passing reg->btf and result of
> >> btf_type_by_id(reg->btf, reg->btf_id), seems like btf_struct_access
> >> maybe is tied closely enough to reg state that passing reg state
> >> directly and getting rid of extraneous args is cleaner.
> >>
> >
> > So Alexei actually suggested dropping both btf and type arguments and simply
> > pass in the register and get it from there.
> >
> > But one call site threw a wrench in the plan:
> >
> > check_ptr_to_map_access -> btf_struct_access
> >
> > Here, it passes it's own btf and type to simulate access to a map. Maybe I
> > should be creating a dummy register on stack and make it work like that for this
> > particular case? Otherwise all other callers pass in what they have from reg.
>
> Ah, sorry for missing that. Personally I'm not a fan of dummy register on the
> stack. Then if btf_struct_access starts using some reg state that wasn't
> populated in the dummy reg it will be confusing.
Well, it can be initialized the same way it's done for normal regs. memset to 0
and then mark_reg_known_zero, memset zeroes state untouched by
mark_reg_known_zero (reg->parent, reg->live, etc.). which won't matter here.
In general, your point is valid as well, but I think for this particular case we
can get away with it, moreso because this is the only case needing such
adjustment.
Or maybe it can be split into two, with the inner call taking btf and btf_id,
while the outer one passes them in. Then btf_struct_access uses reg to pass them
in, while __btf_struct_access takes them directly.
