Re: [PATCH bpf-next v1 2/3] bpf: Emit verifier warnings through prog stderr

[Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>]

Kumar Kartikeya Dwivedi <memxor@gmail.com> writes:

> There is a class of messages that aren't treated as hard errors, such
> that the program must be rejected, but should be surfaced to the user to
> make them aware that while the program succeeded, parts of their program
> are exhibiting behavior that needs attention. One example is usage of
> kfuncs that are supposed to be deprecated, and may be dropped in the
> future, though are preserved for now for backwards compatibility.
>
> Add support for emitting a warning message to the BPF program's stderr
> stream whenever we detect usage of a _impl suffixed kfunc, which have
> now been replaced with KF_IMPLICIT_ARGS variants. For this purpose,
> introduce bpf_stream_pr_warn() as a convenience wrapper, and then mark
> bpf_find_linfo() as global to allow its usage in the verifier to find
> the linfo corresponding to an instruction index.
>
> To make the message more helpful, recommend usage of bpf_ksym_exists()
> as a way for the user to write backwards compatible BPF code that works
> on older kernels offering _impl suffixed kfuncs.
>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
In the commit message: ..such that the program must __not__ be rejected..
>  include/linux/bpf.h          |  9 ++++++
>  include/linux/bpf_verifier.h |  3 +-
>  kernel/bpf/log.c             |  6 ++--
>  kernel/bpf/verifier.c        | 53 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 67 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 2c4f92085d79..d0158edd27b2 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -3912,6 +3912,15 @@ int bpf_stream_stage_dump_stack(struct bpf_stream_stage *ss);
>  		bpf_stream_stage_free(&ss);                    \
>  	})
>  
> +#define bpf_stream_pr_warn(prog, fmt, ...)                              \
> +	({                                                              \
> +		struct bpf_stream_stage __ss;                           \
> +									\
> +		bpf_stream_stage(__ss, prog, BPF_STDERR, ({		\
> +			bpf_stream_printk(__ss, fmt, ##__VA_ARGS__);	\
> +		}));							\
> +	})
> +
>  #ifdef CONFIG_BPF_LSM
>  void bpf_cgroup_atype_get(u32 attach_btf_id, int cgroup_atype);
>  void bpf_cgroup_atype_put(int cgroup_atype);
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 090aa26d1c98..5683c06f5a90 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -873,7 +873,8 @@ int bpf_vlog_init(struct bpf_verifier_log *log, u32 log_level,
>  		  char __user *log_buf, u32 log_size);
>  void bpf_vlog_reset(struct bpf_verifier_log *log, u64 new_pos);
>  int bpf_vlog_finalize(struct bpf_verifier_log *log, u32 *log_size_actual);
> -
> +const struct bpf_line_info *bpf_find_linfo(const struct bpf_verifier_env *env,
> +					   u32 insn_off);
>  __printf(3, 4) void verbose_linfo(struct bpf_verifier_env *env,
>  				  u32 insn_off,
>  				  const char *prefix_fmt, ...);
> diff --git a/kernel/bpf/log.c b/kernel/bpf/log.c
> index 37d72b052192..598b494ded36 100644
> --- a/kernel/bpf/log.c
> +++ b/kernel/bpf/log.c
> @@ -329,8 +329,8 @@ __printf(2, 3) void bpf_log(struct bpf_verifier_log *log,
>  }
>  EXPORT_SYMBOL_GPL(bpf_log);
>  
> -static const struct bpf_line_info *
> -find_linfo(const struct bpf_verifier_env *env, u32 insn_off)
> +const struct bpf_line_info *
> +bpf_find_linfo(const struct bpf_verifier_env *env, u32 insn_off)
>  {
>  	const struct bpf_line_info *linfo;
>  	const struct bpf_prog *prog;
> @@ -390,7 +390,7 @@ __printf(3, 4) void verbose_linfo(struct bpf_verifier_env *env,
>  		return;
>  
>  	prev_linfo = env->prev_linfo;
> -	linfo = find_linfo(env, insn_off);
> +	linfo = bpf_find_linfo(env, insn_off);
>  	if (!linfo || linfo == prev_linfo)
>  		return;
>  
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 8c1cf2eb6cbb..f50e0ebd0ded 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3206,6 +3206,7 @@ struct bpf_kfunc_desc {
>  	u32 func_id;
>  	s32 imm;
>  	u16 offset;
> +	bool warned_deprecated;
>  	unsigned long addr;
>  };
>  
> @@ -14045,6 +14046,53 @@ static int fetch_kfunc_arg_meta(struct bpf_verifier_env *env,
>  	return 0;
>  }
>  
> +static bool get_insn_file_line(const struct bpf_verifier_env *env, u32 insn_off,
> +			       const char **filep, int *linep)
> +{
> +	const struct bpf_line_info *linfo;
> +
> +	if (!env->prog->aux->btf)
> +		return false;
> +
> +	linfo = bpf_find_linfo(env, insn_off);
> +	if (!linfo)
> +		return false;
> +
> +	if (bpf_get_linfo_file_line(env->prog->aux->btf, linfo, filep, NULL, linep))
> +		return false;
> +	return true;
> +}
> +
> +static void warn_for_deprecated_kfuncs(struct bpf_verifier_env *env,
> +				       struct bpf_kfunc_desc *desc,
> +				       const char *func_name,
> +				       int insn_idx)
> +{
> +	int repl_len, line;
> +	const char *file;
> +
> +	if (desc->warned_deprecated)
> +		return;
> +
> +	if (!func_name || !strends(func_name, KF_IMPL_SUFFIX))
> +		return;
> +
> +	repl_len = strlen(func_name) - strlen(KF_IMPL_SUFFIX);
> +
> +	if (get_insn_file_line(env, insn_idx, &file, &line))
> +		snprintf(env->tmp_str_buf, TMP_STR_BUF_LEN, "%s:%u", file, line);
> +	else
> +		snprintf(env->tmp_str_buf, TMP_STR_BUF_LEN, "insn #%d", insn_idx);
> +
> +	bpf_stream_pr_warn(env->prog,
> +			   "WARNING: %s calls deprecated kfunc %s(), which will be removed.\n"
> +			   "WARNING: Switch to kfunc %.*s() instead.\n"
> +			   "WARNING: For older kernels, choose the kfunc using bpf_ksym_exists(%.*s).\n",
> +			   env->tmp_str_buf, func_name, repl_len, func_name, repl_len, func_name);
> +
> +	desc->warned_deprecated = true;
> +}
Commit message suggests this is a generic functionality that we plan to
use for all kinds of deprecations, but this function looks tightly
coupled to KF_IMPL_SUFFIX functions. I think we can make it a bit
friendlier to extending if split into 2 separate helpers:
is_kfunc_deprecated() and warn_deprecated_kfunc(), so in future new
logic for detecting deprecated funcs goes straight to
is_kfunc_deprecated() and we make sure printing and file/line
extractions are not changed. 
> +
>  /* check special kfuncs and return:
>   *  1  - not fall-through to 'else' branch, continue verification
>   *  0  - fall-through to 'else' branch
> @@ -14231,6 +14279,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  {
>  	bool sleepable, rcu_lock, rcu_unlock, preempt_disable, preempt_enable;
>  	u32 i, nargs, ptr_type_id, release_ref_obj_id;
> +	struct bpf_kfunc_desc *desc;
>  	struct bpf_reg_state *regs = cur_regs(env);
>  	const char *func_name, *ptr_type_name;
>  	const struct btf_type *t, *ptr_type;
> @@ -14253,6 +14302,10 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  	func_name = meta.func_name;
>  	insn_aux = &env->insn_aux_data[insn_idx];
>  
> +	desc = find_kfunc_desc(env->prog, insn->imm, insn->off);
> +	if (desc)
> +		warn_for_deprecated_kfuncs(env, desc, func_name, insn_idx);
> +
>  	insn_aux->is_iter_next = is_iter_next_kfunc(&meta);
>  
>  	if (!insn->off &&
> -- 
> 2.52.0