[PATCH bpf-next 0/4] Fix up bpf_jit_limit some more

[PATCH bpf-next 0/4] Fix up bpf_jit_limit some more

[Lorenz Bauer]

Some more cleanups around bpf_jit_limit to make it readable via sysctl.

Things I'm not sure about:
* Is it OK to expose atomic_long_t bpf_jit_limit like this? The sysctl code
  isn't atomic, but maybe it's fine because it's read only.
* All of the JIT related sysctls are quite restrictive, you have to have
  CAP_SYS_ADMIN / CAP_BPF _and_ be root as well. This makes it problematic
  to scrape these to expose them as metrics. Can we relax this somewhat?

Lorenz

Lorenz Bauer (4):
  bpf: define bpf_jit_alloc_exec_limit for riscv JIT
  bpf: define bpf_jit_alloc_exec_limit for arm64 JIT
  bpf: prevent increasing bpf_jit_limit above max
  bpf: export bpf_jit_current

 arch/arm64/net/bpf_jit_comp.c | 5 +++++
 arch/riscv/net/bpf_jit_core.c | 5 +++++
 include/linux/filter.h        | 2 ++
 kernel/bpf/core.c             | 7 ++++---
 net/core/sysctl_net_core.c    | 9 ++++++++-
 5 files changed, 24 insertions(+), 4 deletions(-)

-- 
2.30.2

[PATCH bpf-next 1/4] bpf: define bpf_jit_alloc_exec_limit for riscv JIT

[Lorenz Bauer]

Expose the maximum amount of useable memory from the sparcv JIT.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 arch/riscv/net/bpf_jit_core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/riscv/net/bpf_jit_core.c b/arch/riscv/net/bpf_jit_core.c
index fed86f42dfbe..0fee2cbaaf53 100644
--- a/arch/riscv/net/bpf_jit_core.c
+++ b/arch/riscv/net/bpf_jit_core.c
@@ -166,6 +166,11 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 	return prog;
 }
 
+u64 bpf_jit_alloc_exec_limit(void)
+{
+	return BPF_JIT_REGION_SIZE;
+}
+
 void *bpf_jit_alloc_exec(unsigned long size)
 {
 	return __vmalloc_node_range(size, PAGE_SIZE, BPF_JIT_REGION_START,
-- 
2.30.2

[PATCH bpf-next 2/4] bpf: define bpf_jit_alloc_exec_limit for arm64 JIT

[Lorenz Bauer]

Expose the maximum amount of useable memory from the arm64 JIT.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 arch/arm64/net/bpf_jit_comp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index 41c23f474ea6..803e7773fa86 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -1136,6 +1136,11 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 	return prog;
 }
 
+u64 bpf_jit_alloc_exec_limit(void)
+{
+	return BPF_JIT_REGION_SIZE;
+}
+
 void *bpf_jit_alloc_exec(unsigned long size)
 {
 	return __vmalloc_node_range(size, PAGE_SIZE, BPF_JIT_REGION_START,
-- 
2.30.2

[PATCH bpf-next 3/4] bpf: prevent increasing bpf_jit_limit above max

[Lorenz Bauer]

Restrict bpf_jit_limit to the maximum supported by the arch's JIT.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 include/linux/filter.h     | 1 +
 kernel/bpf/core.c          | 4 +++-
 net/core/sysctl_net_core.c | 2 +-
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index 4a93c12543ee..ef03ff34234d 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -1051,6 +1051,7 @@ extern int bpf_jit_enable;
 extern int bpf_jit_harden;
 extern int bpf_jit_kallsyms;
 extern long bpf_jit_limit;
+extern long bpf_jit_limit_max;
 
 typedef void (*bpf_jit_fill_hole_t)(void *area, unsigned int size);
 
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 6fddc13fe67f..e844a2a4c99a 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -524,6 +524,7 @@ int bpf_jit_enable   __read_mostly = IS_BUILTIN(CONFIG_BPF_JIT_DEFAULT_ON);
 int bpf_jit_kallsyms __read_mostly = IS_BUILTIN(CONFIG_BPF_JIT_DEFAULT_ON);
 int bpf_jit_harden   __read_mostly;
 long bpf_jit_limit   __read_mostly;
+long bpf_jit_limit_max __read_mostly;
 
 static void
 bpf_prog_ksym_set_addr(struct bpf_prog *prog)
@@ -817,7 +818,8 @@ u64 __weak bpf_jit_alloc_exec_limit(void)
 static int __init bpf_jit_charge_init(void)
 {
 	/* Only used as heuristic here to derive limit. */
-	bpf_jit_limit = min_t(u64, round_up(bpf_jit_alloc_exec_limit() >> 2,
+	bpf_jit_limit_max = bpf_jit_alloc_exec_limit();
+	bpf_jit_limit = min_t(u64, round_up(bpf_jit_limit_max >> 2,
 					    PAGE_SIZE), LONG_MAX);
 	return 0;
 }
diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
index c8496c1142c9..5f88526ad61c 100644
--- a/net/core/sysctl_net_core.c
+++ b/net/core/sysctl_net_core.c
@@ -419,7 +419,7 @@ static struct ctl_table net_core_table[] = {
 		.mode		= 0600,
 		.proc_handler	= proc_dolongvec_minmax_bpf_restricted,
 		.extra1		= &long_one,
-		.extra2		= &long_max,
+		.extra2		= &bpf_jit_limit_max,
 	},
 #endif
 	{
-- 
2.30.2

[PATCH bpf-next 4/4] bpf: export bpf_jit_current

[Lorenz Bauer]

Expose bpf_jit_current as a read only value via sysctl.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 include/linux/filter.h     | 1 +
 kernel/bpf/core.c          | 3 +--
 net/core/sysctl_net_core.c | 7 +++++++
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index ef03ff34234d..b2143ad5ce00 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -1052,6 +1052,7 @@ extern int bpf_jit_harden;
 extern int bpf_jit_kallsyms;
 extern long bpf_jit_limit;
 extern long bpf_jit_limit_max;
+extern atomic_long_t bpf_jit_current;
 
 typedef void (*bpf_jit_fill_hole_t)(void *area, unsigned int size);
 
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index e844a2a4c99a..93f95e9ee8be 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -525,6 +525,7 @@ int bpf_jit_kallsyms __read_mostly = IS_BUILTIN(CONFIG_BPF_JIT_DEFAULT_ON);
 int bpf_jit_harden   __read_mostly;
 long bpf_jit_limit   __read_mostly;
 long bpf_jit_limit_max __read_mostly;
+atomic_long_t bpf_jit_current __read_mostly;
 
 static void
 bpf_prog_ksym_set_addr(struct bpf_prog *prog)
@@ -800,8 +801,6 @@ int bpf_jit_add_poke_descriptor(struct bpf_prog *prog,
 	return slot;
 }
 
-static atomic_long_t bpf_jit_current;
-
 /* Can be overridden by an arch's JIT compiler if it has a custom,
  * dedicated BPF backend memory area, or if neither of the two
  * below apply.
diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
index 5f88526ad61c..674aac163b84 100644
--- a/net/core/sysctl_net_core.c
+++ b/net/core/sysctl_net_core.c
@@ -421,6 +421,13 @@ static struct ctl_table net_core_table[] = {
 		.extra1		= &long_one,
 		.extra2		= &bpf_jit_limit_max,
 	},
+	{
+		.procname	= "bpf_jit_current",
+		.data		= &bpf_jit_current,
+		.maxlen		= sizeof(long),
+		.mode		= 0400,
+		.proc_handler	= proc_dolongvec_minmax_bpf_restricted,
+	},
 #endif
 	{
 		.procname	= "netdev_tstamp_prequeue",
-- 
2.30.2

Re: [PATCH bpf-next 1/4] bpf: define bpf_jit_alloc_exec_limit for riscv JIT

[Luke Nelson]

> Expose the maximum amount of useable memory from the sparcv JIT.

sparcv -> riscv?

Otherwise lgtm!

Acked-by: Luke Nelson <luke.r.nels@gmail.com>

Re: [PATCH bpf-next 4/4] bpf: export bpf_jit_current

[Daniel Borkmann]

On 9/24/21 11:55 AM, Lorenz Bauer wrote:
> Expose bpf_jit_current as a read only value via sysctl.
> 
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
>   include/linux/filter.h     | 1 +
>   kernel/bpf/core.c          | 3 +--
>   net/core/sysctl_net_core.c | 7 +++++++
>   3 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index ef03ff34234d..b2143ad5ce00 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -1052,6 +1052,7 @@ extern int bpf_jit_harden;
>   extern int bpf_jit_kallsyms;
>   extern long bpf_jit_limit;
>   extern long bpf_jit_limit_max;
> +extern atomic_long_t bpf_jit_current;
>   
>   typedef void (*bpf_jit_fill_hole_t)(void *area, unsigned int size);
>   
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index e844a2a4c99a..93f95e9ee8be 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -525,6 +525,7 @@ int bpf_jit_kallsyms __read_mostly = IS_BUILTIN(CONFIG_BPF_JIT_DEFAULT_ON);
>   int bpf_jit_harden   __read_mostly;
>   long bpf_jit_limit   __read_mostly;
>   long bpf_jit_limit_max __read_mostly;
> +atomic_long_t bpf_jit_current __read_mostly;
>   
>   static void
>   bpf_prog_ksym_set_addr(struct bpf_prog *prog)
> @@ -800,8 +801,6 @@ int bpf_jit_add_poke_descriptor(struct bpf_prog *prog,
>   	return slot;
>   }
>   
> -static atomic_long_t bpf_jit_current;
> -
>   /* Can be overridden by an arch's JIT compiler if it has a custom,
>    * dedicated BPF backend memory area, or if neither of the two
>    * below apply.
> diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
> index 5f88526ad61c..674aac163b84 100644
> --- a/net/core/sysctl_net_core.c
> +++ b/net/core/sysctl_net_core.c
> @@ -421,6 +421,13 @@ static struct ctl_table net_core_table[] = {
>   		.extra1		= &long_one,
>   		.extra2		= &bpf_jit_limit_max,
>   	},
> +	{
> +		.procname	= "bpf_jit_current",
> +		.data		= &bpf_jit_current,
> +		.maxlen		= sizeof(long),
> +		.mode		= 0400,
> +		.proc_handler	= proc_dolongvec_minmax_bpf_restricted,

Overall series looks good to me. The only nit I would have is that the above could (in theory)
be subject to atomic_long_t vs long type confusion. I would rather prefer to have a small handler
which properly reads out the atomic_long_t and then passes it onwards as a temporary/plain long
to user space.

Thanks,
Daniel

> +	},
>   #endif
>   	{
>   		.procname	= "netdev_tstamp_prequeue",
> 

Re: [PATCH bpf-next 4/4] bpf: export bpf_jit_current

[Jakub Sitnicki]

On Mon, Sep 27, 2021 at 03:34 PM CEST, Daniel Borkmann wrote:
> On 9/24/21 11:55 AM, Lorenz Bauer wrote:
>> Expose bpf_jit_current as a read only value via sysctl.
>> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
>> ---

I find exposing stats via system configuration variables a bit
unexpected. Not sure if there is any example today that we're following.

Maybe an entry under /sys/kernel/debug would be a better fit?

That way we don't have to commit to a sysctl that might go away if we
start charging JIT allocs against memory cgroup quota.

Although that brings up question against which cgroup iptables xt_bpf
allocations should be charged? Root cgroup?

Re: [PATCH bpf-next 4/4] bpf: export bpf_jit_current

[Lorenz Bauer]

On Mon, 27 Sept 2021 at 15:01, Jakub Sitnicki <jakub@cloudflare.com> wrote:
>
> I find exposing stats via system configuration variables a bit
> unexpected. Not sure if there is any example today that we're following.
>
> Maybe an entry under /sys/kernel/debug would be a better fit?
>
> That way we don't have to commit to a sysctl that might go away if we
> start charging JIT allocs against memory cgroup quota.

I had a look around, there are no other obvious places in debugfs or
proc where we already have bpf info exposed. It currently all goes via
sysctl.

There are examples of readonly sysctls:
$ sudo find /proc/sys -perm 0444 | wc -l
90

There are no examples of sysctls with mode 0400 however:
$ sudo find /proc/sys -perm 0400 | wc -l
0

I find it kind of weird that the bpf sysctls are so tightly locked
down (CAP_SYS_ADMIN && root) even for reading. Maybe something I can
change?

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

Re: [PATCH bpf-next 4/4] bpf: export bpf_jit_current

[Nicolas Dichtel]

Le 24/09/2021 à 11:55, Lorenz Bauer a écrit :
> Expose bpf_jit_current as a read only value via sysctl.
bpf_jit_current unit is 'pages' and bpf_jit_limit unit is bytes.
Maybe exposing those values with the same unit will ease debugging.


Regards,
Nicolas

Re: [PATCH bpf-next 2/4] bpf: define bpf_jit_alloc_exec_limit for arm64 JIT

[Will Deacon]

On Fri, Sep 24, 2021 at 10:55:40AM +0100, Lorenz Bauer wrote:
> Expose the maximum amount of useable memory from the arm64 JIT.
> 
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
>  arch/arm64/net/bpf_jit_comp.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index 41c23f474ea6..803e7773fa86 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
> @@ -1136,6 +1136,11 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
>  	return prog;
>  }
>  
> +u64 bpf_jit_alloc_exec_limit(void)
> +{
> +	return BPF_JIT_REGION_SIZE;
> +}

Looks like this won't result in a functional change, as we happen to return
SZ_128M anyway thanks to the way in which the modules area is constructed.

But making this explicit is definitely better, so:

Acked-by: Will Deacon <will@kernel.org>

(I'm assuming this will go via the bpf tree, but please shout if I should
take it via arm64 instead)

Will