Re: [PATCH bpf-next 1/2] bpf: Remove bpf_jit_enable=2 debugging mode

[Daniel Borkmann <daniel@iogearbox.net>]

On 4/15/21 11:32 AM, Jianlin Lv wrote:
> For debugging JITs, dumping the JITed image to kernel log is discouraged,
> "bpftool prog dump jited" is much better way to examine JITed dumps.
> This patch get rid of the code related to bpf_jit_enable=2 mode and
> update the proc handler of bpf_jit_enable, also added auxiliary
> information to explain how to use bpf_jit_disasm tool after this change.
> 
> Signed-off-by: Jianlin Lv <Jianlin.Lv@arm.com>
[...]
> diff --git a/arch/x86/net/bpf_jit_comp32.c b/arch/x86/net/bpf_jit_comp32.c
> index 0a7a2870f111..8d36b4658076 100644
> --- a/arch/x86/net/bpf_jit_comp32.c
> +++ b/arch/x86/net/bpf_jit_comp32.c
> @@ -2566,9 +2566,6 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
>   		cond_resched();
>   	}
>   
> -	if (bpf_jit_enable > 1)
> -		bpf_jit_dump(prog->len, proglen, pass + 1, image);
> -
>   	if (image) {
>   		bpf_jit_binary_lock_ro(header);
>   		prog->bpf_func = (void *)image;
> diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
> index c8496c1142c9..990b1720c7a4 100644
> --- a/net/core/sysctl_net_core.c
> +++ b/net/core/sysctl_net_core.c
> @@ -273,16 +273,8 @@ static int proc_dointvec_minmax_bpf_enable(struct ctl_table *table, int write,
>   
>   	tmp.data = &jit_enable;
>   	ret = proc_dointvec_minmax(&tmp, write, buffer, lenp, ppos);
> -	if (write && !ret) {
> -		if (jit_enable < 2 ||
> -		    (jit_enable == 2 && bpf_dump_raw_ok(current_cred()))) {
> -			*(int *)table->data = jit_enable;
> -			if (jit_enable == 2)
> -				pr_warn("bpf_jit_enable = 2 was set! NEVER use this in production, only for JIT debugging!\n");
> -		} else {
> -			ret = -EPERM;
> -		}
> -	}
> +	if (write && !ret)
> +		*(int *)table->data = jit_enable;
>   	return ret;
>   }
>   
> @@ -389,7 +381,7 @@ static struct ctl_table net_core_table[] = {
>   		.extra2		= SYSCTL_ONE,
>   # else
>   		.extra1		= SYSCTL_ZERO,
> -		.extra2		= &two,
> +		.extra2		= SYSCTL_ONE,
>   # endif
>   	},
>   # ifdef CONFIG_HAVE_EBPF_JIT
> diff --git a/tools/bpf/bpf_jit_disasm.c b/tools/bpf/bpf_jit_disasm.c
> index c8ae95804728..efa4b17ae016 100644
> --- a/tools/bpf/bpf_jit_disasm.c
> +++ b/tools/bpf/bpf_jit_disasm.c
> @@ -7,7 +7,7 @@
>    *
>    * To get the disassembly of the JIT code, do the following:
>    *
> - *  1) `echo 2 > /proc/sys/net/core/bpf_jit_enable`
> + *  1) Insert bpf_jit_dump() and recompile the kernel to output JITed image into log

Hmm, if we remove bpf_jit_dump(), the next drive-by cleanup patch will be thrown
at bpf@vger stating that bpf_jit_dump() has no in-tree users and should be removed.
Maybe we should be removing bpf_jit_disasm.c along with it as well as bpf_jit_dump()
itself ... I guess if it's ever needed in those rare occasions for JIT debugging we
can resurrect it from old kernels just locally. But yeah, bpftool's jit dump should
suffice for vast majority of use cases.

There was a recent set for ppc32 jit which was merged into ppc tree which will create
a merge conflict with this one [0]. So we would need a rebase and take it maybe during
merge win once the ppc32 landed..

   [0] https://lore.kernel.org/bpf/cover.1616430991.git.christophe.leroy@csgroup.eu/

>    *  2) Load a BPF filter (e.g. `tcpdump -p -n -s 0 -i eth1 host 192.168.20.0/24`)
>    *  3) Run e.g. `bpf_jit_disasm -o` to read out the last JIT code
>    *
> diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c
> index 40a88df275f9..98c7eec2923f 100644
> --- a/tools/bpf/bpftool/feature.c
> +++ b/tools/bpf/bpftool/feature.c
> @@ -203,9 +203,6 @@ static void probe_jit_enable(void)
>   		case 1:
>   			printf("JIT compiler is enabled\n");
>   			break;
> -		case 2:
> -			printf("JIT compiler is enabled with debugging traces in kernel logs\n");
> -			break;

This would still need to be there for older kernels ...

>   		case -1:
>   			printf("Unable to retrieve JIT-compiler status\n");
>   			break;
> 


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv