Re: [PATCH v1 bpf-next 16/18] selftests/bpf: Test fmod_ret candidates found by bpf_scan_ir

[Anton Protopopov]

On 26/11/06 09:50PM, bot+bpf-ci@xxxxxxxxxx wrote:
> > selftests/bpf: Test fmod_ret candidates found by bpf_scan_ir
> 
> TL;DR: Nits only in the selftests: a fmod_ret test program's name may
> misdescribe its target, and some includes in bpf_testmod_retval.c may be
> unused.
> 
> > diff --git a/tools/testing/selftests/bpf/progs/fmodret_retval.c b/tools/testing/selftests/bpf/progs/fmodret_retval.c
> > new file mode 100644
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/fmodret_retval.c
> 
> [ ... ]
> 
> > +/* Declared targets and the static rules are not restricted */
> > +SEC("fmod_ret/bpf_testmod_retval_declared")
> > +__success
> > +int BPF_PROG(declared_any, int ret)
> > +{
> > +	return -1;
> > +}
> > +
> > +SEC("fmod_ret/bpf_modify_return_test")
> > +__success
> > +int BPF_PROG(static_rule_any, int a, int *b, int ret)
> > +{
> > +	return -1;
> > +}
> 
> This isn't a bug, but bpf_modify_return_test() is in the
> bpf_test_modify_return_ids BTF_FMODRET set, so it looks like a declared
> target rather than one allowed by check_attach_modify_return().
> 
> Would a name like declared_vmlinux_any fit better, or was this meant to
> attach to something the static rules allow?

Yep, thanks. They have different nature: one is from vmlinux, one is from
module. Will rename correspondingly.

> [ ... ]
> 
> > diff --git a/tools/testing/selftests/bpf/test_kmods/bpf_testmod_retval.c b/tools/testing/selftests/bpf/test_kmods/bpf_testmod_retval.c
> > new file mode 100644
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/test_kmods/bpf_testmod_retval.c
> 
> [ ... ]
> 
> > +#include <linux/atomic.h>
> > +#include <linux/btf.h>
> > +#include <linux/btf_ids.h>
> > +#include <linux/compiler.h>
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> > +#include <linux/error-injection.h>
> > +#include <linux/mutex.h>
> > +#include <linux/rcupdate.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/types.h>
> 
> This isn't a bug, but are linux/mutex.h, linux/rcupdate.h and linux/btf.h
> still needed here, or are they left over from earlier fixtures?
> 
> [ ... ]

Thanks, indeed, will remove.

> 
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
> 
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/42294987431