[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 6 Jun 2023 10:08:42 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Maninder Singh <maninder1.s@...sung.com>
Cc: ast@...nel.org, daniel@...earbox.net, john.fastabend@...il.com,
andrii@...nel.org, martin.lau@...ux.dev, song@...nel.org,
yhs@...com, kpsingh@...nel.org, sdf@...gle.com, haoluo@...gle.com,
jolsa@...nel.org, thunder.leizhen@...wei.com, mcgrof@...nel.org,
boqun.feng@...il.com, vincenzopalazzodev@...il.com,
ojeda@...nel.org, jgross@...e.com, brauner@...nel.org,
michael.christie@...cle.com, samitolvanen@...gle.com,
glider@...gle.com, peterz@...radead.org, keescook@...omium.org,
stephen.s.brennan@...cle.com, alan.maguire@...cle.com,
pmladek@...e.com, linux-kernel@...r.kernel.org,
bpf@...r.kernel.org, Onkarnath <onkarnath.1@...sung.com>
Subject: Re: [PATCH v4 2/3] bpf: make bpf_dump_raw_ok() based on CONFIG_KALLSYMS
On Mon, Jun 5, 2023 at 9:28 PM Maninder Singh <maninder1.s@...sung.com> wrote:
>
> bpf_dump_raw_ok() depends on kallsyms_show_value() and we already
> have a false definition for the !CONFIG_KALLSYMS case. But we'll
> soon expand on kallsyms_show_value() and so to make the code
> easier to follow just provide a direct !CONFIG_KALLSYMS definition
> for bpf_dump_raw_ok() as well.
I'm sorry, I'm failing to follow the exact reasoning about
simplification. It seems simpler to have
static inline bool kallsyms_show_value(const struct cred *cred)
{
return false;
}
and control it from kallsyms-related internal header, rather than
adding CONFIG_KALLSYMS ifdef-ery to include/linux/filter.h and
redefining that `return false` decision. What if in the future we
decide that if !CONFIG_KALLSYMS it's ok to show raw addresses, now
we'll have to remember to update it in two places.
Unless I'm missing some other complications?
>
> Co-developed-by: Onkarnath <onkarnath.1@...sung.com>
> Signed-off-by: Onkarnath <onkarnath.1@...sung.com>
> Signed-off-by: Maninder Singh <maninder1.s@...sung.com>
> Reviewed-by: Luis Chamberlain <mcgrof@...nel.org>
> ---
> include/linux/filter.h | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index bbce89937fde..1f237a3bb11a 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -923,13 +923,21 @@ bool bpf_jit_supports_kfunc_call(void);
> bool bpf_jit_supports_far_kfunc_call(void);
> bool bpf_helper_changes_pkt_data(void *func);
>
> +/*
> + * Reconstruction of call-sites is dependent on kallsyms,
> + * thus make dump the same restriction.
> + */
> +#ifdef CONFIG_KALLSYMS
> static inline bool bpf_dump_raw_ok(const struct cred *cred)
> {
> - /* Reconstruction of call-sites is dependent on kallsyms,
> - * thus make dump the same restriction.
> - */
> return kallsyms_show_value(cred);
> }
> +#else
> +static inline bool bpf_dump_raw_ok(const struct cred *cred)
> +{
> + return false;
> +}
> +#endif
>
> struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
> const struct bpf_insn *patch, u32 len);
> --
> 2.17.1
>
Powered by blists - more mailing lists