On Thu, Mar 03, 2022 at 04:47:59AM IST, Alexei Starovoitov wrote: > On Wed, Mar 2, 2022 at 3:00 PM Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> wrote: > > > > > > fwiw I like patches 1-3. > > > I think extra check here for release func is justified on its own. > > > Converting it into: > > > fixed_off_ok = false; > > > if (type == PTR_TO_BTF_ID && (!is_release_func || !reg->ref_obj_id)) > > > fixed_off_ok = true; > > > obfuscates the check to me. > > > > I was talking of putting this inside check_func_arg_reg_off. I think we should > > do the same check for BPF helpers as well (rn only one supports releasing > > PTR_TO_BTF_ID, soon we may have others). Just passing a bool to > > check_func_arg_reg_off to indicate we are checking for release func (helper or > > kfunc have same rules here) would allow putting this check inside it. > > Hmm. check_func_arg() is called before we call > is_release_function(func_id). > Are you proposing to call it before and pass > another boolean into check_func_arg() or store the flag in meta? We save meta.func_id before calling check_func_arg. Inside it we can do: err = check_func_arg_reg_off(env, reg, regno, arg_type, is_release_function(meta->func_id)); I actually tried open coding it for BPF helpers, and it was more complicated. If we delay this check until is_release_function call after check_func_arg, we need to remember if reg for whom meta->ref_obj_id had off > 0 and type PTR_TO_BTF_ID. If we put it inside check_reg_type or check_func_arg, you need to call is_release_function anyway there. Compared to these two options, doing it in check_func_arg_reg_off looks better to me, but ymmv. > Sounds ugly. > imo reg->off is a simple enough check to keep it open coded > where necessary. -- Kartikeya
