Re: [PATCH bpf-next 5/5] selftests/bpf: Cover BPF_F_REPLACE in test_cgroup_attach

[Martin Lau <kafai@fb.com>]

On Tue, Dec 10, 2019 at 06:33:31PM -0800, Andrey Ignatov wrote:
> Test replacement of a cgroup-bpf program attached with BPF_F_ALLOW_MULTI
> and possible failure modes: invalid combination of flags, invalid
> replace_bpf_fd, replacing a non-attachd to specified cgroup program.
> 
> Example of program replacing:
> 
>   # gdb -q ./test_cgroup_attach
>   Reading symbols from /data/users/rdna/bin/test_cgroup_attach...done.
>   ...
>   Breakpoint 1, test_multiprog () at test_cgroup_attach.c:442
>   442     test_cgroup_attach.c: No such file or directory.
>   (gdb)
>   [2]+  Stopped                 gdb -q ./test_cgroup_attach
>   # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1
>   ID       AttachType      AttachFlags     Name
>   35       egress          multi
>   36       egress          multi
>   # fg gdb -q ./test_cgroup_attach
>   c
>   Continuing.
>   Detaching after fork from child process 361.
> 
>   Breakpoint 2, test_multiprog () at test_cgroup_attach.c:453
>   453     in test_cgroup_attach.c
>   (gdb)
>   [2]+  Stopped                 gdb -q ./test_cgroup_attach
>   # bpftool c s /mnt/cgroup2/cgroup-test-work-dir/cg1
>   ID       AttachType      AttachFlags     Name
>   41       egress          multi
>   36       egress          multi
> 
> Signed-off-by: Andrey Ignatov <rdna@fb.com>
> ---
>  .../selftests/bpf/test_cgroup_attach.c        | 61 +++++++++++++++++--
>  1 file changed, 56 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/test_cgroup_attach.c b/tools/testing/selftests/bpf/test_cgroup_attach.c
> index 7671909ee1cb..b9148d752207 100644
> --- a/tools/testing/selftests/bpf/test_cgroup_attach.c
> +++ b/tools/testing/selftests/bpf/test_cgroup_attach.c
> @@ -250,7 +250,7 @@ static int prog_load_cnt(int verdict, int val)
>  		BPF_LD_MAP_FD(BPF_REG_1, map_fd),
>  		BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem),
>  		BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 2),
> -		BPF_MOV64_IMM(BPF_REG_1, val), /* r1 = 1 */
> +		BPF_MOV64_IMM(BPF_REG_1, val), /* r1 = val */
>  		BPF_RAW_INSN(BPF_STX | BPF_XADD | BPF_DW, BPF_REG_0, BPF_REG_1, 0, 0), /* xadd r0 += r1 */
>  
>  		BPF_LD_MAP_FD(BPF_REG_1, cgroup_storage_fd),
> @@ -290,11 +290,12 @@ static int test_multiprog(void)
>  {
>  	__u32 prog_ids[4], prog_cnt = 0, attach_flags, saved_prog_id;
>  	int cg1 = 0, cg2 = 0, cg3 = 0, cg4 = 0, cg5 = 0, key = 0;
> -	int drop_prog, allow_prog[6] = {}, rc = 0;
> +	int drop_prog, allow_prog[7] = {}, rc = 0;
> +	struct bpf_prog_attach_attr attach_attr;
>  	unsigned long long value;
>  	int i = 0;
>  
> -	for (i = 0; i < 6; i++) {
> +	for (i = 0; i < ARRAY_SIZE(allow_prog); i++) {
>  		allow_prog[i] = prog_load_cnt(1, 1 << i);
>  		if (!allow_prog[i])
>  			goto err;
> @@ -400,6 +401,56 @@ static int test_multiprog(void)
>  	assert(bpf_map_lookup_elem(map_fd, &key, &value) == 0);
>  	assert(value == 1 + 2 + 8 + 16);
>  
> +	/* invalid input */
> +
> +	memset(&attach_attr, 0, sizeof(attach_attr));
> +	attach_attr.target_fd		= cg1;
> +	attach_attr.prog_fd		= allow_prog[6];
> +	attach_attr.replace_prog_fd	= allow_prog[0];
> +	attach_attr.type		= BPF_CGROUP_INET_EGRESS;
> +	attach_attr.flags		= BPF_F_ALLOW_OVERRIDE | BPF_F_REPLACE;
> +
> +	if (!bpf_prog_attach_xattr(&attach_attr)) {
> +		log_err("Unexpected success with OVERRIDE | REPLACE");
> +		goto err;
> +	}
> +	assert(errno == EINVAL);
> +
> +	attach_attr.flags = BPF_F_REPLACE;
> +	if (!bpf_prog_attach_xattr(&attach_attr)) {
> +		log_err("Unexpected success with REPLACE alone");
> +		goto err;
> +	}
> +	assert(errno == EINVAL);
> +	attach_attr.flags = BPF_F_ALLOW_MULTI | BPF_F_REPLACE;
> +
> +	attach_attr.replace_prog_fd = -1;
> +	if (!bpf_prog_attach_xattr(&attach_attr)) {
The whole set LGTM.  I expect this attach bit will change based on
the discussion in patch 4.


> +		log_err("Unexpected success with bad replace fd");
> +		goto err;
> +	}