diff mbox series

[v2,bpf-next,5/5] selftests/bpf: Update sockopt_sk test to the use bpf_set_retval

Message ID 4f20b77cb46812dbc2bdcd7e3fa87c7573bde55e.1639619851.git.zhuyifei@google.com (mailing list archive)
State Accepted
Delegated to: BPF
Headers show
Series bpf: allow cgroup progs to export custom retval to userspace | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for bpf-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 9 maintainers not CCed: netdev@vger.kernel.org kafai@fb.com shuah@kernel.org songliubraving@fb.com john.fastabend@gmail.com kpsingh@kernel.org yauheni.kaliuta@redhat.com yhs@fb.com linux-kselftest@vger.kernel.org
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 110 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next success VM_Test
bpf/vmtest-bpf-next-PR success PR summary

Commit Message

YiFei Zhu Dec. 16, 2021, 2:04 a.m. UTC 
The tests would break without this patch, because at one point it calls
  getsockopt(fd, SOL_TCP, TCP_ZEROCOPY_RECEIVE, &buf, &optlen)
This getsockopt receives the kernel-set -EINVAL. Prior to this patch
series, the eBPF getsockopt hook's -EPERM would override kernel's
-EINVAL, however, after this patch series, return 0's automatic
-EPERM will not; the eBPF prog has to explicitly bpf_set_retval(-EPERM)
if that is wanted.

I also removed the explicit mentions of EPERM in the comments in the
prog.

Signed-off-by: YiFei Zhu <zhuyifei@google.com>
Reviewed-by: Stanislav Fomichev <sdf@google.com>
---
 .../selftests/bpf/prog_tests/sockopt_sk.c     |  2 +-
 .../testing/selftests/bpf/progs/sockopt_sk.c  | 32 +++++++++----------
 2 files changed, 17 insertions(+), 17 deletions(-)

Comments

Andrii Nakryiko Dec. 21, 2021, 11:13 p.m. UTC | #1 
On Wed, Dec 15, 2021 at 6:04 PM YiFei Zhu <zhuyifei@google.com> wrote:
>
> The tests would break without this patch, because at one point it calls

Please fix selftests in the same patch where kernel change breaks it
to ensure a proper "bisectability" of the code.

>   getsockopt(fd, SOL_TCP, TCP_ZEROCOPY_RECEIVE, &buf, &optlen)
> This getsockopt receives the kernel-set -EINVAL. Prior to this patch
> series, the eBPF getsockopt hook's -EPERM would override kernel's
> -EINVAL, however, after this patch series, return 0's automatic
> -EPERM will not; the eBPF prog has to explicitly bpf_set_retval(-EPERM)
> if that is wanted.
>
> I also removed the explicit mentions of EPERM in the comments in the
> prog.
>
> Signed-off-by: YiFei Zhu <zhuyifei@google.com>
> Reviewed-by: Stanislav Fomichev <sdf@google.com>
> ---
>  .../selftests/bpf/prog_tests/sockopt_sk.c     |  2 +-
>  .../testing/selftests/bpf/progs/sockopt_sk.c  | 32 +++++++++----------
>  2 files changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c b/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
> index 4b937e5dbaca..164aa5020bf1 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
> @@ -177,7 +177,7 @@ static int getsetsockopt(void)
>         optlen = sizeof(buf.zc);
>         errno = 0;
>         err = getsockopt(fd, SOL_TCP, TCP_ZEROCOPY_RECEIVE, &buf, &optlen);
> -       if (errno != EPERM) {
> +       if (errno != EINVAL) {
>                 log_err("Unexpected getsockopt(TCP_ZEROCOPY_RECEIVE) err=%d errno=%d",
>                         err, errno);
>                 goto err;
> diff --git a/tools/testing/selftests/bpf/progs/sockopt_sk.c b/tools/testing/selftests/bpf/progs/sockopt_sk.c
> index 79c8139b63b8..d0298dccedcd 100644
> --- a/tools/testing/selftests/bpf/progs/sockopt_sk.c
> +++ b/tools/testing/selftests/bpf/progs/sockopt_sk.c
> @@ -73,17 +73,17 @@ int _getsockopt(struct bpf_sockopt *ctx)
>                  */
>
>                 if (optval + sizeof(struct tcp_zerocopy_receive) > optval_end)
> -                       return 0; /* EPERM, bounds check */
> +                       return 0; /* bounds check */
>
>                 if (((struct tcp_zerocopy_receive *)optval)->address != 0)
> -                       return 0; /* EPERM, unexpected data */
> +                       return 0; /* unexpected data */
>
>                 return 1;
>         }
>
>         if (ctx->level == SOL_IP && ctx->optname == IP_FREEBIND) {
>                 if (optval + 1 > optval_end)
> -                       return 0; /* EPERM, bounds check */
> +                       return 0; /* bounds check */
>
>                 ctx->retval = 0; /* Reset system call return value to zero */
>
> @@ -96,24 +96,24 @@ int _getsockopt(struct bpf_sockopt *ctx)
>                  * bytes of data.
>                  */
>                 if (optval_end - optval != page_size)
> -                       return 0; /* EPERM, unexpected data size */
> +                       return 0; /* unexpected data size */
>
>                 return 1;
>         }
>
>         if (ctx->level != SOL_CUSTOM)
> -               return 0; /* EPERM, deny everything except custom level */
> +               return 0; /* deny everything except custom level */
>
>         if (optval + 1 > optval_end)
> -               return 0; /* EPERM, bounds check */
> +               return 0; /* bounds check */
>
>         storage = bpf_sk_storage_get(&socket_storage_map, ctx->sk, 0,
>                                      BPF_SK_STORAGE_GET_F_CREATE);
>         if (!storage)
> -               return 0; /* EPERM, couldn't get sk storage */
> +               return 0; /* couldn't get sk storage */
>
>         if (!ctx->retval)
> -               return 0; /* EPERM, kernel should not have handled
> +               return 0; /* kernel should not have handled
>                            * SOL_CUSTOM, something is wrong!
>                            */
>         ctx->retval = 0; /* Reset system call return value to zero */
> @@ -152,7 +152,7 @@ int _setsockopt(struct bpf_sockopt *ctx)
>                 /* Overwrite SO_SNDBUF value */
>
>                 if (optval + sizeof(__u32) > optval_end)
> -                       return 0; /* EPERM, bounds check */
> +                       return 0; /* bounds check */
>
>                 *(__u32 *)optval = 0x55AA;
>                 ctx->optlen = 4;
> @@ -164,7 +164,7 @@ int _setsockopt(struct bpf_sockopt *ctx)
>                 /* Always use cubic */
>
>                 if (optval + 5 > optval_end)
> -                       return 0; /* EPERM, bounds check */
> +                       return 0; /* bounds check */
>
>                 memcpy(optval, "cubic", 5);
>                 ctx->optlen = 5;
> @@ -175,10 +175,10 @@ int _setsockopt(struct bpf_sockopt *ctx)
>         if (ctx->level == SOL_IP && ctx->optname == IP_FREEBIND) {
>                 /* Original optlen is larger than PAGE_SIZE. */
>                 if (ctx->optlen != page_size * 2)
> -                       return 0; /* EPERM, unexpected data size */
> +                       return 0; /* unexpected data size */
>
>                 if (optval + 1 > optval_end)
> -                       return 0; /* EPERM, bounds check */
> +                       return 0; /* bounds check */
>
>                 /* Make sure we can trim the buffer. */
>                 optval[0] = 0;
> @@ -189,21 +189,21 @@ int _setsockopt(struct bpf_sockopt *ctx)
>                  * bytes of data.
>                  */
>                 if (optval_end - optval != page_size)
> -                       return 0; /* EPERM, unexpected data size */
> +                       return 0; /* unexpected data size */
>
>                 return 1;
>         }
>
>         if (ctx->level != SOL_CUSTOM)
> -               return 0; /* EPERM, deny everything except custom level */
> +               return 0; /* deny everything except custom level */
>
>         if (optval + 1 > optval_end)
> -               return 0; /* EPERM, bounds check */
> +               return 0; /* bounds check */
>
>         storage = bpf_sk_storage_get(&socket_storage_map, ctx->sk, 0,
>                                      BPF_SK_STORAGE_GET_F_CREATE);
>         if (!storage)
> -               return 0; /* EPERM, couldn't get sk storage */
> +               return 0; /* couldn't get sk storage */
>
>         storage->val = optval[0];
>         ctx->optlen = -1; /* BPF has consumed this option, don't call kernel
> --
> 2.34.1.173.g76aa8bc2d0-goog
>
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c b/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
index 4b937e5dbaca..164aa5020bf1 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
@@ -177,7 +177,7 @@  static int getsetsockopt(void)
 	optlen = sizeof(buf.zc);
 	errno = 0;
 	err = getsockopt(fd, SOL_TCP, TCP_ZEROCOPY_RECEIVE, &buf, &optlen);
-	if (errno != EPERM) {
+	if (errno != EINVAL) {
 		log_err("Unexpected getsockopt(TCP_ZEROCOPY_RECEIVE) err=%d errno=%d",
 			err, errno);
 		goto err;
diff --git a/tools/testing/selftests/bpf/progs/sockopt_sk.c b/tools/testing/selftests/bpf/progs/sockopt_sk.c
index 79c8139b63b8..d0298dccedcd 100644
--- a/tools/testing/selftests/bpf/progs/sockopt_sk.c
+++ b/tools/testing/selftests/bpf/progs/sockopt_sk.c
@@ -73,17 +73,17 @@  int _getsockopt(struct bpf_sockopt *ctx)
 		 */
 
 		if (optval + sizeof(struct tcp_zerocopy_receive) > optval_end)
-			return 0; /* EPERM, bounds check */
+			return 0; /* bounds check */
 
 		if (((struct tcp_zerocopy_receive *)optval)->address != 0)
-			return 0; /* EPERM, unexpected data */
+			return 0; /* unexpected data */
 
 		return 1;
 	}
 
 	if (ctx->level == SOL_IP && ctx->optname == IP_FREEBIND) {
 		if (optval + 1 > optval_end)
-			return 0; /* EPERM, bounds check */
+			return 0; /* bounds check */
 
 		ctx->retval = 0; /* Reset system call return value to zero */
 
@@ -96,24 +96,24 @@  int _getsockopt(struct bpf_sockopt *ctx)
 		 * bytes of data.
 		 */
 		if (optval_end - optval != page_size)
-			return 0; /* EPERM, unexpected data size */
+			return 0; /* unexpected data size */
 
 		return 1;
 	}
 
 	if (ctx->level != SOL_CUSTOM)
-		return 0; /* EPERM, deny everything except custom level */
+		return 0; /* deny everything except custom level */
 
 	if (optval + 1 > optval_end)
-		return 0; /* EPERM, bounds check */
+		return 0; /* bounds check */
 
 	storage = bpf_sk_storage_get(&socket_storage_map, ctx->sk, 0,
 				     BPF_SK_STORAGE_GET_F_CREATE);
 	if (!storage)
-		return 0; /* EPERM, couldn't get sk storage */
+		return 0; /* couldn't get sk storage */
 
 	if (!ctx->retval)
-		return 0; /* EPERM, kernel should not have handled
+		return 0; /* kernel should not have handled
 			   * SOL_CUSTOM, something is wrong!
 			   */
 	ctx->retval = 0; /* Reset system call return value to zero */
@@ -152,7 +152,7 @@  int _setsockopt(struct bpf_sockopt *ctx)
 		/* Overwrite SO_SNDBUF value */
 
 		if (optval + sizeof(__u32) > optval_end)
-			return 0; /* EPERM, bounds check */
+			return 0; /* bounds check */
 
 		*(__u32 *)optval = 0x55AA;
 		ctx->optlen = 4;
@@ -164,7 +164,7 @@  int _setsockopt(struct bpf_sockopt *ctx)
 		/* Always use cubic */
 
 		if (optval + 5 > optval_end)
-			return 0; /* EPERM, bounds check */
+			return 0; /* bounds check */
 
 		memcpy(optval, "cubic", 5);
 		ctx->optlen = 5;
@@ -175,10 +175,10 @@  int _setsockopt(struct bpf_sockopt *ctx)
 	if (ctx->level == SOL_IP && ctx->optname == IP_FREEBIND) {
 		/* Original optlen is larger than PAGE_SIZE. */
 		if (ctx->optlen != page_size * 2)
-			return 0; /* EPERM, unexpected data size */
+			return 0; /* unexpected data size */
 
 		if (optval + 1 > optval_end)
-			return 0; /* EPERM, bounds check */
+			return 0; /* bounds check */
 
 		/* Make sure we can trim the buffer. */
 		optval[0] = 0;
@@ -189,21 +189,21 @@  int _setsockopt(struct bpf_sockopt *ctx)
 		 * bytes of data.
 		 */
 		if (optval_end - optval != page_size)
-			return 0; /* EPERM, unexpected data size */
+			return 0; /* unexpected data size */
 
 		return 1;
 	}
 
 	if (ctx->level != SOL_CUSTOM)
-		return 0; /* EPERM, deny everything except custom level */
+		return 0; /* deny everything except custom level */
 
 	if (optval + 1 > optval_end)
-		return 0; /* EPERM, bounds check */
+		return 0; /* bounds check */
 
 	storage = bpf_sk_storage_get(&socket_storage_map, ctx->sk, 0,
 				     BPF_SK_STORAGE_GET_F_CREATE);
 	if (!storage)
-		return 0; /* EPERM, couldn't get sk storage */
+		return 0; /* couldn't get sk storage */
 
 	storage->val = optval[0];
 	ctx->optlen = -1; /* BPF has consumed this option, don't call kernel