Re: [PATCH 1/6] bpf: Allow ctx access for pointers to scalar

On Wed, Jan 22, 2020 at 08:09:59AM -0800, Alexei Starovoitov wrote:

SNIP

quoted

quoted

quoted

It cannot dereference it. Use it as what?

If this is from original bcc code, it will use bpf_probe_read for 
dereference. This is what I understand when I first reviewed this patch.
But it will be good to get Jiri's confirmation.

it blocked me from accessing 'filename' argument when I probed
do_sys_open via trampoline in bcc, like:

	KRETFUNC_PROBE(do_sys_open)
	{
	    const char *filename = (const char *) args[1];

AFAICS the current code does not allow for trampoline arguments
being other pointers than to void or struct, the patch should
detect that the argument is pointer to scalar type and let it
pass

Got it. I've looked up your bcc patches and I agree that there is no way to
workaround. BTF type argument of that kernel function is 'const char *' and the
verifier will enforce that if bpf program tries to cast it the verifier will
still see 'const char *'. (It's done this way by design). How about we special
case 'char *' in the verifier? Then my concern regarding future extensibility
of 'int *' and 'long *' will go away.
Compilers have a long history special casing 'char *'. In particular signed
char because it's a pointer to null terminated string. I think it's still a
special pointer from pointer aliasing point of view. I think the verifier can
treat it as scalar here too. In the future the verifier will get smarter and
will recognize it as PTR_TO_NULL_STRING while 'u8 *', 'u32 *' will be
PTR_TO_BTF_ID. I think it will solve this particular issue. I like conservative
approach to the verifier improvements: start with strict checking and relax it
on case-by-case. Instead of accepting wide range of cases and cause potential
compatibility issues.

ok, so something like below?

jirka


---

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 832b5d7fd892..dd678b8e00b7 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c

@@ -3664,6 +3664,19 @@ struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog)
 	}
 }
 
+static bool is_string_ptr(struct btf *btf, const struct btf_type *t)
+{
+	/* t comes in already as a pointer */
+	t = btf_type_by_id(btf, t->type);
+
+	/* allow const */
+	if (BTF_INFO_KIND(t->info) == BTF_KIND_CONST)
+		t = btf_type_by_id(btf, t->type);
+
+	/* char, signed char, unsigned char */
+	return btf_type_is_int(t) && t->size == 1;
+}
+
 bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 		    const struct bpf_prog *prog,
 		    struct bpf_insn_access_aux *info)

@@ -3730,6 +3743,9 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 		 */
 		return true;
 
+	if (is_string_ptr(btf, t))
+		return true;
+
 	/* this is a pointer to another type */
 	info->reg_type = PTR_TO_BTF_ID;