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 ? > > Thanks for the feedback! > > Kenny
