[RFC 0/3] bpf: Add d_path helper

[RFC 0/3] bpf: Add d_path helper

[Jiri Olsa]

hi,
adding d_path helper to return full path for 'path' object.

I originally added and used 'file_path' helper, which did the same,
but used 'struct file' object. Then realized that file_path is just
a wrapper for d_path, so we'd cover more calling sites if we add
d_path helper and allowed resolving BTF object within another object,
so we could call d_path also with file pointer, like:

  bpf_d_path(&file->f_path, buf, size);

This feature is mainly to be able to add dpath (filepath originally)
function to bpftrace, which seems to work nicely now, like:

  # bpftrace -e 'kretfunc:fget { printf("%s\n", dpath(args->ret->f_path));  }' 

I'm not completely sure this is all safe and bullet proof and there's
no other way to do this, hence RFC post.

I'd be happy also with file_path function, but I thought it'd be
a shame not to try to add d_path with the verifier change.
I'm open to any suggestions ;-)

thanks,
jirka


---
Jiri Olsa (3):
      bpf: Add support to check if BTF object is nested in another object
      bpf: Add d_path helper
      selftests/bpf: Add test for d_path helper

 include/linux/bpf.h                             |   3 ++
 include/uapi/linux/bpf.h                        |  14 ++++++-
 kernel/bpf/btf.c                                |  69 +++++++++++++++++++++++++++++++++
 kernel/bpf/verifier.c                           |  18 ++++++++-
 kernel/trace/bpf_trace.c                        |  31 +++++++++++++++
 scripts/bpf_helpers_doc.py                      |   2 +
 tools/include/uapi/linux/bpf.h                  |  14 ++++++-
 tools/testing/selftests/bpf/prog_tests/d_path.c | 196 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/testing/selftests/bpf/progs/test_d_path.c |  71 ++++++++++++++++++++++++++++++++++
 9 files changed, 414 insertions(+), 4 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/d_path.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_d_path.c

[PATCH 1/3] bpf: Add support to check if BTF object is nested in another object

[Jiri Olsa]

Adding btf_struct_address function that takes 2 BTF objects
and offset as arguments and checks wether object A is nested
in object B on given offset.

This function is be used when checking the helper function
PTR_TO_BTF_ID arguments. If the argument has an offset value,
the btf_struct_address will check if the final address is
the expected BTF ID.

This way we can access nested BTF objects under PTR_TO_BTF_ID
pointer type and pass them to helpers, while they still point
to valid kernel BTF objects.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/bpf.h   |  3 ++
 kernel/bpf/btf.c      | 69 +++++++++++++++++++++++++++++++++++++++++++
 kernel/bpf/verifier.c | 18 +++++++++--
 3 files changed, 88 insertions(+), 2 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index fd2b2322412d..d3bad7ee60c6 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1196,6 +1196,9 @@ int btf_struct_access(struct bpf_verifier_log *log,
 		      const struct btf_type *t, int off, int size,
 		      enum bpf_access_type atype,
 		      u32 *next_btf_id);
+int btf_struct_address(struct bpf_verifier_log *log,
+		     const struct btf_type *t,
+		     u32 off, u32 exp_id);
 int btf_resolve_helper_id(struct bpf_verifier_log *log,
 			  const struct bpf_func_proto *fn, int);
 
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index d65c6912bdaf..9aafffa57d8b 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -4002,6 +4002,75 @@ int btf_struct_access(struct bpf_verifier_log *log,
 	return -EINVAL;
 }
 
+int btf_struct_address(struct bpf_verifier_log *log,
+		       const struct btf_type *t,
+		       u32 off, u32 exp_id)
+{
+	u32 i, moff, mtrue_end, msize = 0;
+	const struct btf_member *member;
+	const struct btf_type *mtype;
+	const char *tname, *mname;
+
+again:
+	tname = __btf_name_by_offset(btf_vmlinux, t->name_off);
+	if (!btf_type_is_struct(t)) {
+		bpf_log(log, "Type '%s' is not a struct\n", tname);
+		return -EINVAL;
+	}
+
+	if (off > t->size) {
+		bpf_log(log, "address beyond struct %s at off %u size %u\n",
+			tname, off, t->size);
+		return -EACCES;
+	}
+
+	for_each_member(i, t, member) {
+		/* offset of the field in bytes */
+		moff = btf_member_bit_offset(t, member) / 8;
+		if (off < moff)
+			/* won't find anything, field is already too far */
+			break;
+
+		/* we found the member */
+		if (off == moff && member->type == exp_id)
+			return 0;
+
+		/* type of the field */
+		mtype = btf_type_by_id(btf_vmlinux, member->type);
+		mname = __btf_name_by_offset(btf_vmlinux, member->name_off);
+
+		mtype = btf_resolve_size(btf_vmlinux, mtype, &msize,
+					 NULL, NULL);
+		if (IS_ERR(mtype)) {
+			bpf_log(log, "field %s doesn't have size\n", mname);
+			return -EFAULT;
+		}
+
+		mtrue_end = moff + msize;
+		if (off >= mtrue_end)
+			/* no overlap with member, keep iterating */
+			continue;
+
+		/* the 'off' we're looking for is either equal to start
+		 * of this field or inside of this struct
+		 */
+		if (btf_type_is_struct(mtype)) {
+			/* our field must be inside that union or struct */
+			t = mtype;
+
+			/* adjust offset we're looking for */
+			off -= moff;
+			goto again;
+		}
+
+		bpf_log(log, "struct %s doesn't have struct field at offset %d\n", tname, off);
+		return -EACCES;
+	}
+
+	bpf_log(log, "struct %s doesn't have field at offset %d\n", tname, off);
+	return -EACCES;
+}
+
 static int __btf_resolve_helper_id(struct bpf_verifier_log *log, void *fn,
 				   int arg)
 {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 04c6630cc18f..6eb88bef4379 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3103,6 +3103,18 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
 	return 0;
 }
 
+static void check_ptr_in_btf(struct bpf_verifier_env *env,
+			     struct bpf_reg_state *reg,
+			     u32 exp_id)
+{
+	const struct btf_type *t = btf_type_by_id(btf_vmlinux, reg->btf_id);
+
+	if (!btf_struct_address(&env->log, t, reg->off, exp_id)) {
+		reg->btf_id = exp_id;
+		reg->off = 0;
+	}
+}
+
 /* check whether memory at (regno + off) is accessible for t = (read | write)
  * if t==write, value_regno is a register which value is stored into memory
  * if t==read, value_regno is a register which will receive the value from memory
@@ -3696,10 +3708,12 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 regno,
 		expected_type = PTR_TO_BTF_ID;
 		if (type != expected_type)
 			goto err_type;
+		if (reg->off)
+			check_ptr_in_btf(env, reg, meta->btf_id);
 		if (reg->btf_id != meta->btf_id) {
-			verbose(env, "Helper has type %s got %s in R%d\n",
+			verbose(env, "Helper has type %s got %s in R%d, off %d\n",
 				kernel_type_name(meta->btf_id),
-				kernel_type_name(reg->btf_id), regno);
+				kernel_type_name(reg->btf_id), regno, reg->off);
 
 			return -EACCES;
 		}
-- 
2.25.2

Re: [PATCH 1/3] bpf: Add support to check if BTF object is nested in another object

[Alexei Starovoitov]

On Wed, Apr 01, 2020 at 01:09:05PM +0200, Jiri Olsa wrote:
> Adding btf_struct_address function that takes 2 BTF objects
> and offset as arguments and checks wether object A is nested
> in object B on given offset.
> 
> This function is be used when checking the helper function
> PTR_TO_BTF_ID arguments. If the argument has an offset value,
> the btf_struct_address will check if the final address is
> the expected BTF ID.
> 
> This way we can access nested BTF objects under PTR_TO_BTF_ID
> pointer type and pass them to helpers, while they still point
> to valid kernel BTF objects.
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  include/linux/bpf.h   |  3 ++
>  kernel/bpf/btf.c      | 69 +++++++++++++++++++++++++++++++++++++++++++
>  kernel/bpf/verifier.c | 18 +++++++++--
>  3 files changed, 88 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index fd2b2322412d..d3bad7ee60c6 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1196,6 +1196,9 @@ int btf_struct_access(struct bpf_verifier_log *log,
>  		      const struct btf_type *t, int off, int size,
>  		      enum bpf_access_type atype,
>  		      u32 *next_btf_id);
> +int btf_struct_address(struct bpf_verifier_log *log,
> +		     const struct btf_type *t,
> +		     u32 off, u32 exp_id);
>  int btf_resolve_helper_id(struct bpf_verifier_log *log,
>  			  const struct bpf_func_proto *fn, int);
>  
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index d65c6912bdaf..9aafffa57d8b 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -4002,6 +4002,75 @@ int btf_struct_access(struct bpf_verifier_log *log,
>  	return -EINVAL;
>  }
>  
> +int btf_struct_address(struct bpf_verifier_log *log,
> +		       const struct btf_type *t,
> +		       u32 off, u32 exp_id)
> +{
> +	u32 i, moff, mtrue_end, msize = 0;
> +	const struct btf_member *member;
> +	const struct btf_type *mtype;
> +	const char *tname, *mname;
> +
> +again:
> +	tname = __btf_name_by_offset(btf_vmlinux, t->name_off);
> +	if (!btf_type_is_struct(t)) {
> +		bpf_log(log, "Type '%s' is not a struct\n", tname);
> +		return -EINVAL;
> +	}
> +
> +	if (off > t->size) {
> +		bpf_log(log, "address beyond struct %s at off %u size %u\n",
> +			tname, off, t->size);
> +		return -EACCES;
> +	}
> +
> +	for_each_member(i, t, member) {
> +		/* offset of the field in bytes */
> +		moff = btf_member_bit_offset(t, member) / 8;
> +		if (off < moff)
> +			/* won't find anything, field is already too far */
> +			break;
> +
> +		/* we found the member */
> +		if (off == moff && member->type == exp_id)
> +			return 0;
> +
> +		/* type of the field */
> +		mtype = btf_type_by_id(btf_vmlinux, member->type);
> +		mname = __btf_name_by_offset(btf_vmlinux, member->name_off);
> +
> +		mtype = btf_resolve_size(btf_vmlinux, mtype, &msize,
> +					 NULL, NULL);
> +		if (IS_ERR(mtype)) {
> +			bpf_log(log, "field %s doesn't have size\n", mname);
> +			return -EFAULT;
> +		}
> +
> +		mtrue_end = moff + msize;
> +		if (off >= mtrue_end)
> +			/* no overlap with member, keep iterating */
> +			continue;
> +
> +		/* the 'off' we're looking for is either equal to start
> +		 * of this field or inside of this struct
> +		 */
> +		if (btf_type_is_struct(mtype)) {
> +			/* our field must be inside that union or struct */
> +			t = mtype;
> +
> +			/* adjust offset we're looking for */
> +			off -= moff;
> +			goto again;
> +		}

Looks like copy-paste that should be generalized into common helper.

> +
> +		bpf_log(log, "struct %s doesn't have struct field at offset %d\n", tname, off);
> +		return -EACCES;
> +	}
> +
> +	bpf_log(log, "struct %s doesn't have field at offset %d\n", tname, off);
> +	return -EACCES;
> +}
> +
>  static int __btf_resolve_helper_id(struct bpf_verifier_log *log, void *fn,
>  				   int arg)
>  {
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 04c6630cc18f..6eb88bef4379 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3103,6 +3103,18 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
>  	return 0;
>  }
>  
> +static void check_ptr_in_btf(struct bpf_verifier_env *env,
> +			     struct bpf_reg_state *reg,
> +			     u32 exp_id)
> +{
> +	const struct btf_type *t = btf_type_by_id(btf_vmlinux, reg->btf_id);
> +
> +	if (!btf_struct_address(&env->log, t, reg->off, exp_id)) {
> +		reg->btf_id = exp_id;
> +		reg->off = 0;

This doesn't look right.
If you simply overwrite btf_id and off in the reg it will contain wrong info
in any subsequent instruction.
Typically it would be ok, since this reg is a function argument and will be
scratched after the call, but consider:
bpf_foo(&file->f_path, &file->f_owner);
The same base register will be used to construct R1 and R2
and above re-assign will screw up R1.

Re: [PATCH 1/3] bpf: Add support to check if BTF object is nested in another object

[Jiri Olsa]

On Mon, Apr 06, 2020 at 06:16:01PM -0700, Alexei Starovoitov wrote:

SNIP

> > +			continue;
> > +
> > +		/* the 'off' we're looking for is either equal to start
> > +		 * of this field or inside of this struct
> > +		 */
> > +		if (btf_type_is_struct(mtype)) {
> > +			/* our field must be inside that union or struct */
> > +			t = mtype;
> > +
> > +			/* adjust offset we're looking for */
> > +			off -= moff;
> > +			goto again;
> > +		}
> 
> Looks like copy-paste that should be generalized into common helper.

right, I think we could have some common code with btf_struct_access,
but id not want to complicate the change for rfc

> 
> > +
> > +		bpf_log(log, "struct %s doesn't have struct field at offset %d\n", tname, off);
> > +		return -EACCES;
> > +	}
> > +
> > +	bpf_log(log, "struct %s doesn't have field at offset %d\n", tname, off);
> > +	return -EACCES;
> > +}
> > +
> >  static int __btf_resolve_helper_id(struct bpf_verifier_log *log, void *fn,
> >  				   int arg)
> >  {
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 04c6630cc18f..6eb88bef4379 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -3103,6 +3103,18 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
> >  	return 0;
> >  }
> >  
> > +static void check_ptr_in_btf(struct bpf_verifier_env *env,
> > +			     struct bpf_reg_state *reg,
> > +			     u32 exp_id)
> > +{
> > +	const struct btf_type *t = btf_type_by_id(btf_vmlinux, reg->btf_id);
> > +
> > +	if (!btf_struct_address(&env->log, t, reg->off, exp_id)) {
> > +		reg->btf_id = exp_id;
> > +		reg->off = 0;
> 
> This doesn't look right.
> If you simply overwrite btf_id and off in the reg it will contain wrong info
> in any subsequent instruction.
> Typically it would be ok, since this reg is a function argument and will be
> scratched after the call, but consider:
> bpf_foo(&file->f_path, &file->f_owner);
> The same base register will be used to construct R1 and R2
> and above re-assign will screw up R1.

ok.. I'll use the 'new btf id' just to do check on the helper args
and not change the reg's btf id

thanks,
jirka

[PATCH 2/3] bpf: Add d_path helper

[Jiri Olsa]

Adding d_path helper function that returns full path
for give 'struct path' object, which needs to be the
kernel BTF 'path' object.

The helper calls directly d_path function.

Updating also bpf.h tools uapi header and adding
'path' to bpf_helpers_doc.py script.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/uapi/linux/bpf.h       | 14 +++++++++++++-
 kernel/trace/bpf_trace.c       | 31 +++++++++++++++++++++++++++++++
 scripts/bpf_helpers_doc.py     |  2 ++
 tools/include/uapi/linux/bpf.h | 14 +++++++++++++-
 4 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 2e29a671d67e..8da1b4750364 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3025,6 +3025,17 @@ union bpf_attr {
  *		* **-EOPNOTSUPP**	Unsupported operation, for example a
  *					call from outside of TC ingress.
  *		* **-ESOCKTNOSUPPORT**	Socket type not supported (reuseport).
+ *
+ * int bpf_d_path(struct path *path, char *buf, u32 sz)
+ *	Description
+ *		Return full path for given 'struct path' object, which
+ *		needs to be the kernel BTF 'path' object. The path is
+ *		returned in buffer provided 'buf' of size 'sz'.
+ *
+ *	Return
+ *		length of returned string on success, or a negative
+ *		error in case of failure
+ *
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -3151,7 +3162,8 @@ union bpf_attr {
 	FN(xdp_output),			\
 	FN(get_netns_cookie),		\
 	FN(get_current_ancestor_cgroup_id),	\
-	FN(sk_assign),
+	FN(sk_assign),			\
+	FN(d_path),
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index ca1796747a77..6ca390b2b26e 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -779,6 +779,35 @@ static const struct bpf_func_proto bpf_send_signal_thread_proto = {
 	.arg1_type	= ARG_ANYTHING,
 };
 
+BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz)
+{
+	char *p = d_path(path, buf, sz - 1);
+	int len;
+
+	if (IS_ERR(p)) {
+		len = PTR_ERR(p);
+	} else {
+		len = strlen(p);
+		if (len && p != buf) {
+			memmove(buf, p, len);
+			buf[len] = 0;
+		}
+	}
+
+	return len;
+}
+
+static u32 bpf_d_path_btf_ids[3];
+static const struct bpf_func_proto bpf_d_path_proto = {
+	.func		= bpf_d_path,
+	.gpl_only	= true,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_PTR_TO_BTF_ID,
+	.arg2_type	= ARG_PTR_TO_MEM,
+	.arg3_type	= ARG_CONST_SIZE,
+	.btf_id		= bpf_d_path_btf_ids,
+};
+
 const struct bpf_func_proto *
 bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
@@ -1224,6 +1253,8 @@ tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 	case BPF_FUNC_xdp_output:
 		return &bpf_xdp_output_proto;
 #endif
+	case BPF_FUNC_d_path:
+		return &bpf_d_path_proto;
 	default:
 		return raw_tp_prog_func_proto(func_id, prog);
 	}
diff --git a/scripts/bpf_helpers_doc.py b/scripts/bpf_helpers_doc.py
index f43d193aff3a..8f62cbc4c3ff 100755
--- a/scripts/bpf_helpers_doc.py
+++ b/scripts/bpf_helpers_doc.py
@@ -418,6 +418,7 @@ class PrinterHelpers(Printer):
             'struct __sk_buff',
             'struct sk_msg_md',
             'struct xdp_md',
+            'struct path',
     ]
     known_types = {
             '...',
@@ -450,6 +451,7 @@ class PrinterHelpers(Printer):
             'struct sk_reuseport_md',
             'struct sockaddr',
             'struct tcphdr',
+            'struct path',
     }
     mapped_types = {
             'u8': '__u8',
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 2e29a671d67e..8da1b4750364 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -3025,6 +3025,17 @@ union bpf_attr {
  *		* **-EOPNOTSUPP**	Unsupported operation, for example a
  *					call from outside of TC ingress.
  *		* **-ESOCKTNOSUPPORT**	Socket type not supported (reuseport).
+ *
+ * int bpf_d_path(struct path *path, char *buf, u32 sz)
+ *	Description
+ *		Return full path for given 'struct path' object, which
+ *		needs to be the kernel BTF 'path' object. The path is
+ *		returned in buffer provided 'buf' of size 'sz'.
+ *
+ *	Return
+ *		length of returned string on success, or a negative
+ *		error in case of failure
+ *
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -3151,7 +3162,8 @@ union bpf_attr {
 	FN(xdp_output),			\
 	FN(get_netns_cookie),		\
 	FN(get_current_ancestor_cgroup_id),	\
-	FN(sk_assign),
+	FN(sk_assign),			\
+	FN(d_path),
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call
-- 
2.25.2

Re: [PATCH 2/3] bpf: Add d_path helper

[Florent Revest]

On Wed, 2020-04-01 at 13:09 +0200, Jiri Olsa wrote:
> + * int bpf_d_path(struct path *path, char *buf, u32 sz)
> + *	Description
> + *		Return full path for given 'struct path' object, which
> + *		needs to be the kernel BTF 'path' object. The path is
> + *		returned in buffer provided 'buf' of size 'sz'.
> + *
> + *	Return
> + *		length of returned string on success, or a negative
> + *		error in case of failure
> + *

You might want to add that d_path is ambiguous since it can add
" (deleted)" at the end of your path and you don't know whether this is
actually part of the file path or not. :) 

> +BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz)
> +{
> +	char *p = d_path(path, buf, sz - 1);

I am curious why you'd use sz - 1 here? In my experience, d_path's
output is 0 limited so you shouldn't need to keep an extra byte for
that (if that was the intention here).

> +	int len;
> +
> +	if (IS_ERR(p)) {
> +		len = PTR_ERR(p);
> +	} else {
> +		len = strlen(p);
> +		if (len && p != buf) {
> +			memmove(buf, p, len);

Have you considered returning the offset within buf instead and let the
BPF program do pointer arithmetics to find the beginning of the string?

> +			buf[len] = 0;

If my previous comment about sz - 1 is true, then this wouldn't be
necessary, you could just use memmove with len + 1.

> +		}
> +	}
> +
> +	return len;
> +}

Re: [PATCH 2/3] bpf: Add d_path helper

[Jiri Olsa]

On Thu, Apr 02, 2020 at 04:02:55PM +0200, Florent Revest wrote:
> On Wed, 2020-04-01 at 13:09 +0200, Jiri Olsa wrote:
> > + * int bpf_d_path(struct path *path, char *buf, u32 sz)
> > + *	Description
> > + *		Return full path for given 'struct path' object, which
> > + *		needs to be the kernel BTF 'path' object. The path is
> > + *		returned in buffer provided 'buf' of size 'sz'.
> > + *
> > + *	Return
> > + *		length of returned string on success, or a negative
> > + *		error in case of failure
> > + *
> 
> You might want to add that d_path is ambiguous since it can add
> " (deleted)" at the end of your path and you don't know whether this is
> actually part of the file path or not. :) 

right

> 
> > +BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz)
> > +{
> > +	char *p = d_path(path, buf, sz - 1);
> 
> I am curious why you'd use sz - 1 here? In my experience, d_path's
> output is 0 limited so you shouldn't need to keep an extra byte for
> that (if that was the intention here).
> 
> > +	int len;
> > +
> > +	if (IS_ERR(p)) {
> > +		len = PTR_ERR(p);
> > +	} else {
> > +		len = strlen(p);
> > +		if (len && p != buf) {
> > +			memmove(buf, p, len);
> 
> Have you considered returning the offset within buf instead and let the
> BPF program do pointer arithmetics to find the beginning of the string?

we could do that.. I was following some other user of d_path,
which I can't find at the moment ;-) I'll check

> 
> > +			buf[len] = 0;
> 
> If my previous comment about sz - 1 is true, then this wouldn't be
> necessary, you could just use memmove with len + 1.

hum, you might be right, I'll check on this

thanks,
jirka

> 
> > +		}
> > +	}
> > +
> > +	return len;
> > +}
> 

Re: [PATCH 2/3] bpf: Add d_path helper

[Andrii Nakryiko]

On Fri, Apr 3, 2020 at 2:03 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Thu, Apr 02, 2020 at 04:02:55PM +0200, Florent Revest wrote:
> > On Wed, 2020-04-01 at 13:09 +0200, Jiri Olsa wrote:
> > > + * int bpf_d_path(struct path *path, char *buf, u32 sz)
> > > + * Description
> > > + *         Return full path for given 'struct path' object, which
> > > + *         needs to be the kernel BTF 'path' object. The path is
> > > + *         returned in buffer provided 'buf' of size 'sz'.
> > > + *
> > > + * Return
> > > + *         length of returned string on success, or a negative
> > > + *         error in case of failure
> > > + *
> >
> > You might want to add that d_path is ambiguous since it can add
> > " (deleted)" at the end of your path and you don't know whether this is
> > actually part of the file path or not. :)
>
> right
>
> >
> > > +BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz)
> > > +{
> > > +   char *p = d_path(path, buf, sz - 1);
> >
> > I am curious why you'd use sz - 1 here? In my experience, d_path's
> > output is 0 limited so you shouldn't need to keep an extra byte for
> > that (if that was the intention here).
> >
> > > +   int len;
> > > +
> > > +   if (IS_ERR(p)) {
> > > +           len = PTR_ERR(p);
> > > +   } else {
> > > +           len = strlen(p);
> > > +           if (len && p != buf) {
> > > +                   memmove(buf, p, len);
> >
> > Have you considered returning the offset within buf instead and let the
> > BPF program do pointer arithmetics to find the beginning of the string?
>
> we could do that.. I was following some other user of d_path,
> which I can't find at the moment ;-) I'll check

This would make it hard to support variable-length data encoding and
sending it over perf_buffer, because it would prevent back-to-back
"stitching" of multiple strings compactly in output buffer. So I think
current approach is preferable.

>
> >
> > > +                   buf[len] = 0;
> >
> > If my previous comment about sz - 1 is true, then this wouldn't be
> > necessary, you could just use memmove with len + 1.
>
> hum, you might be right, I'll check on this
>
> thanks,
> jirka
>
> >
> > > +           }
> > > +   }
> > > +
> > > +   return len;
> > > +}
> >
>

[PATCH 3/3] selftests/bpf: Add test for d_path helper

[Jiri Olsa]

Adding test for d_path helper which is pretty much
copied from Wenbo Zhang's test for bpf_get_fd_path,
which never made it in.

I've failed so far to compile the test with <linux/fs.h>
kernel header, so for now adding 'struct file' with f_path
member that has same offset as kernel's file object.

Original-patch-by: Wenbo Zhang <ethercflow@gmail.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 .../testing/selftests/bpf/prog_tests/d_path.c | 196 ++++++++++++++++++
 .../testing/selftests/bpf/progs/test_d_path.c |  71 +++++++
 2 files changed, 267 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/d_path.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_d_path.c

diff --git a/tools/testing/selftests/bpf/prog_tests/d_path.c b/tools/testing/selftests/bpf/prog_tests/d_path.c
new file mode 100644
index 000000000000..6b69bfda6c19
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/d_path.c
@@ -0,0 +1,196 @@
+// SPDX-License-Identifier: GPL-2.0
+#define _GNU_SOURCE
+#include <test_progs.h>
+#include <sys/stat.h>
+#include <linux/sched.h>
+#include <sys/syscall.h>
+
+#define MAX_PATH_LEN		128
+#define MAX_FILES		7
+#define MAX_EVENT_NUM		16
+
+static struct d_path_test_data {
+	pid_t pid;
+	__u32 cnt_stat;
+	__u32 cnt_close;
+	char paths_stat[MAX_EVENT_NUM][MAX_PATH_LEN];
+	char paths_close[MAX_EVENT_NUM][MAX_PATH_LEN];
+} dst;
+
+static struct {
+	__u32 cnt;
+	char paths[MAX_EVENT_NUM][MAX_PATH_LEN];
+} src;
+
+static int set_pathname(int fd, pid_t pid)
+{
+	char buf[MAX_PATH_LEN];
+
+	snprintf(buf, MAX_PATH_LEN, "/proc/%d/fd/%d", pid, fd);
+	return readlink(buf, src.paths[src.cnt++], MAX_PATH_LEN);
+}
+
+static int trigger_fstat_events(pid_t pid)
+{
+	int sockfd = -1, procfd = -1, devfd = -1;
+	int localfd = -1, indicatorfd = -1;
+	int pipefd[2] = { -1, -1 };
+	struct stat fileStat;
+	int ret = -1;
+
+	/* unmountable pseudo-filesystems */
+	if (CHECK_FAIL(pipe(pipefd) < 0))
+		return ret;
+	/* unmountable pseudo-filesystems */
+	sockfd = socket(AF_INET, SOCK_STREAM, 0);
+	if (CHECK_FAIL(sockfd < 0))
+		goto out_close;
+	/* mountable pseudo-filesystems */
+	procfd = open("/proc/self/comm", O_RDONLY);
+	if (CHECK_FAIL(procfd < 0))
+		goto out_close;
+	devfd = open("/dev/urandom", O_RDONLY);
+	if (CHECK_FAIL(devfd < 0))
+		goto out_close;
+	localfd = open("/tmp/d_path_loadgen.txt", O_CREAT | O_RDONLY);
+	if (CHECK_FAIL(localfd < 0))
+		goto out_close;
+	/* bpf_d_path will return path with (deleted) */
+	remove("/tmp/d_path_loadgen.txt");
+	indicatorfd = open("/tmp/", O_PATH);
+	if (CHECK_FAIL(indicatorfd < 0))
+		goto out_close;
+
+	ret = set_pathname(pipefd[0], pid);
+	if (CHECK_FAIL(ret < 0))
+		goto out_close;
+	ret = set_pathname(pipefd[1], pid);
+	if (CHECK_FAIL(ret < 0))
+		goto out_close;
+	ret = set_pathname(sockfd, pid);
+	if (CHECK_FAIL(ret < 0))
+		goto out_close;
+	ret = set_pathname(procfd, pid);
+	if (CHECK_FAIL(ret < 0))
+		goto out_close;
+	ret = set_pathname(devfd, pid);
+	if (CHECK_FAIL(ret < 0))
+		goto out_close;
+	ret = set_pathname(localfd, pid);
+	if (CHECK_FAIL(ret < 0))
+		goto out_close;
+	ret = set_pathname(indicatorfd, pid);
+	if (CHECK_FAIL(ret < 0))
+		goto out_close;
+
+	/* triggers vfs_getattr */
+	fstat(pipefd[0], &fileStat);
+	fstat(pipefd[1], &fileStat);
+	fstat(sockfd, &fileStat);
+	fstat(procfd, &fileStat);
+	fstat(devfd, &fileStat);
+	fstat(localfd, &fileStat);
+	fstat(indicatorfd, &fileStat);
+
+out_close:
+	/* triggers filp_close */
+	close(pipefd[0]);
+	close(pipefd[1]);
+	close(sockfd);
+	close(procfd);
+	close(devfd);
+	close(localfd);
+	close(indicatorfd);
+	return ret;
+}
+
+void test_d_path(void)
+{
+	const char *prog_name_1 = "fentry/vfs_getattr";
+	const char *prog_name_2 = "fentry/filp_close";
+	const char *obj_file = "test_d_path.o";
+	int err, results_map_fd, duration = 0;
+	struct bpf_program *tp_prog1 = NULL;
+	struct bpf_program *tp_prog2 = NULL;
+	struct bpf_link *tp_link1 = NULL;
+	struct bpf_link *tp_link2 = NULL;
+	struct bpf_object *obj = NULL;
+	const int zero = 0;
+
+	obj = bpf_object__open_file(obj_file, NULL);
+	if (CHECK(IS_ERR(obj), "obj_open_file", "err %ld\n", PTR_ERR(obj)))
+		return;
+
+	err = bpf_object__load(obj);
+	if (CHECK(err, "obj_load", "err %d\n", err))
+		goto cleanup;
+
+	tp_prog1 = bpf_object__find_program_by_title(obj, prog_name_1);
+	if (CHECK(!tp_prog1, "find_tp",
+		  "prog '%s' not found\n", prog_name_1))
+		goto cleanup;
+
+	tp_prog2 = bpf_object__find_program_by_title(obj, prog_name_2);
+	if (CHECK(!tp_prog2, "find_tp",
+		  "prog '%s' not found\n", prog_name_2))
+		goto cleanup;
+
+	tp_link1 = bpf_program__attach_trace(tp_prog1);
+	if (CHECK(IS_ERR(tp_link1), "attach_tp",
+		  "err %ld\n", PTR_ERR(tp_link1))) {
+		tp_link1 = NULL;
+		goto cleanup;
+	}
+
+	tp_link2 = bpf_program__attach_trace(tp_prog2);
+	if (CHECK(IS_ERR(tp_link2), "attach_tp",
+		  "err %ld\n", PTR_ERR(tp_link2))) {
+		tp_link2 = NULL;
+		goto cleanup;
+	}
+
+	results_map_fd = bpf_find_map(__func__, obj, "test_d_p.bss");
+	if (CHECK(results_map_fd < 0, "find_bss_map",
+		  "err %d\n", results_map_fd))
+		goto cleanup;
+
+	dst.pid = getpid();
+	err = bpf_map_update_elem(results_map_fd, &zero, &dst, 0);
+	if (CHECK(err, "update_elem",
+		  "failed to set pid filter: %d\n", err))
+		goto cleanup;
+
+	err = trigger_fstat_events(dst.pid);
+	if (CHECK_FAIL(err < 0))
+		goto cleanup;
+
+	err = bpf_map_lookup_elem(results_map_fd, &zero, &dst);
+	if (CHECK(err, "get_results",
+		  "failed to get results: %d\n", err))
+		goto cleanup;
+
+	for (int i = 0; i < MAX_FILES; i++) {
+		if (i < 3) {
+			CHECK((dst.paths_stat[i][0] == 0), "d_path",
+			      "failed to filter fs [%d]: %s vs %s\n",
+			      i, src.paths[i], dst.paths_stat[i]);
+			CHECK((dst.paths_close[i][0] == 0), "d_path",
+			      "failed to filter fs [%d]: %s vs %s\n",
+			      i, src.paths[i], dst.paths_close[i]);
+		} else {
+			CHECK(strncmp(src.paths[i], dst.paths_stat[i], MAX_PATH_LEN),
+			      "d_path",
+			      "failed to get stat path[%d]: %s vs %s\n",
+			      i, src.paths[i], dst.paths_stat[i]);
+			CHECK(strncmp(src.paths[i], dst.paths_close[i], MAX_PATH_LEN),
+			      "d_path",
+			      "failed to get close path[%d]: %s vs %s\n",
+			      i, src.paths[i], dst.paths_close[i]);
+		}
+	}
+
+cleanup:
+	bpf_link__destroy(tp_link2);
+	bpf_link__destroy(tp_link1);
+	bpf_object__close(obj);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_d_path.c b/tools/testing/selftests/bpf/progs/test_d_path.c
new file mode 100644
index 000000000000..f75c108a5773
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_d_path.c
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include <linux/ptrace.h>
+#include <linux/fs.h>
+#include <string.h>
+#include <unistd.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include <linux/fs.h>
+
+#define MAX_PATH_LEN		128
+#define MAX_EVENT_NUM		16
+
+static struct d_path_test_data {
+	pid_t pid;
+	__u32 cnt_stat;
+	__u32 cnt_close;
+	char paths_stat[MAX_EVENT_NUM][MAX_PATH_LEN];
+	char paths_close[MAX_EVENT_NUM][MAX_PATH_LEN];
+} data;
+
+struct path;
+struct kstat;
+
+SEC("fentry/vfs_getattr")
+int BPF_PROG(prog_stat, struct path *path, struct kstat *stat,
+	     __u32 request_mask, unsigned int query_flags)
+{
+	pid_t pid = bpf_get_current_pid_tgid() >> 32;
+
+	if (pid != data.pid)
+		return 0;
+
+	if (data.cnt_stat >= MAX_EVENT_NUM)
+		return 0;
+
+	bpf_d_path(path, data.paths_stat[data.cnt_stat], MAX_PATH_LEN);
+	data.cnt_stat++;
+	return 0;
+}
+
+/*
+ * TODO
+ * I've failed so far to compile the test with <linux/fs.h>
+ * kernel header, so for now adding 'struct file' with f_path
+ * member that has same offset as kernel's file object.
+ */
+struct file {
+	__u64	 foo1;
+	__u64	 foo2;
+	void	*f_path;
+};
+
+SEC("fentry/filp_close")
+int BPF_PROG(prog_close, struct file *file, void *id)
+{
+	pid_t pid = bpf_get_current_pid_tgid() >> 32;
+
+	if (pid != data.pid)
+		return 0;
+
+	if (data.cnt_close >= MAX_EVENT_NUM)
+		return 0;
+
+	bpf_d_path((struct path *) &file->f_path, data.paths_close[data.cnt_close], MAX_PATH_LEN);
+	data.cnt_close++;
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.25.2

Re: [RFC 0/3] bpf: Add d_path helper

[Florent Revest]

On Wed, 2020-04-01 at 13:09 +0200, Jiri Olsa wrote:
> hi,
> adding d_path helper to return full path for 'path' object.
> 
> I originally added and used 'file_path' helper, which did the same,
> but used 'struct file' object. Then realized that file_path is just
> a wrapper for d_path, so we'd cover more calling sites if we add
> d_path helper and allowed resolving BTF object within another object,
> so we could call d_path also with file pointer, like:
> 
>   bpf_d_path(&file->f_path, buf, size);
> 
> This feature is mainly to be able to add dpath (filepath originally)
> function to bpftrace, which seems to work nicely now, like:
> 
>   # bpftrace -e 'kretfunc:fget { printf("%s\n", dpath(args->ret-
> >f_path));  }' 
> 
> I'm not completely sure this is all safe and bullet proof and there's
> no other way to do this, hence RFC post.
> 
> I'd be happy also with file_path function, but I thought it'd be
> a shame not to try to add d_path with the verifier change.
> I'm open to any suggestions ;-)

First of all I want to mention that we are really interested in this
feature so thanks a lot for bringing it up Jiri! I have experimented
with similar BPF helpers in the past few months so I hope my input can
be helpful! :)

One of our use-cases is to gather information about execution events,
including a bunch of paths (such as the executable command, the
resolved executable file path and the current-working-directory) and
then output them to Perf.
Each of those paths can be up to PATH_MAX(one page) long so we would
pre-allocate a data structure with a few identifiers (to later
reassemble the event from userspace) and a page of data and then we
would output it using bpf_perf_event_output. However, with three mostly
empty pages per event, we would quickly fill up the ring buffer and
loose many events.
This might be a bit out-of-scope at this moment but one of the
teachings we got from playing with such a helper is that we would also
need a helper for outputting strings to Perf, pre-pended with a header
buffer.

Re: [RFC 0/3] bpf: Add d_path helper

[Jiri Olsa]

On Thu, Apr 02, 2020 at 04:03:00PM +0200, Florent Revest wrote:
> On Wed, 2020-04-01 at 13:09 +0200, Jiri Olsa wrote:
> > hi,
> > adding d_path helper to return full path for 'path' object.
> > 
> > I originally added and used 'file_path' helper, which did the same,
> > but used 'struct file' object. Then realized that file_path is just
> > a wrapper for d_path, so we'd cover more calling sites if we add
> > d_path helper and allowed resolving BTF object within another object,
> > so we could call d_path also with file pointer, like:
> > 
> >   bpf_d_path(&file->f_path, buf, size);
> > 
> > This feature is mainly to be able to add dpath (filepath originally)
> > function to bpftrace, which seems to work nicely now, like:
> > 
> >   # bpftrace -e 'kretfunc:fget { printf("%s\n", dpath(args->ret-
> > >f_path));  }' 
> > 
> > I'm not completely sure this is all safe and bullet proof and there's
> > no other way to do this, hence RFC post.
> > 
> > I'd be happy also with file_path function, but I thought it'd be
> > a shame not to try to add d_path with the verifier change.
> > I'm open to any suggestions ;-)
> 
> First of all I want to mention that we are really interested in this
> feature so thanks a lot for bringing it up Jiri! I have experimented
> with similar BPF helpers in the past few months so I hope my input can
> be helpful! :)
> 
> One of our use-cases is to gather information about execution events,
> including a bunch of paths (such as the executable command, the
> resolved executable file path and the current-working-directory) and
> then output them to Perf.
> Each of those paths can be up to PATH_MAX(one page) long so we would
> pre-allocate a data structure with a few identifiers (to later
> reassemble the event from userspace) and a page of data and then we
> would output it using bpf_perf_event_output. However, with three mostly
> empty pages per event, we would quickly fill up the ring buffer and
> loose many events.
> This might be a bit out-of-scope at this moment but one of the
> teachings we got from playing with such a helper is that we would also
> need a helper for outputting strings to Perf, pre-pended with a header
> buffer.

I think bpftrace uses fixed size as well at some point,
but very small one, which is still sufficent for tools usage,
but we can always send only data with the size of the path

thanks for info
jirka

Re: [RFC 0/3] bpf: Add d_path helper

[Al Viro]

On Wed, Apr 01, 2020 at 01:09:04PM +0200, Jiri Olsa wrote:
> hi,
> adding d_path helper to return full path for 'path' object.
> 
> I originally added and used 'file_path' helper, which did the same,
> but used 'struct file' object. Then realized that file_path is just
> a wrapper for d_path, so we'd cover more calling sites if we add
> d_path helper and allowed resolving BTF object within another object,
> so we could call d_path also with file pointer, like:
> 
>   bpf_d_path(&file->f_path, buf, size);
> 
> This feature is mainly to be able to add dpath (filepath originally)
> function to bpftrace, which seems to work nicely now, like:
> 
>   # bpftrace -e 'kretfunc:fget { printf("%s\n", dpath(args->ret->f_path));  }' 
> 
> I'm not completely sure this is all safe and bullet proof and there's
> no other way to do this, hence RFC post.
> 
> I'd be happy also with file_path function, but I thought it'd be
> a shame not to try to add d_path with the verifier change.
> I'm open to any suggestions ;-)

What are the locking conditions guaranteed to that sucker?  Note that d_path()
is *NOT* lockless - call it from an interrupt/NMI/etc. and you are fucked.
It can grab rename_lock and mount_lock; usually it avoids that, so you won't
see them grabbed on every call, but after the first seqlock mismatch it will
fall back to grabbing the spinlock in question.  And then there's ->d_dname(),
with whatever things _that_ chooses to do....

Re: [RFC 0/3] bpf: Add d_path helper

[Jiri Olsa]

On Thu, Apr 02, 2020 at 03:21:06PM +0100, Al Viro wrote:
> On Wed, Apr 01, 2020 at 01:09:04PM +0200, Jiri Olsa wrote:
> > hi,
> > adding d_path helper to return full path for 'path' object.
> > 
> > I originally added and used 'file_path' helper, which did the same,
> > but used 'struct file' object. Then realized that file_path is just
> > a wrapper for d_path, so we'd cover more calling sites if we add
> > d_path helper and allowed resolving BTF object within another object,
> > so we could call d_path also with file pointer, like:
> > 
> >   bpf_d_path(&file->f_path, buf, size);
> > 
> > This feature is mainly to be able to add dpath (filepath originally)
> > function to bpftrace, which seems to work nicely now, like:
> > 
> >   # bpftrace -e 'kretfunc:fget { printf("%s\n", dpath(args->ret->f_path));  }' 
> > 
> > I'm not completely sure this is all safe and bullet proof and there's
> > no other way to do this, hence RFC post.
> > 
> > I'd be happy also with file_path function, but I thought it'd be
> > a shame not to try to add d_path with the verifier change.
> > I'm open to any suggestions ;-)
> 
> What are the locking conditions guaranteed to that sucker?  Note that d_path()
> is *NOT* lockless - call it from an interrupt/NMI/etc. and you are fucked.
> It can grab rename_lock and mount_lock; usually it avoids that, so you won't
> see them grabbed on every call, but after the first seqlock mismatch it will
> fall back to grabbing the spinlock in question.  And then there's ->d_dname(),
> with whatever things _that_ chooses to do....

if we limit it just to task context I think it would still be
helpful for us:

  if (in_task())
	d_path..

perhaps even create a d_path version without d_dname callback
if that'd be still a problem, because it seems to be there mainly
for special filesystems..?

thanks,
jirka

Re: [RFC 0/3] bpf: Add d_path helper

[Al Viro]

On Fri, Apr 03, 2020 at 11:08:28AM +0200, Jiri Olsa wrote:

> if we limit it just to task context I think it would still be
> helpful for us:
> 
>   if (in_task())
> 	d_path..
> 
> perhaps even create a d_path version without d_dname callback
> if that'd be still a problem, because it seems to be there mainly
> for special filesystems..?

IDGI...
	1) d_path(), by definition, is dependent upon the
process' root - the same <mount,dentry> pair will yield
different strings if caller is chrooted.  You *can't* just
use a random process' root
	2) we are *NOT* making rename_lock and mount_lock
disable interrupts.  Not happening.

So it has to be process-synchronous anyway.  Could you describe
where that thing is going to be callable?

Re: [RFC 0/3] bpf: Add d_path helper

[Jiri Olsa]

On Mon, Apr 06, 2020 at 04:16:02AM +0100, Al Viro wrote:
> On Fri, Apr 03, 2020 at 11:08:28AM +0200, Jiri Olsa wrote:
> 
> > if we limit it just to task context I think it would still be
> > helpful for us:
> > 
> >   if (in_task())
> > 	d_path..
> > 
> > perhaps even create a d_path version without d_dname callback
> > if that'd be still a problem, because it seems to be there mainly
> > for special filesystems..?
> 
> IDGI...
> 	1) d_path(), by definition, is dependent upon the
> process' root - the same <mount,dentry> pair will yield
> different strings if caller is chrooted.  You *can't* just
> use a random process' root
> 	2) we are *NOT* making rename_lock and mount_lock
> disable interrupts.  Not happening.
> 
> So it has to be process-synchronous anyway.  Could you describe
> where that thing is going to be callable?

it could be called as bpf helper from any place we could put
the trampoline probe on.. so most of the kernel functions
(at entry or exit) .. we can make checks, like for context
before we allow to call it

is there any way we could have d_path functionality (even
reduced and not working for all cases) that could be used
or called like that?

thanks,
jirka

Re: [RFC 0/3] bpf: Add d_path helper

[Al Viro]

On Mon, Apr 06, 2020 at 11:09:18AM +0200, Jiri Olsa wrote:

> it could be called as bpf helper from any place we could put
> the trampoline probe on.. so most of the kernel functions
> (at entry or exit) .. we can make checks, like for context
> before we allow to call it

Hard NAK, then.  If you can insert its call at the entry to
e.g. umount_tree(), you will get deadlocks.  The same for e.g.
select_collect() (same effect on a different seqlock).

Re: [RFC 0/3] bpf: Add d_path helper

[Alexei Starovoitov]

On Mon, Apr 06, 2020 at 11:09:18AM +0200, Jiri Olsa wrote:
> 
> is there any way we could have d_path functionality (even
> reduced and not working for all cases) that could be used
> or called like that?

I agree with Al. This helper cannot be enabled for all of bpf tracing.
We have to white list its usage for specific callsites only.
May be all of lsm hooks are safe. I don't know yet. This has to be
analyzed carefully. Every hook. One by one.
in_task() isn't really a solution.

At the same time I agree that such helper is badly needed.
Folks have been requesting it for long time.

Re: [RFC 0/3] bpf: Add d_path helper

[Jiri Olsa]

On Mon, Apr 06, 2020 at 06:10:52PM -0700, Alexei Starovoitov wrote:
> On Mon, Apr 06, 2020 at 11:09:18AM +0200, Jiri Olsa wrote:
> > 
> > is there any way we could have d_path functionality (even
> > reduced and not working for all cases) that could be used
> > or called like that?
> 
> I agree with Al. This helper cannot be enabled for all of bpf tracing.
> We have to white list its usage for specific callsites only.
> May be all of lsm hooks are safe. I don't know yet. This has to be
> analyzed carefully. Every hook. One by one.
> in_task() isn't really a solution.

ok, I thought of white list and it seemed too much to me,
but there's probably no other way

jirka

> 
> At the same time I agree that such helper is badly needed.
> Folks have been requesting it for long time.
> 

Re: [RFC 0/3] bpf: Add d_path helper

[KP Singh]

On 06-Apr 18:10, Alexei Starovoitov wrote:
> On Mon, Apr 06, 2020 at 11:09:18AM +0200, Jiri Olsa wrote:
> > 
> > is there any way we could have d_path functionality (even
> > reduced and not working for all cases) that could be used
> > or called like that?
> 
> I agree with Al. This helper cannot be enabled for all of bpf tracing.
> We have to white list its usage for specific callsites only.
> May be all of lsm hooks are safe. I don't know yet. This has to be
> analyzed carefully. Every hook. One by one.

I agree with this, there are some LSM hooks which do get called in
interrupt context, eg. task_free (which gets called in an RCU
callback).

The hooks that we are using it for and we know that it works (using
our experimental helpers similar to this) are the bprm_* hooks in the
exec pathway (for logic based on the path of the executable).

It might be worth whitelisting these functions by adding verifier ops
for LSM programs?

Would you want to do it as a part of this series?

- KP

> in_task() isn't really a solution.
> 
> At the same time I agree that such helper is badly needed.
> Folks have been requesting it for long time.

Re: [RFC 0/3] bpf: Add d_path helper

[Jiri Olsa]

On Tue, Apr 07, 2020 at 11:27:53AM +0200, KP Singh wrote:
> On 06-Apr 18:10, Alexei Starovoitov wrote:
> > On Mon, Apr 06, 2020 at 11:09:18AM +0200, Jiri Olsa wrote:
> > > 
> > > is there any way we could have d_path functionality (even
> > > reduced and not working for all cases) that could be used
> > > or called like that?
> > 
> > I agree with Al. This helper cannot be enabled for all of bpf tracing.
> > We have to white list its usage for specific callsites only.
> > May be all of lsm hooks are safe. I don't know yet. This has to be
> > analyzed carefully. Every hook. One by one.
> 
> I agree with this, there are some LSM hooks which do get called in
> interrupt context, eg. task_free (which gets called in an RCU
> callback).
> 
> The hooks that we are using it for and we know that it works (using
> our experimental helpers similar to this) are the bprm_* hooks in the
> exec pathway (for logic based on the path of the executable).
> 
> It might be worth whitelisting these functions by adding verifier ops
> for LSM programs?
> 
> Would you want to do it as a part of this series?

I guess we should to do some generic whitelist solution that
would be usable by any prog type.. I'll try to put something
together

jirka