On Sat, 15 Jan 2022 at 15:48, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Fri, Jan 14, 2022 at 3:14 PM Kenny Yu <kennyyu@xxxxxx> 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 > > ? > I would personally find it more intuitive to have process information passed as either the first argument (like process_vm_readv does), or as "last", just before the flags (as extra information required w.r.t. to local versions, e.g. bpf_copy_from_user).
