diff mbox series

[v2,bpf-next,1/6] selftests/bpf: Fix bind program for big endian systems

Message ID 20240412165230.2009746-2-jrife@google.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series selftests/bpf: Add sockaddr tests for kernel networking | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
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-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
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-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
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 / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-33 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-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-42 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-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 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-25 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-24 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-27 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_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, 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_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_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_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 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-40 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-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 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-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 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_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next, async
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: 949 this patch: 949
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 17 of 17 maintainers
netdev/build_clang success Errors and warnings before: 955 this patch: 955
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: 961 this patch: 961
netdev/checkpatch warning WARNING: Use of volatile is usually wrong: see Documentation/process/volatile-considered-harmful.rst WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? WARNING: line length of 82 exceeds 80 columns WARNING: line length of 85 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jordan Rife April 12, 2024, 4:52 p.m. UTC 
Without this fix, the bind4 and bind6 programs will reject bind attempts
on big endian systems. This patch ensures that CI tests pass for the
s390x architecture.

Signed-off-by: Jordan Rife <jrife@google.com>
---
 .../testing/selftests/bpf/progs/bind4_prog.c  | 18 ++++++++++--------
 .../testing/selftests/bpf/progs/bind6_prog.c  | 18 ++++++++++--------
 tools/testing/selftests/bpf/progs/bind_prog.h | 19 +++++++++++++++++++
 3 files changed, 39 insertions(+), 16 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/bind_prog.h

Comments

Kui-Feng Lee April 13, 2024, 1:01 a.m. UTC | #1 
On 4/12/24 09:52, Jordan Rife wrote:
> Without this fix, the bind4 and bind6 programs will reject bind attempts
> on big endian systems. This patch ensures that CI tests pass for the
> s390x architecture.
> 
> Signed-off-by: Jordan Rife <jrife@google.com>
> ---
>   .../testing/selftests/bpf/progs/bind4_prog.c  | 18 ++++++++++--------
>   .../testing/selftests/bpf/progs/bind6_prog.c  | 18 ++++++++++--------
>   tools/testing/selftests/bpf/progs/bind_prog.h | 19 +++++++++++++++++++
>   3 files changed, 39 insertions(+), 16 deletions(-)
>   create mode 100644 tools/testing/selftests/bpf/progs/bind_prog.h
> 
> diff --git a/tools/testing/selftests/bpf/progs/bind4_prog.c b/tools/testing/selftests/bpf/progs/bind4_prog.c
> index a487f60b73ac4..2bc052ecb6eef 100644
> --- a/tools/testing/selftests/bpf/progs/bind4_prog.c
> +++ b/tools/testing/selftests/bpf/progs/bind4_prog.c
> @@ -12,6 +12,8 @@
>   #include <bpf/bpf_helpers.h>
>   #include <bpf/bpf_endian.h>
>   
> +#include "bind_prog.h"
> +
>   #define SERV4_IP		0xc0a801feU /* 192.168.1.254 */
>   #define SERV4_PORT		4040
>   #define SERV4_REWRITE_IP	0x7f000001U /* 127.0.0.1 */
> @@ -118,23 +120,23 @@ int bind_v4_prog(struct bpf_sock_addr *ctx)
>   
>   	// u8 narrow loads:
>   	user_ip4 = 0;
> -	user_ip4 |= ((volatile __u8 *)&ctx->user_ip4)[0] << 0;
> -	user_ip4 |= ((volatile __u8 *)&ctx->user_ip4)[1] << 8;
> -	user_ip4 |= ((volatile __u8 *)&ctx->user_ip4)[2] << 16;
> -	user_ip4 |= ((volatile __u8 *)&ctx->user_ip4)[3] << 24;
> +	user_ip4 |= load_byte_ntoh(ctx->user_ip4, 0, sizeof(user_ip4));
> +	user_ip4 |= load_byte_ntoh(ctx->user_ip4, 1, sizeof(user_ip4));
> +	user_ip4 |= load_byte_ntoh(ctx->user_ip4, 2, sizeof(user_ip4));
> +	user_ip4 |= load_byte_ntoh(ctx->user_ip4, 3, sizeof(user_ip4));
>   	if (ctx->user_ip4 != user_ip4)
>   		return 0;
>   
>   	user_port = 0;
> -	user_port |= ((volatile __u8 *)&ctx->user_port)[0] << 0;
> -	user_port |= ((volatile __u8 *)&ctx->user_port)[1] << 8;
> +	user_port |= load_byte_ntoh(ctx->user_port, 0, sizeof(user_port));
> +	user_port |= load_byte_ntoh(ctx->user_port, 1, sizeof(user_port));
>   	if (ctx->user_port != user_port)
>   		return 0;
>   
>   	// u16 narrow loads:
>   	user_ip4 = 0;
> -	user_ip4 |= ((volatile __u16 *)&ctx->user_ip4)[0] << 0;
> -	user_ip4 |= ((volatile __u16 *)&ctx->user_ip4)[1] << 16;
> +	user_ip4 |= load_word_ntoh(ctx->user_ip4, 0, sizeof(user_ip4));
> +	user_ip4 |= load_word_ntoh(ctx->user_ip4, 1, sizeof(user_ip4));
>   	if (ctx->user_ip4 != user_ip4)
>   		return 0;
>   
> diff --git a/tools/testing/selftests/bpf/progs/bind6_prog.c b/tools/testing/selftests/bpf/progs/bind6_prog.c
> index d62cd9e9cf0ea..194583e3375bf 100644
> --- a/tools/testing/selftests/bpf/progs/bind6_prog.c
> +++ b/tools/testing/selftests/bpf/progs/bind6_prog.c
> @@ -12,6 +12,8 @@
>   #include <bpf/bpf_helpers.h>
>   #include <bpf/bpf_endian.h>
>   
> +#include "bind_prog.h"
> +
>   #define SERV6_IP_0		0xfaceb00c /* face:b00c:1234:5678::abcd */
>   #define SERV6_IP_1		0x12345678
>   #define SERV6_IP_2		0x00000000
> @@ -129,25 +131,25 @@ int bind_v6_prog(struct bpf_sock_addr *ctx)
>   	// u8 narrow loads:
>   	for (i = 0; i < 4; i++) {
>   		user_ip6 = 0;
> -		user_ip6 |= ((volatile __u8 *)&ctx->user_ip6[i])[0] << 0;
> -		user_ip6 |= ((volatile __u8 *)&ctx->user_ip6[i])[1] << 8;
> -		user_ip6 |= ((volatile __u8 *)&ctx->user_ip6[i])[2] << 16;
> -		user_ip6 |= ((volatile __u8 *)&ctx->user_ip6[i])[3] << 24;
> +		user_ip6 |= load_byte_ntoh(ctx->user_ip6[i], 0, sizeof(user_ip6));
> +		user_ip6 |= load_byte_ntoh(ctx->user_ip6[i], 1, sizeof(user_ip6));
> +		user_ip6 |= load_byte_ntoh(ctx->user_ip6[i], 2, sizeof(user_ip6));
> +		user_ip6 |= load_byte_ntoh(ctx->user_ip6[i], 3, sizeof(user_ip6));
>   		if (ctx->user_ip6[i] != user_ip6)
>   			return 0;
>   	}
>   
>   	user_port = 0;
> -	user_port |= ((volatile __u8 *)&ctx->user_port)[0] << 0;
> -	user_port |= ((volatile __u8 *)&ctx->user_port)[1] << 8;
> +	user_port |= load_byte_ntoh(ctx->user_port, 0, sizeof(user_port));
> +	user_port |= load_byte_ntoh(ctx->user_port, 1, sizeof(user_port));
>   	if (ctx->user_port != user_port)
>   		return 0;
>   
>   	// u16 narrow loads:
>   	for (i = 0; i < 4; i++) {
>   		user_ip6 = 0;
> -		user_ip6 |= ((volatile __u16 *)&ctx->user_ip6[i])[0] << 0;
> -		user_ip6 |= ((volatile __u16 *)&ctx->user_ip6[i])[1] << 16;
> +		user_ip6 |= load_word_ntoh(ctx->user_ip6[i], 0, sizeof(user_ip6));
> +		user_ip6 |= load_word_ntoh(ctx->user_ip6[i], 1, sizeof(user_ip6));
>   		if (ctx->user_ip6[i] != user_ip6)
>   			return 0;
>   	}
> diff --git a/tools/testing/selftests/bpf/progs/bind_prog.h b/tools/testing/selftests/bpf/progs/bind_prog.h
> new file mode 100644
> index 0000000000000..0fdc466aec346
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/bind_prog.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __BIND_PROG_H__
> +#define __BIND_PROG_H__
> +
> +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> +#define load_byte_ntoh(src, b, s) \
> +	(((volatile __u8 *)&(src))[b] << 8 * b)
> +#define load_word_ntoh(src, w, s) \
> +	(((volatile __u16 *)&(src))[w] << 16 * w)
> +#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> +#define load_byte_ntoh(src, b, s) \
> +	(((volatile __u8 *)&(src))[(b) + (sizeof(src) - (s))] << 8 * ((s) - (b) - 1))
> +#define load_word_ntoh(src, w, s) \
> +	(((volatile __u16 *)&(src))[w] << 16 * (((s) / 2) - (w) - 1))
These names, load_byte_ntoh() and load_word_ntoh(), are miss-leading.

They don't actually do byte-order conversion from network order to host
order. Network order is big endian. 0xdeadbeef in u32 should be stored
as the sequence of

   0xde, 0xad, 0xbe, 0xef

The little endian implementation of load_word_ntoh() provided here will
return 0xadde and 0xefbe0000. However, a network order to host order
conversion should return 0xbeef and 0xdead0000 for little endian.

The little endian implementation of load_byte_ntoh() here returns 0xde,
0xad00, 0xbe0000, and 0xef000000. However, a network to host order
conversion should return 0xef, 0xbe00, 0xad0000, and 0xde00000.

So, they just access raw data following the host byte order, not
providing any byte order conversion.


> +#else
> +# error "Fix your compiler's __BYTE_ORDER__?!"
> +#endif
> +
> +#endif
Jordan Rife April 13, 2024, 1:19 a.m. UTC | #2 
Kui-Feng,

You are right. Maybe simply "load_word" and "load_byte" would be a
better name here. WDYT?

-Jordan


On Fri, Apr 12, 2024 at 6:01 PM Kui-Feng Lee <sinquersw@gmail.com> wrote:
>
>
>
> On 4/12/24 09:52, Jordan Rife wrote:
> > Without this fix, the bind4 and bind6 programs will reject bind attempts
> > on big endian systems. This patch ensures that CI tests pass for the
> > s390x architecture.
> >
> > Signed-off-by: Jordan Rife <jrife@google.com>
> > ---
> >   .../testing/selftests/bpf/progs/bind4_prog.c  | 18 ++++++++++--------
> >   .../testing/selftests/bpf/progs/bind6_prog.c  | 18 ++++++++++--------
> >   tools/testing/selftests/bpf/progs/bind_prog.h | 19 +++++++++++++++++++
> >   3 files changed, 39 insertions(+), 16 deletions(-)
> >   create mode 100644 tools/testing/selftests/bpf/progs/bind_prog.h
> >
> > diff --git a/tools/testing/selftests/bpf/progs/bind4_prog.c b/tools/testing/selftests/bpf/progs/bind4_prog.c
> > index a487f60b73ac4..2bc052ecb6eef 100644
> > --- a/tools/testing/selftests/bpf/progs/bind4_prog.c
> > +++ b/tools/testing/selftests/bpf/progs/bind4_prog.c
> > @@ -12,6 +12,8 @@
> >   #include <bpf/bpf_helpers.h>
> >   #include <bpf/bpf_endian.h>
> >
> > +#include "bind_prog.h"
> > +
> >   #define SERV4_IP            0xc0a801feU /* 192.168.1.254 */
> >   #define SERV4_PORT          4040
> >   #define SERV4_REWRITE_IP    0x7f000001U /* 127.0.0.1 */
> > @@ -118,23 +120,23 @@ int bind_v4_prog(struct bpf_sock_addr *ctx)
> >
> >       // u8 narrow loads:
> >       user_ip4 = 0;
> > -     user_ip4 |= ((volatile __u8 *)&ctx->user_ip4)[0] << 0;
> > -     user_ip4 |= ((volatile __u8 *)&ctx->user_ip4)[1] << 8;
> > -     user_ip4 |= ((volatile __u8 *)&ctx->user_ip4)[2] << 16;
> > -     user_ip4 |= ((volatile __u8 *)&ctx->user_ip4)[3] << 24;
> > +     user_ip4 |= load_byte_ntoh(ctx->user_ip4, 0, sizeof(user_ip4));
> > +     user_ip4 |= load_byte_ntoh(ctx->user_ip4, 1, sizeof(user_ip4));
> > +     user_ip4 |= load_byte_ntoh(ctx->user_ip4, 2, sizeof(user_ip4));
> > +     user_ip4 |= load_byte_ntoh(ctx->user_ip4, 3, sizeof(user_ip4));
> >       if (ctx->user_ip4 != user_ip4)
> >               return 0;
> >
> >       user_port = 0;
> > -     user_port |= ((volatile __u8 *)&ctx->user_port)[0] << 0;
> > -     user_port |= ((volatile __u8 *)&ctx->user_port)[1] << 8;
> > +     user_port |= load_byte_ntoh(ctx->user_port, 0, sizeof(user_port));
> > +     user_port |= load_byte_ntoh(ctx->user_port, 1, sizeof(user_port));
> >       if (ctx->user_port != user_port)
> >               return 0;
> >
> >       // u16 narrow loads:
> >       user_ip4 = 0;
> > -     user_ip4 |= ((volatile __u16 *)&ctx->user_ip4)[0] << 0;
> > -     user_ip4 |= ((volatile __u16 *)&ctx->user_ip4)[1] << 16;
> > +     user_ip4 |= load_word_ntoh(ctx->user_ip4, 0, sizeof(user_ip4));
> > +     user_ip4 |= load_word_ntoh(ctx->user_ip4, 1, sizeof(user_ip4));
> >       if (ctx->user_ip4 != user_ip4)
> >               return 0;
> >
> > diff --git a/tools/testing/selftests/bpf/progs/bind6_prog.c b/tools/testing/selftests/bpf/progs/bind6_prog.c
> > index d62cd9e9cf0ea..194583e3375bf 100644
> > --- a/tools/testing/selftests/bpf/progs/bind6_prog.c
> > +++ b/tools/testing/selftests/bpf/progs/bind6_prog.c
> > @@ -12,6 +12,8 @@
> >   #include <bpf/bpf_helpers.h>
> >   #include <bpf/bpf_endian.h>
> >
> > +#include "bind_prog.h"
> > +
> >   #define SERV6_IP_0          0xfaceb00c /* face:b00c:1234:5678::abcd */
> >   #define SERV6_IP_1          0x12345678
> >   #define SERV6_IP_2          0x00000000
> > @@ -129,25 +131,25 @@ int bind_v6_prog(struct bpf_sock_addr *ctx)
> >       // u8 narrow loads:
> >       for (i = 0; i < 4; i++) {
> >               user_ip6 = 0;
> > -             user_ip6 |= ((volatile __u8 *)&ctx->user_ip6[i])[0] << 0;
> > -             user_ip6 |= ((volatile __u8 *)&ctx->user_ip6[i])[1] << 8;
> > -             user_ip6 |= ((volatile __u8 *)&ctx->user_ip6[i])[2] << 16;
> > -             user_ip6 |= ((volatile __u8 *)&ctx->user_ip6[i])[3] << 24;
> > +             user_ip6 |= load_byte_ntoh(ctx->user_ip6[i], 0, sizeof(user_ip6));
> > +             user_ip6 |= load_byte_ntoh(ctx->user_ip6[i], 1, sizeof(user_ip6));
> > +             user_ip6 |= load_byte_ntoh(ctx->user_ip6[i], 2, sizeof(user_ip6));
> > +             user_ip6 |= load_byte_ntoh(ctx->user_ip6[i], 3, sizeof(user_ip6));
> >               if (ctx->user_ip6[i] != user_ip6)
> >                       return 0;
> >       }
> >
> >       user_port = 0;
> > -     user_port |= ((volatile __u8 *)&ctx->user_port)[0] << 0;
> > -     user_port |= ((volatile __u8 *)&ctx->user_port)[1] << 8;
> > +     user_port |= load_byte_ntoh(ctx->user_port, 0, sizeof(user_port));
> > +     user_port |= load_byte_ntoh(ctx->user_port, 1, sizeof(user_port));
> >       if (ctx->user_port != user_port)
> >               return 0;
> >
> >       // u16 narrow loads:
> >       for (i = 0; i < 4; i++) {
> >               user_ip6 = 0;
> > -             user_ip6 |= ((volatile __u16 *)&ctx->user_ip6[i])[0] << 0;
> > -             user_ip6 |= ((volatile __u16 *)&ctx->user_ip6[i])[1] << 16;
> > +             user_ip6 |= load_word_ntoh(ctx->user_ip6[i], 0, sizeof(user_ip6));
> > +             user_ip6 |= load_word_ntoh(ctx->user_ip6[i], 1, sizeof(user_ip6));
> >               if (ctx->user_ip6[i] != user_ip6)
> >                       return 0;
> >       }
> > diff --git a/tools/testing/selftests/bpf/progs/bind_prog.h b/tools/testing/selftests/bpf/progs/bind_prog.h
> > new file mode 100644
> > index 0000000000000..0fdc466aec346
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/bind_prog.h
> > @@ -0,0 +1,19 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __BIND_PROG_H__
> > +#define __BIND_PROG_H__
> > +
> > +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> > +#define load_byte_ntoh(src, b, s) \
> > +     (((volatile __u8 *)&(src))[b] << 8 * b)
> > +#define load_word_ntoh(src, w, s) \
> > +     (((volatile __u16 *)&(src))[w] << 16 * w)
> > +#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> > +#define load_byte_ntoh(src, b, s) \
> > +     (((volatile __u8 *)&(src))[(b) + (sizeof(src) - (s))] << 8 * ((s) - (b) - 1))
> > +#define load_word_ntoh(src, w, s) \
> > +     (((volatile __u16 *)&(src))[w] << 16 * (((s) / 2) - (w) - 1))
> These names, load_byte_ntoh() and load_word_ntoh(), are miss-leading.
>
> They don't actually do byte-order conversion from network order to host
> order. Network order is big endian. 0xdeadbeef in u32 should be stored
> as the sequence of
>
>    0xde, 0xad, 0xbe, 0xef
>
> The little endian implementation of load_word_ntoh() provided here will
> return 0xadde and 0xefbe0000. However, a network order to host order
> conversion should return 0xbeef and 0xdead0000 for little endian.
>
> The little endian implementation of load_byte_ntoh() here returns 0xde,
> 0xad00, 0xbe0000, and 0xef000000. However, a network to host order
> conversion should return 0xef, 0xbe00, 0xad0000, and 0xde00000.
>
> So, they just access raw data following the host byte order, not
> providing any byte order conversion.
>
>
> > +#else
> > +# error "Fix your compiler's __BYTE_ORDER__?!"
> > +#endif
> > +
> > +#endif
Kui-Feng Lee April 13, 2024, 1:28 a.m. UTC | #3 
On 4/12/24 18:17, Jordan Rife wrote:
> Kui-Feng,
> 
> You are right. Maybe simply "load_word" and "load_byte" would be a 
> better name here. WDYT?

Agree!


> 
> -Jordan
> 
> 
> On Fri, Apr 12, 2024 at 6:01 PM Kui-Feng Lee <sinquersw@gmail.com 
> <mailto:sinquersw@gmail.com>> wrote:
> 
> 
> 
>     On 4/12/24 09:52, Jordan Rife wrote:
>      > Without this fix, the bind4 and bind6 programs will reject bind
>     attempts
>      > on big endian systems. This patch ensures that CI tests pass for the
>      > s390x architecture.
>      >
>      > Signed-off-by: Jordan Rife <jrife@google.com
>     <mailto:jrife@google.com>>
>      > ---
>      >   .../testing/selftests/bpf/progs/bind4_prog.c  | 18
>     ++++++++++--------
>      >   .../testing/selftests/bpf/progs/bind6_prog.c  | 18
>     ++++++++++--------
>      >   tools/testing/selftests/bpf/progs/bind_prog.h | 19
>     +++++++++++++++++++
>      >   3 files changed, 39 insertions(+), 16 deletions(-)
>      >   create mode 100644 tools/testing/selftests/bpf/progs/bind_prog.h
>      >
>      > diff --git a/tools/testing/selftests/bpf/progs/bind4_prog.c
>     b/tools/testing/selftests/bpf/progs/bind4_prog.c
>      > index a487f60b73ac4..2bc052ecb6eef 100644
>      > --- a/tools/testing/selftests/bpf/progs/bind4_prog.c
>      > +++ b/tools/testing/selftests/bpf/progs/bind4_prog.c
>      > @@ -12,6 +12,8 @@
>      >   #include <bpf/bpf_helpers.h>
>      >   #include <bpf/bpf_endian.h>
>      >
>      > +#include "bind_prog.h"
>      > +
>      >   #define SERV4_IP            0xc0a801feU /* 192.168.1.254 */
>      >   #define SERV4_PORT          4040
>      >   #define SERV4_REWRITE_IP    0x7f000001U /* 127.0.0.1 */
>      > @@ -118,23 +120,23 @@ int bind_v4_prog(struct bpf_sock_addr *ctx)
>      >
>      >       // u8 narrow loads:
>      >       user_ip4 = 0;
>      > -     user_ip4 |= ((volatile __u8 *)&ctx->user_ip4)[0] << 0;
>      > -     user_ip4 |= ((volatile __u8 *)&ctx->user_ip4)[1] << 8;
>      > -     user_ip4 |= ((volatile __u8 *)&ctx->user_ip4)[2] << 16;
>      > -     user_ip4 |= ((volatile __u8 *)&ctx->user_ip4)[3] << 24;
>      > +     user_ip4 |= load_byte_ntoh(ctx->user_ip4, 0, sizeof(user_ip4));
>      > +     user_ip4 |= load_byte_ntoh(ctx->user_ip4, 1, sizeof(user_ip4));
>      > +     user_ip4 |= load_byte_ntoh(ctx->user_ip4, 2, sizeof(user_ip4));
>      > +     user_ip4 |= load_byte_ntoh(ctx->user_ip4, 3, sizeof(user_ip4));
>      >       if (ctx->user_ip4 != user_ip4)
>      >               return 0;
>      >
>      >       user_port = 0;
>      > -     user_port |= ((volatile __u8 *)&ctx->user_port)[0] << 0;
>      > -     user_port |= ((volatile __u8 *)&ctx->user_port)[1] << 8;
>      > +     user_port |= load_byte_ntoh(ctx->user_port, 0,
>     sizeof(user_port));
>      > +     user_port |= load_byte_ntoh(ctx->user_port, 1,
>     sizeof(user_port));
>      >       if (ctx->user_port != user_port)
>      >               return 0;
>      >
>      >       // u16 narrow loads:
>      >       user_ip4 = 0;
>      > -     user_ip4 |= ((volatile __u16 *)&ctx->user_ip4)[0] << 0;
>      > -     user_ip4 |= ((volatile __u16 *)&ctx->user_ip4)[1] << 16;
>      > +     user_ip4 |= load_word_ntoh(ctx->user_ip4, 0, sizeof(user_ip4));
>      > +     user_ip4 |= load_word_ntoh(ctx->user_ip4, 1, sizeof(user_ip4));
>      >       if (ctx->user_ip4 != user_ip4)
>      >               return 0;
>      >
>      > diff --git a/tools/testing/selftests/bpf/progs/bind6_prog.c
>     b/tools/testing/selftests/bpf/progs/bind6_prog.c
>      > index d62cd9e9cf0ea..194583e3375bf 100644
>      > --- a/tools/testing/selftests/bpf/progs/bind6_prog.c
>      > +++ b/tools/testing/selftests/bpf/progs/bind6_prog.c
>      > @@ -12,6 +12,8 @@
>      >   #include <bpf/bpf_helpers.h>
>      >   #include <bpf/bpf_endian.h>
>      >
>      > +#include "bind_prog.h"
>      > +
>      >   #define SERV6_IP_0          0xfaceb00c /*
>     face:b00c:1234:5678::abcd */
>      >   #define SERV6_IP_1          0x12345678
>      >   #define SERV6_IP_2          0x00000000
>      > @@ -129,25 +131,25 @@ int bind_v6_prog(struct bpf_sock_addr *ctx)
>      >       // u8 narrow loads:
>      >       for (i = 0; i < 4; i++) {
>      >               user_ip6 = 0;
>      > -             user_ip6 |= ((volatile __u8 *)&ctx->user_ip6[i])[0]
>     << 0;
>      > -             user_ip6 |= ((volatile __u8 *)&ctx->user_ip6[i])[1]
>     << 8;
>      > -             user_ip6 |= ((volatile __u8 *)&ctx->user_ip6[i])[2]
>     << 16;
>      > -             user_ip6 |= ((volatile __u8 *)&ctx->user_ip6[i])[3]
>     << 24;
>      > +             user_ip6 |= load_byte_ntoh(ctx->user_ip6[i], 0,
>     sizeof(user_ip6));
>      > +             user_ip6 |= load_byte_ntoh(ctx->user_ip6[i], 1,
>     sizeof(user_ip6));
>      > +             user_ip6 |= load_byte_ntoh(ctx->user_ip6[i], 2,
>     sizeof(user_ip6));
>      > +             user_ip6 |= load_byte_ntoh(ctx->user_ip6[i], 3,
>     sizeof(user_ip6));
>      >               if (ctx->user_ip6[i] != user_ip6)
>      >                       return 0;
>      >       }
>      >
>      >       user_port = 0;
>      > -     user_port |= ((volatile __u8 *)&ctx->user_port)[0] << 0;
>      > -     user_port |= ((volatile __u8 *)&ctx->user_port)[1] << 8;
>      > +     user_port |= load_byte_ntoh(ctx->user_port, 0,
>     sizeof(user_port));
>      > +     user_port |= load_byte_ntoh(ctx->user_port, 1,
>     sizeof(user_port));
>      >       if (ctx->user_port != user_port)
>      >               return 0;
>      >
>      >       // u16 narrow loads:
>      >       for (i = 0; i < 4; i++) {
>      >               user_ip6 = 0;
>      > -             user_ip6 |= ((volatile __u16
>     *)&ctx->user_ip6[i])[0] << 0;
>      > -             user_ip6 |= ((volatile __u16
>     *)&ctx->user_ip6[i])[1] << 16;
>      > +             user_ip6 |= load_word_ntoh(ctx->user_ip6[i], 0,
>     sizeof(user_ip6));
>      > +             user_ip6 |= load_word_ntoh(ctx->user_ip6[i], 1,
>     sizeof(user_ip6));
>      >               if (ctx->user_ip6[i] != user_ip6)
>      >                       return 0;
>      >       }
>      > diff --git a/tools/testing/selftests/bpf/progs/bind_prog.h
>     b/tools/testing/selftests/bpf/progs/bind_prog.h
>      > new file mode 100644
>      > index 0000000000000..0fdc466aec346
>      > --- /dev/null
>      > +++ b/tools/testing/selftests/bpf/progs/bind_prog.h
>      > @@ -0,0 +1,19 @@
>      > +/* SPDX-License-Identifier: GPL-2.0 */
>      > +#ifndef __BIND_PROG_H__
>      > +#define __BIND_PROG_H__
>      > +
>      > +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>      > +#define load_byte_ntoh(src, b, s) \
>      > +     (((volatile __u8 *)&(src))[b] << 8 * b)
>      > +#define load_word_ntoh(src, w, s) \
>      > +     (((volatile __u16 *)&(src))[w] << 16 * w)
>      > +#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
>      > +#define load_byte_ntoh(src, b, s) \
>      > +     (((volatile __u8 *)&(src))[(b) + (sizeof(src) - (s))] << 8
>     * ((s) - (b) - 1))
>      > +#define load_word_ntoh(src, w, s) \
>      > +     (((volatile __u16 *)&(src))[w] << 16 * (((s) / 2) - (w) - 1))
>     These names, load_byte_ntoh() and load_word_ntoh(), are miss-leading.
> 
>     They don't actually do byte-order conversion from network order to host
>     order. Network order is big endian. 0xdeadbeef in u32 should be stored
>     as the sequence of
> 
>         0xde, 0xad, 0xbe, 0xef
> 
>     The little endian implementation of load_word_ntoh() provided here will
>     return 0xadde and 0xefbe0000. However, a network order to host order
>     conversion should return 0xbeef and 0xdead0000 for little endian.
> 
>     The little endian implementation of load_byte_ntoh() here returns 0xde,
>     0xad00, 0xbe0000, and 0xef000000. However, a network to host order
>     conversion should return 0xef, 0xbe00, 0xad0000, and 0xde00000.
> 
>     So, they just access raw data following the host byte order, not
>     providing any byte order conversion.
> 
> 
>      > +#else
>      > +# error "Fix your compiler's __BYTE_ORDER__?!"
>      > +#endif
>      > +
>      > +#endif
>
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/progs/bind4_prog.c b/tools/testing/selftests/bpf/progs/bind4_prog.c
index a487f60b73ac4..2bc052ecb6eef 100644
--- a/tools/testing/selftests/bpf/progs/bind4_prog.c
+++ b/tools/testing/selftests/bpf/progs/bind4_prog.c
@@ -12,6 +12,8 @@ 
 #include <bpf/bpf_helpers.h>
 #include <bpf/bpf_endian.h>
 
+#include "bind_prog.h"
+
 #define SERV4_IP		0xc0a801feU /* 192.168.1.254 */
 #define SERV4_PORT		4040
 #define SERV4_REWRITE_IP	0x7f000001U /* 127.0.0.1 */
@@ -118,23 +120,23 @@  int bind_v4_prog(struct bpf_sock_addr *ctx)
 
 	// u8 narrow loads:
 	user_ip4 = 0;
-	user_ip4 |= ((volatile __u8 *)&ctx->user_ip4)[0] << 0;
-	user_ip4 |= ((volatile __u8 *)&ctx->user_ip4)[1] << 8;
-	user_ip4 |= ((volatile __u8 *)&ctx->user_ip4)[2] << 16;
-	user_ip4 |= ((volatile __u8 *)&ctx->user_ip4)[3] << 24;
+	user_ip4 |= load_byte_ntoh(ctx->user_ip4, 0, sizeof(user_ip4));
+	user_ip4 |= load_byte_ntoh(ctx->user_ip4, 1, sizeof(user_ip4));
+	user_ip4 |= load_byte_ntoh(ctx->user_ip4, 2, sizeof(user_ip4));
+	user_ip4 |= load_byte_ntoh(ctx->user_ip4, 3, sizeof(user_ip4));
 	if (ctx->user_ip4 != user_ip4)
 		return 0;
 
 	user_port = 0;
-	user_port |= ((volatile __u8 *)&ctx->user_port)[0] << 0;
-	user_port |= ((volatile __u8 *)&ctx->user_port)[1] << 8;
+	user_port |= load_byte_ntoh(ctx->user_port, 0, sizeof(user_port));
+	user_port |= load_byte_ntoh(ctx->user_port, 1, sizeof(user_port));
 	if (ctx->user_port != user_port)
 		return 0;
 
 	// u16 narrow loads:
 	user_ip4 = 0;
-	user_ip4 |= ((volatile __u16 *)&ctx->user_ip4)[0] << 0;
-	user_ip4 |= ((volatile __u16 *)&ctx->user_ip4)[1] << 16;
+	user_ip4 |= load_word_ntoh(ctx->user_ip4, 0, sizeof(user_ip4));
+	user_ip4 |= load_word_ntoh(ctx->user_ip4, 1, sizeof(user_ip4));
 	if (ctx->user_ip4 != user_ip4)
 		return 0;
 
diff --git a/tools/testing/selftests/bpf/progs/bind6_prog.c b/tools/testing/selftests/bpf/progs/bind6_prog.c
index d62cd9e9cf0ea..194583e3375bf 100644
--- a/tools/testing/selftests/bpf/progs/bind6_prog.c
+++ b/tools/testing/selftests/bpf/progs/bind6_prog.c
@@ -12,6 +12,8 @@ 
 #include <bpf/bpf_helpers.h>
 #include <bpf/bpf_endian.h>
 
+#include "bind_prog.h"
+
 #define SERV6_IP_0		0xfaceb00c /* face:b00c:1234:5678::abcd */
 #define SERV6_IP_1		0x12345678
 #define SERV6_IP_2		0x00000000
@@ -129,25 +131,25 @@  int bind_v6_prog(struct bpf_sock_addr *ctx)
 	// u8 narrow loads:
 	for (i = 0; i < 4; i++) {
 		user_ip6 = 0;
-		user_ip6 |= ((volatile __u8 *)&ctx->user_ip6[i])[0] << 0;
-		user_ip6 |= ((volatile __u8 *)&ctx->user_ip6[i])[1] << 8;
-		user_ip6 |= ((volatile __u8 *)&ctx->user_ip6[i])[2] << 16;
-		user_ip6 |= ((volatile __u8 *)&ctx->user_ip6[i])[3] << 24;
+		user_ip6 |= load_byte_ntoh(ctx->user_ip6[i], 0, sizeof(user_ip6));
+		user_ip6 |= load_byte_ntoh(ctx->user_ip6[i], 1, sizeof(user_ip6));
+		user_ip6 |= load_byte_ntoh(ctx->user_ip6[i], 2, sizeof(user_ip6));
+		user_ip6 |= load_byte_ntoh(ctx->user_ip6[i], 3, sizeof(user_ip6));
 		if (ctx->user_ip6[i] != user_ip6)
 			return 0;
 	}
 
 	user_port = 0;
-	user_port |= ((volatile __u8 *)&ctx->user_port)[0] << 0;
-	user_port |= ((volatile __u8 *)&ctx->user_port)[1] << 8;
+	user_port |= load_byte_ntoh(ctx->user_port, 0, sizeof(user_port));
+	user_port |= load_byte_ntoh(ctx->user_port, 1, sizeof(user_port));
 	if (ctx->user_port != user_port)
 		return 0;
 
 	// u16 narrow loads:
 	for (i = 0; i < 4; i++) {
 		user_ip6 = 0;
-		user_ip6 |= ((volatile __u16 *)&ctx->user_ip6[i])[0] << 0;
-		user_ip6 |= ((volatile __u16 *)&ctx->user_ip6[i])[1] << 16;
+		user_ip6 |= load_word_ntoh(ctx->user_ip6[i], 0, sizeof(user_ip6));
+		user_ip6 |= load_word_ntoh(ctx->user_ip6[i], 1, sizeof(user_ip6));
 		if (ctx->user_ip6[i] != user_ip6)
 			return 0;
 	}
diff --git a/tools/testing/selftests/bpf/progs/bind_prog.h b/tools/testing/selftests/bpf/progs/bind_prog.h
new file mode 100644
index 0000000000000..0fdc466aec346
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/bind_prog.h
@@ -0,0 +1,19 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __BIND_PROG_H__
+#define __BIND_PROG_H__
+
+#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+#define load_byte_ntoh(src, b, s) \
+	(((volatile __u8 *)&(src))[b] << 8 * b)
+#define load_word_ntoh(src, w, s) \
+	(((volatile __u16 *)&(src))[w] << 16 * w)
+#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
+#define load_byte_ntoh(src, b, s) \
+	(((volatile __u8 *)&(src))[(b) + (sizeof(src) - (s))] << 8 * ((s) - (b) - 1))
+#define load_word_ntoh(src, w, s) \
+	(((volatile __u16 *)&(src))[w] << 16 * (((s) / 2) - (w) - 1))
+#else
+# error "Fix your compiler's __BYTE_ORDER__?!"
+#endif
+
+#endif