Re: [PATCH v2 bpf-next 4/4] selftests/bpf: Add test for sleepable bpf iterator programs

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Kenny Yu <kennyyu@fb.com>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	Andrii Nakryiko <andrii@kernel.org>,
	Alexei Starovoitov <ast@kernel.org>, bpf <bpf@vger.kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Yonghong Song <yhs@fb.com>
Subject: Re: [PATCH v2 bpf-next 4/4] selftests/bpf: Add test for sleepable bpf iterator programs
Date: Fri, 14 Jan 2022 17:38:11 -0800	[thread overview]
Message-ID: <CAEf4BzY9s1ngF_ja_rrpY=1cNX=byVSjptNT-LaEKTsUJEfP6Q@mail.gmail.com> (raw)
In-Reply-To: <20220114231426.426052-1-kennyyu@fb.com>

On Fri, Jan 14, 2022 at 3:14 PM Kenny Yu <kennyyu@fb.com> wrote:
>
> Hi Alexei,
>
> > > +// New helper added
> > > +static long (*bpf_access_process_vm)(
> > > +       struct task_struct *tsk,
> > > +       unsigned long addr,
> > > +       void *buf,
> > > +       int len,
> > > +       unsigned int gup_flags) = (void *)186;
> >
> > This shouldn't be needed.
> > Since patch 1 updates tools/include/uapi/linux/bpf.h
> > it will be in bpf_helper_defs.h automatically.
>
> I will fix. This is my first time writing selftests, so I am not too familiar
> with how these are built and run. For my understanding, are these tests
> meant to be built and run after booting the new kernel?

Look at vmtest.sh under tools/testing/selftests/bpf, it handles
building kernel, selftests and spinning up qemu instance for running
selftests inside it.

>
> > > +
> > > +// Copied from include/linux/mm.h
> > > +#define FOLL_REMOTE 0x2000 /* we are working on non-current tsk/mm */
> >
> > Please use C style comments only.
>
> I will fix.
>
> > > +       numread = bpf_access_process_vm(task,
> > > +                                       (unsigned long)ptr,
> > > +                                       (void *)&user_data,
> > > +                                       sizeof(uint32_t),
> > > +                                       FOLL_REMOTE);
> >
> > We probably would need to hide flags like FOLL_REMOTE
> > inside the helper otherwise prog might confuse the kernel.
> > In this case I'm not even sure that FOLL_REMOTE is needed.
> > I suspect gup_flags=0 in all cases will work fine.
> > We're not doing write here and not pining anything.
> > fast_gup is not necessary either.
>
> Thanks for the suggestion! I'll remove the flag argument from the helper
> to simplify the API for bpf programs. This means that the helper will have
> the following signature:
>
>   bpf_access_process_vm(struct task_struct *tsk,
>                         unsigned long addr,
>                         void *buf,
>                         int len);

keeping generic u64 flags makes sense for the future, so I'd keep it.

But I also wanted to point out that this helper is logically in the
same family as bpf_probe_read_kernel/user and bpf_copy_from_user, etc,
where we have consistent pattern that first two arguments specify
destination buffer (so buf + len) and the remaining ones specify
source (in probe_read it's just an address, here it's tsk_addr). So I
wonder if it would be less surprising and more consistent to reorder
and have:

buf, len, tsk, addr, flags

?

>
> Thanks for the feedback!
>
> Kenny