Re: [PATCH RFC] bpf: Add support for reading user pointers

[Qais Yousef]

On 05/05/19 13:29, Joel Fernandes wrote:
> On Sun, May 05, 2019 at 12:04:24PM +0100, Qais Yousef wrote:
> > On 05/03/19 09:49, Joel Fernandes wrote:
> > > On Fri, May 03, 2019 at 01:12:34PM +0100, Qais Yousef wrote:
> > > > Hi Joel
> > > > 
> > > > On 05/02/19 16:49, Joel Fernandes (Google) wrote:
> > > > > The eBPF based opensnoop tool fails to read the file path string passed
> > > > > to the do_sys_open function. This is because it is a pointer to
> > > > > userspace address and causes an -EFAULT when read with
> > > > > probe_kernel_read. This is not an issue when running the tool on x86 but
> > > > > is an issue on arm64. This patch adds a new bpf function call based
> > > > 
> > > > I just did an experiment and if I use Android 4.9 kernel I indeed fail to see
> > > > PATH info when running opensnoop. But if I run on 5.1-rc7 opensnoop behaves
> > > > correctly on arm64.
> > > > 
> > > > My guess either a limitation that was fixed on later kernel versions or Android
> > > > kernel has some strict option/modifications that make this fail?
> > > 
> > > Thanks a lot for checking, yes I was testing 4.9 kernel with this patch (pixel 3).
> > > 
> > > I am not sure what has changed since then, but I still think it is a good
> > > idea to make the code more robust against such future issues anyway. In
> > > particular, we learnt with extensive discussions that user/kernel pointers
> > > are not necessarily distinguishable purely based on their address.
> > 
> > Yes I wasn't arguing against that. But the commit message is misleading or
> > needs more explanation at least. I tried 4.9.y stable and arm64 worked on that
> > too. Why do you think it's an arm64 problem?
> 
> Well it is broken on at least on at least one arm64 device and the patch I
> sent fixes it. We know that the bpf is using wrong kernel API so why not fix
> it? Are you saying we should not fix it like in this patch? Or do you have
> another fix in mind?

Again I have no issue with the new API. But the claim that it's a fix for
a broken arm64 is a big stretch. AFAICT you don't understand the root cause of
why copy_to_user_inatomic() fails in your case. Given that Android 4.9 has
its own patches on top of 4.9 stable, it might be something that was introduced
in one of these patches that breaks opensnoop, and by making it use the new API
you might be simply working around the problem. All I can see is that vanilla
4.9 stable works on arm64.

So I am happy about introducing the new API but not happy with the commit
message or the explanation given in it. Unless you can investigate the root
cause and relate how this fixes it (and not workaround a problem you're
specifically having) I think it's better to introduce this patch as a generic
new API that is more robust to handle reading __user data in BPF and drop
reference to opensnoop failures. They raise more questions and the real
intention of this patch anyway is to provide the new correct way for BPF
programs to read __user data regardless opensnoop fails or not AFAIU.

Cheers

--
Qais Yousef