On Wed, Mar 2, 2022 at 3:29 PM Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> wrote: > > 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. I see. Yeah. You're right. check_func_arg_reg_off(env, reg, regno, arg_type, is_release_function(meta->func_id)) is indeed cleaner.
