diff mbox series

[v2] net/socket: Check cgroup_bpf_enabled() only once in do_sock_getsockopt

Message ID 20240819155627.1367-1-Tze-nan.Wu@mediatek.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series [v2] net/socket: Check cgroup_bpf_enabled() only once in do_sock_getsockopt | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 23 this patch: 23
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 13 maintainers not CCed: andrii@kernel.org eddyz87@gmail.com haoluo@google.com linux-arm-kernel@lists.infradead.org jolsa@kernel.org daniel@iogearbox.net bpf@vger.kernel.org song@kernel.org yonghong.song@linux.dev ast@kernel.org kpsingh@kernel.org martin.lau@linux.dev john.fastabend@gmail.com
netdev/build_clang success Errors and warnings before: 28 this patch: 28
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1009 this patch: 1009
netdev/checkpatch warning WARNING: Argument 'enabled' is not used in function-like macro WARNING: Argument 'optlen' is not used in function-like macro WARNING: line length of 88 exceeds 80 columns WARNING: line length of 89 exceeds 80 columns WARNING: line length of 93 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 24 this patch: 24
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-17 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-18 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18

Commit Message

Tze-nan Wu Aug. 19, 2024, 3:56 p.m. UTC 
The return value from `cgroup_bpf_enabled(CGROUP_GETSOCKOPT)` can change
between the invocations of `BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN` and
`BPF_CGROUP_RUN_PROG_GETSOCKOPT`.

If `cgroup_bpf_enabled(CGROUP_GETSOCKOPT)` changes from "false" to
"true" between the invocations of `BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN` and
`BPF_CGROUP_RUN_PROG_GETSOCKOPT`, `BPF_CGROUP_RUN_PROG_GETSOCKOPT` will
receive an -EFAULT from `__cgroup_bpf_run_filter_getsockopt(max_optlen=0)`
due to `get_user()` was not reached in `BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN`.

Scenario shown as below:

           `process A`                      `process B`
           -----------                      ------------
  BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN
                                            enable CGROUP_GETSOCKOPT
  BPF_CGROUP_RUN_PROG_GETSOCKOPT (-EFAULT)

To prevent this, invoke `cgroup_bpf_enabled()` only once and cache the
result in a newly added local variable `enabled`.
Both `BPF_CGROUP_*` macros in `do_sock_getsockopt` will then check their
condition using the same `enabled` variable as the condition variable,
instead of using the return values from `cgroup_bpf_enabled` called by
themselves as the condition variable(which could yield different results).
This ensures that either both `BPF_CGROUP_*` macros pass the condition
or neither does.

Co-developed-by: Yanghui Li <yanghui.li@mediatek.com>
Signed-off-by: Yanghui Li <yanghui.li@mediatek.com>
Co-developed-by: Cheng-Jui Wang <cheng-jui.wang@mediatek.com>
Signed-off-by: Cheng-Jui Wang <cheng-jui.wang@mediatek.com>
Signed-off-by: Tze-nan Wu <Tze-nan.Wu@mediatek.com>
---

Chagnes from v1 to v2: https://lore.kernel.org/all/20240819082513.27176-1-Tze-nan.Wu@mediatek.com/
  Instead of using cgroup_lock in the fastpath, invoke cgroup_bpf_enabled
  only once and cache the value in the variable `enabled`. `BPF_CGROUP_*`
  macros in do_sock_getsockopt can then both check their condition with
  the same variable, ensuring that either they both passing the condition
  or both do not.

Appreciate for reviewing this!
This patch should make cgroup_bpf_enabled() only using once,
but not sure if "BPF_CGROUP_*" is modifiable?(not familiar with code here)

If it's not, then maybe I can come up another patch like below one:
	+++ b/net/socket.c
	  	int max_optlen __maybe_unused;
	 	const struct proto_ops *ops;
	 	int err;
	+	bool enabled;
	
	 	err = security_socket_getsockopt(sock, level, optname);
	 	if (err)
	 		return err;
	
	-	if (!compat)
	+	enabled = cgroup_bpf_enabled(CGROUP_GETSOCKOPT);
	+   if (!compat && enabled)
			max_optlen = BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen);

But this will cause do_sock_getsockopt calling cgroup_bpf_enabled up to
three times , Wondering which approach will be more acceptable?

---
 include/linux/bpf-cgroup.h | 13 ++++++-------
 net/socket.c               |  9 ++++++---
 2 files changed, 12 insertions(+), 10 deletions(-)

Comments

Kuniyuki Iwashima Aug. 19, 2024, 6:18 p.m. UTC | #1 
From: Tze-nan Wu <Tze-nan.Wu@mediatek.com>
Date: Mon, 19 Aug 2024 23:56:27 +0800
> The return value from `cgroup_bpf_enabled(CGROUP_GETSOCKOPT)` can change
> between the invocations of `BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN` and
> `BPF_CGROUP_RUN_PROG_GETSOCKOPT`.
> 
> If `cgroup_bpf_enabled(CGROUP_GETSOCKOPT)` changes from "false" to
> "true" between the invocations of `BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN` and
> `BPF_CGROUP_RUN_PROG_GETSOCKOPT`, `BPF_CGROUP_RUN_PROG_GETSOCKOPT` will
> receive an -EFAULT from `__cgroup_bpf_run_filter_getsockopt(max_optlen=0)`
> due to `get_user()` was not reached in `BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN`.
> 
> Scenario shown as below:
> 
>            `process A`                      `process B`
>            -----------                      ------------
>   BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN
>                                             enable CGROUP_GETSOCKOPT
>   BPF_CGROUP_RUN_PROG_GETSOCKOPT (-EFAULT)
> 
> To prevent this, invoke `cgroup_bpf_enabled()` only once and cache the
> result in a newly added local variable `enabled`.
> Both `BPF_CGROUP_*` macros in `do_sock_getsockopt` will then check their
> condition using the same `enabled` variable as the condition variable,
> instead of using the return values from `cgroup_bpf_enabled` called by
> themselves as the condition variable(which could yield different results).
> This ensures that either both `BPF_CGROUP_*` macros pass the condition
> or neither does.
> 
> Co-developed-by: Yanghui Li <yanghui.li@mediatek.com>
> Signed-off-by: Yanghui Li <yanghui.li@mediatek.com>
> Co-developed-by: Cheng-Jui Wang <cheng-jui.wang@mediatek.com>
> Signed-off-by: Cheng-Jui Wang <cheng-jui.wang@mediatek.com>
> Signed-off-by: Tze-nan Wu <Tze-nan.Wu@mediatek.com>
> ---
> 
> Chagnes from v1 to v2: https://lore.kernel.org/all/20240819082513.27176-1-Tze-nan.Wu@mediatek.com/
>   Instead of using cgroup_lock in the fastpath, invoke cgroup_bpf_enabled
>   only once and cache the value in the variable `enabled`. `BPF_CGROUP_*`
>   macros in do_sock_getsockopt can then both check their condition with
>   the same variable, ensuring that either they both passing the condition
>   or both do not.
> 
> Appreciate for reviewing this!
> This patch should make cgroup_bpf_enabled() only using once,
> but not sure if "BPF_CGROUP_*" is modifiable?(not familiar with code here)
> 
> If it's not, then maybe I can come up another patch like below one:
> 	+++ b/net/socket.c
> 	  	int max_optlen __maybe_unused;
> 	 	const struct proto_ops *ops;
> 	 	int err;
> 	+	bool enabled;
> 	
> 	 	err = security_socket_getsockopt(sock, level, optname);
> 	 	if (err)
> 	 		return err;
> 	
> 	-	if (!compat)
> 	+	enabled = cgroup_bpf_enabled(CGROUP_GETSOCKOPT);
> 	+   if (!compat && enabled)
> 			max_optlen = BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen);
> 
> But this will cause do_sock_getsockopt calling cgroup_bpf_enabled up to
> three times , Wondering which approach will be more acceptable?
> 
> ---
>  include/linux/bpf-cgroup.h | 13 ++++++-------
>  net/socket.c               |  9 ++++++---
>  2 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
> index fb3c3e7181e6..251632d52fa9 100644
> --- a/include/linux/bpf-cgroup.h
> +++ b/include/linux/bpf-cgroup.h
> @@ -390,20 +390,19 @@ static inline bool cgroup_bpf_sock_enabled(struct sock *sk,
>  	__ret;								       \
>  })
>  
> -#define BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen)			       \
> +#define BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen, enabled)			       \

Please keep \ aligned.  Same for other places.


>  ({									       \
>  	int __ret = 0;							       \
> -	if (cgroup_bpf_enabled(CGROUP_GETSOCKOPT))			       \

Can you assign 'enabled' here to hide its usage in the macro ?


> +	if (enabled)			       \
>  		copy_from_sockptr(&__ret, optlen, sizeof(int));		       \
>  	__ret;								       \
>  })
>  
>  #define BPF_CGROUP_RUN_PROG_GETSOCKOPT(sock, level, optname, optval, optlen,   \
> -				       max_optlen, retval)		       \
> +				       max_optlen, retval, enabled)		       \
>  ({									       \
>  	int __ret = retval;						       \
> -	if (cgroup_bpf_enabled(CGROUP_GETSOCKOPT) &&			       \
> -	    cgroup_bpf_sock_enabled(sock, CGROUP_GETSOCKOPT))		       \
> +	if (enabled && cgroup_bpf_sock_enabled(sock, CGROUP_GETSOCKOPT))		    \
>  		if (!(sock)->sk_prot->bpf_bypass_getsockopt ||		       \
>  		    !INDIRECT_CALL_INET_1((sock)->sk_prot->bpf_bypass_getsockopt, \
>  					tcp_bpf_bypass_getsockopt,	       \
> @@ -518,9 +517,9 @@ static inline int bpf_percpu_cgroup_storage_update(struct bpf_map *map,
>  #define BPF_CGROUP_RUN_PROG_SOCK_OPS(sock_ops) ({ 0; })
>  #define BPF_CGROUP_RUN_PROG_DEVICE_CGROUP(atype, major, minor, access) ({ 0; })
>  #define BPF_CGROUP_RUN_PROG_SYSCTL(head,table,write,buf,count,pos) ({ 0; })
> -#define BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen) ({ 0; })
> +#define BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen, enabled) ({ 0; })
>  #define BPF_CGROUP_RUN_PROG_GETSOCKOPT(sock, level, optname, optval, \
> -				       optlen, max_optlen, retval) ({ retval; })
> +				       optlen, max_optlen, retval, enabled) ({ retval; })
>  #define BPF_CGROUP_RUN_PROG_GETSOCKOPT_KERN(sock, level, optname, optval, \
>  					    optlen, retval) ({ retval; })
>  #define BPF_CGROUP_RUN_PROG_SETSOCKOPT(sock, level, optname, optval, optlen, \
> diff --git a/net/socket.c b/net/socket.c
> index fcbdd5bc47ac..5336a2755bb4 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -2365,13 +2365,16 @@ int do_sock_getsockopt(struct socket *sock, bool compat, int level,
>  	int max_optlen __maybe_unused;
>  	const struct proto_ops *ops;
>  	int err;
> +	bool enabled;

Please keep reverse xmas tree order.
https://docs.kernel.org/process/maintainer-netdev.html#local-variable-ordering-reverse-xmas-tree-rcs


>  
>  	err = security_socket_getsockopt(sock, level, optname);
>  	if (err)
>  		return err;
>  
> -	if (!compat)
> -		max_optlen = BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen);
> +	if (!compat) {
> +		enabled = cgroup_bpf_enabled(CGROUP_GETSOCKOPT);
> +		max_optlen = BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen, enabled);
> +	}
>  
>  	ops = READ_ONCE(sock->ops);
>  	if (level == SOL_SOCKET) {
> @@ -2390,7 +2393,7 @@ int do_sock_getsockopt(struct socket *sock, bool compat, int level,
>  	if (!compat)
>  		err = BPF_CGROUP_RUN_PROG_GETSOCKOPT(sock->sk, level, optname,
>  						     optval, optlen, max_optlen,
> -						     err);
> +						     err, enabled);
>  
>  	return err;
>  }
> -- 
> 2.45.2
Tze-nan Wu Aug. 20, 2024, 10:53 a.m. UTC | #2 
On Mon, 2024-08-19 at 11:18 -0700, Kuniyuki Iwashima wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  From: Tze-nan Wu <Tze-nan.Wu@mediatek.com>
> Date: Mon, 19 Aug 2024 23:56:27 +0800
> > The return value from `cgroup_bpf_enabled(CGROUP_GETSOCKOPT)` can
> change
> > between the invocations of `BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN` and
> > `BPF_CGROUP_RUN_PROG_GETSOCKOPT`.
> > 
> > If `cgroup_bpf_enabled(CGROUP_GETSOCKOPT)` changes from "false" to
> > "true" between the invocations of
> `BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN` and
> > `BPF_CGROUP_RUN_PROG_GETSOCKOPT`, `BPF_CGROUP_RUN_PROG_GETSOCKOPT`
> will
> > receive an -EFAULT from
> `__cgroup_bpf_run_filter_getsockopt(max_optlen=0)`
> > due to `get_user()` was not reached in
> `BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN`.
> > 
> > Scenario shown as below:
> > 
> >            `process A`                      `process B`
> >            -----------                      ------------
> >   BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN
> >                                             enable
> CGROUP_GETSOCKOPT
> >   BPF_CGROUP_RUN_PROG_GETSOCKOPT (-EFAULT)
> > 
> > To prevent this, invoke `cgroup_bpf_enabled()` only once and cache
> the
> > result in a newly added local variable `enabled`.
> > Both `BPF_CGROUP_*` macros in `do_sock_getsockopt` will then check
> their
> > condition using the same `enabled` variable as the condition
> variable,
> > instead of using the return values from `cgroup_bpf_enabled` called
> by
> > themselves as the condition variable(which could yield different
> results).
> > This ensures that either both `BPF_CGROUP_*` macros pass the
> condition
> > or neither does.
> > 
> > Co-developed-by: Yanghui Li <yanghui.li@mediatek.com>
> > Signed-off-by: Yanghui Li <yanghui.li@mediatek.com>
> > Co-developed-by: Cheng-Jui Wang <cheng-jui.wang@mediatek.com>
> > Signed-off-by: Cheng-Jui Wang <cheng-jui.wang@mediatek.com>
> > Signed-off-by: Tze-nan Wu <Tze-nan.Wu@mediatek.com>
> > ---
> > 
> > Chagnes from v1 to v2: 
> https://lore.kernel.org/all/20240819082513.27176-1-Tze-nan.Wu@mediatek.com/
> >   Instead of using cgroup_lock in the fastpath, invoke
> cgroup_bpf_enabled
> >   only once and cache the value in the variable `enabled`.
> `BPF_CGROUP_*`
> >   macros in do_sock_getsockopt can then both check their condition
> with
> >   the same variable, ensuring that either they both passing the
> condition
> >   or both do not.
> > 
> > Appreciate for reviewing this!
> > This patch should make cgroup_bpf_enabled() only using once,
> > but not sure if "BPF_CGROUP_*" is modifiable?(not familiar with
> code here)
> > 
> > If it's not, then maybe I can come up another patch like below one:
> > 	+++ b/net/socket.c
> > 	  	int max_optlen __maybe_unused;
> > 	 	const struct proto_ops *ops;
> > 	 	int err;
> > 	+	bool enabled;
> > 	
> > 	 	err = security_socket_getsockopt(sock, level, optname);
> > 	 	if (err)
> > 	 		return err;
> > 	
> > 	-	if (!compat)
> > 	+	enabled = cgroup_bpf_enabled(CGROUP_GETSOCKOPT);
> > 	+   if (!compat && enabled)
> > 			max_optlen =
> BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen);
> > 
> > But this will cause do_sock_getsockopt calling cgroup_bpf_enabled
> up to
> > three times , Wondering which approach will be more acceptable?
> > 
> > ---
> >  include/linux/bpf-cgroup.h | 13 ++++++-------
> >  net/socket.c               |  9 ++++++---
> >  2 files changed, 12 insertions(+), 10 deletions(-)
> > 
> > diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-
> cgroup.h
> > index fb3c3e7181e6..251632d52fa9 100644
> > --- a/include/linux/bpf-cgroup.h
> > +++ b/include/linux/bpf-cgroup.h
> > @@ -390,20 +390,19 @@ static inline bool
> cgroup_bpf_sock_enabled(struct sock *sk,
> >  	__ret;								
>        \
> >  })
> >  
> > -#define BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen)			
>        \
> > +#define BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen, enabled)		
> 	       \
> 
> Please keep \ aligned.  Same for other places.
> 
> 
> >  ({									
>        \
> >  	int __ret = 0;							
>        \
> > -	if (cgroup_bpf_enabled(CGROUP_GETSOCKOPT))			
>        \
> 
> Can you assign 'enabled' here to hide its usage in the macro ?

 Hi Kuniyuki,

 No Problem, and thanks for your recommendation.
 Version 3 has hide the cgroup_bpf_enabled in macro,
 and fix the coding sytle issue.

 --Tze-nan
>  
> 
> > +	if (enabled)			       \
> >  		copy_from_sockptr(&__ret, optlen, sizeof(int));		
>        \
> >  	__ret;								
>        \
> >  })
> >  
> >  #define BPF_CGROUP_RUN_PROG_GETSOCKOPT(sock, level, optname,
> optval, optlen,   \
> > -				       max_optlen, retval)		
>        \
> > +				       max_optlen, retval, enabled)	
> 	       \
> >  ({									
>        \
> >  	int __ret = retval;						
>        \
> > -	if (cgroup_bpf_enabled(CGROUP_GETSOCKOPT) &&			
>        \
> > -	    cgroup_bpf_sock_enabled(sock, CGROUP_GETSOCKOPT))		
>        \
> > +	if (enabled && cgroup_bpf_sock_enabled(sock,
> CGROUP_GETSOCKOPT))		    \
> >  		if (!(sock)->sk_prot->bpf_bypass_getsockopt ||		
>        \
> >  		    !INDIRECT_CALL_INET_1((sock)->sk_prot-
> >bpf_bypass_getsockopt, \
> >  					tcp_bpf_bypass_getsockopt,	
>        \
> > @@ -518,9 +517,9 @@ static inline int
> bpf_percpu_cgroup_storage_update(struct bpf_map *map,
> >  #define BPF_CGROUP_RUN_PROG_SOCK_OPS(sock_ops) ({ 0; })
> >  #define BPF_CGROUP_RUN_PROG_DEVICE_CGROUP(atype, major, minor,
> access) ({ 0; })
> >  #define BPF_CGROUP_RUN_PROG_SYSCTL(head,table,write,buf,count,pos)
> ({ 0; })
> > -#define BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen) ({ 0; })
> > +#define BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen, enabled) ({ 0; })
> >  #define BPF_CGROUP_RUN_PROG_GETSOCKOPT(sock, level, optname,
> optval, \
> > -				       optlen, max_optlen, retval) ({
> retval; })
> > +				       optlen, max_optlen, retval,
> enabled) ({ retval; })
> >  #define BPF_CGROUP_RUN_PROG_GETSOCKOPT_KERN(sock, level, optname,
> optval, \
> >  					    optlen, retval) ({ retval;
> })
> >  #define BPF_CGROUP_RUN_PROG_SETSOCKOPT(sock, level, optname,
> optval, optlen, \
> > diff --git a/net/socket.c b/net/socket.c
> > index fcbdd5bc47ac..5336a2755bb4 100644
> > --- a/net/socket.c
> > +++ b/net/socket.c
> > @@ -2365,13 +2365,16 @@ int do_sock_getsockopt(struct socket *sock,
> bool compat, int level,
> >  	int max_optlen __maybe_unused;
> >  	const struct proto_ops *ops;
> >  	int err;
> > +	bool enabled;
> 
> Please keep reverse xmas tree order.
> 
https://docs.kernel.org/process/maintainer-netdev.html#local-variable-ordering-reverse-xmas-tree-rcs
> 
> 
> >  
> >  	err = security_socket_getsockopt(sock, level, optname);
> >  	if (err)
> >  		return err;
> >  
> > -	if (!compat)
> > -		max_optlen = BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen);
> > +	if (!compat) {
> > +		enabled = cgroup_bpf_enabled(CGROUP_GETSOCKOPT);
> > +		max_optlen = BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen,
> enabled);
> > +	}
> >  
> >  	ops = READ_ONCE(sock->ops);
> >  	if (level == SOL_SOCKET) {
> > @@ -2390,7 +2393,7 @@ int do_sock_getsockopt(struct socket *sock,
> bool compat, int level,
> >  	if (!compat)
> >  		err = BPF_CGROUP_RUN_PROG_GETSOCKOPT(sock->sk, level,
> optname,
> >  						     optval, optlen,
> max_optlen,
> > -						     err);
> > +						     err, enabled);
> >  
> >  	return err;
> >  }
> > -- 
> > 2.45.2
>
diff mbox series

Patch

diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
index fb3c3e7181e6..251632d52fa9 100644
--- a/include/linux/bpf-cgroup.h
+++ b/include/linux/bpf-cgroup.h
@@ -390,20 +390,19 @@  static inline bool cgroup_bpf_sock_enabled(struct sock *sk,
 	__ret;								       \
 })
 
-#define BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen)			       \
+#define BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen, enabled)			       \
 ({									       \
 	int __ret = 0;							       \
-	if (cgroup_bpf_enabled(CGROUP_GETSOCKOPT))			       \
+	if (enabled)			       \
 		copy_from_sockptr(&__ret, optlen, sizeof(int));		       \
 	__ret;								       \
 })
 
 #define BPF_CGROUP_RUN_PROG_GETSOCKOPT(sock, level, optname, optval, optlen,   \
-				       max_optlen, retval)		       \
+				       max_optlen, retval, enabled)		       \
 ({									       \
 	int __ret = retval;						       \
-	if (cgroup_bpf_enabled(CGROUP_GETSOCKOPT) &&			       \
-	    cgroup_bpf_sock_enabled(sock, CGROUP_GETSOCKOPT))		       \
+	if (enabled && cgroup_bpf_sock_enabled(sock, CGROUP_GETSOCKOPT))		    \
 		if (!(sock)->sk_prot->bpf_bypass_getsockopt ||		       \
 		    !INDIRECT_CALL_INET_1((sock)->sk_prot->bpf_bypass_getsockopt, \
 					tcp_bpf_bypass_getsockopt,	       \
@@ -518,9 +517,9 @@  static inline int bpf_percpu_cgroup_storage_update(struct bpf_map *map,
 #define BPF_CGROUP_RUN_PROG_SOCK_OPS(sock_ops) ({ 0; })
 #define BPF_CGROUP_RUN_PROG_DEVICE_CGROUP(atype, major, minor, access) ({ 0; })
 #define BPF_CGROUP_RUN_PROG_SYSCTL(head,table,write,buf,count,pos) ({ 0; })
-#define BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen) ({ 0; })
+#define BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen, enabled) ({ 0; })
 #define BPF_CGROUP_RUN_PROG_GETSOCKOPT(sock, level, optname, optval, \
-				       optlen, max_optlen, retval) ({ retval; })
+				       optlen, max_optlen, retval, enabled) ({ retval; })
 #define BPF_CGROUP_RUN_PROG_GETSOCKOPT_KERN(sock, level, optname, optval, \
 					    optlen, retval) ({ retval; })
 #define BPF_CGROUP_RUN_PROG_SETSOCKOPT(sock, level, optname, optval, optlen, \
diff --git a/net/socket.c b/net/socket.c
index fcbdd5bc47ac..5336a2755bb4 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -2365,13 +2365,16 @@  int do_sock_getsockopt(struct socket *sock, bool compat, int level,
 	int max_optlen __maybe_unused;
 	const struct proto_ops *ops;
 	int err;
+	bool enabled;
 
 	err = security_socket_getsockopt(sock, level, optname);
 	if (err)
 		return err;
 
-	if (!compat)
-		max_optlen = BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen);
+	if (!compat) {
+		enabled = cgroup_bpf_enabled(CGROUP_GETSOCKOPT);
+		max_optlen = BPF_CGROUP_GETSOCKOPT_MAX_OPTLEN(optlen, enabled);
+	}
 
 	ops = READ_ONCE(sock->ops);
 	if (level == SOL_SOCKET) {
@@ -2390,7 +2393,7 @@  int do_sock_getsockopt(struct socket *sock, bool compat, int level,
 	if (!compat)
 		err = BPF_CGROUP_RUN_PROG_GETSOCKOPT(sock->sk, level, optname,
 						     optval, optlen, max_optlen,
-						     err);
+						     err, enabled);
 
 	return err;
 }