diff mbox series

[bpf-next,v5] bpf: add new helper get_file_path for mapping a file descriptor to a pathname

Message ID 20191101125707.10043-1-ethercflow@gmail.com
State Changes Requested
Delegated to: BPF Maintainers
Headers show
Series [bpf-next,v5] bpf: add new helper get_file_path for mapping a file descriptor to a pathname | expand

Commit Message

Wenbo Zhang Nov. 1, 2019, 12:57 p.m. UTC 
When people want to identify which file system files are being opened,
read, and written to, they can use this helper with file descriptor as
input to achieve this goal. Other pseudo filesystems are also supported.

This requirement is mainly discussed here:

  https://github.com/iovisor/bcc/issues/237

v4->v5: addressed Andrii and Daniel's feedback
- rename bpf_fd2path to bpf_get_file_path to be consistent with other
helper's names
- when fdget_raw fails, set ret to -EBADF instead of -EINVAL
- remove fdput from fdget_raw's error path
- use IS_ERR instead of IS_ERR_OR_NULL as d_path ether returns a pointer
into the buffer or an error code if the path was too long
- modify the normal path's return value to return copied string lengh
including NUL
- update this helper description's Return bits.

v3->v4: addressed Daniel's feedback
- fix missing fdput()
- move fd2path from kernel/bpf/trace.c to kernel/trace/bpf_trace.c
- move fd2path's test code to another patch
- add comment to explain why use fdget_raw instead of fdget

v2->v3: addressed Yonghong's feedback
- remove unnecessary LOCKDOWN_BPF_READ
- refactor error handling section for enhanced readability
- provide a test case in tools/testing/selftests/bpf

v1->v2: addressed Daniel's feedback
- fix backward compatibility
- add this helper description
- fix signed-off name
---
 include/uapi/linux/bpf.h       | 15 ++++++++++-
 kernel/trace/bpf_trace.c       | 49 ++++++++++++++++++++++++++++++++++
 tools/include/uapi/linux/bpf.h | 15 ++++++++++-
 3 files changed, 77 insertions(+), 2 deletions(-)

Comments

Andrii Nakryiko Nov. 2, 2019, 5:36 a.m. UTC | #1 
On Fri, Nov 1, 2019 at 5:57 AM Wenbo Zhang <ethercflow@gmail.com> wrote:
>
> When people want to identify which file system files are being opened,
> read, and written to, they can use this helper with file descriptor as
> input to achieve this goal. Other pseudo filesystems are also supported.
>
> This requirement is mainly discussed here:
>
>   https://github.com/iovisor/bcc/issues/237
>
> v4->v5: addressed Andrii and Daniel's feedback
> - rename bpf_fd2path to bpf_get_file_path to be consistent with other
> helper's names
> - when fdget_raw fails, set ret to -EBADF instead of -EINVAL
> - remove fdput from fdget_raw's error path
> - use IS_ERR instead of IS_ERR_OR_NULL as d_path ether returns a pointer
> into the buffer or an error code if the path was too long
> - modify the normal path's return value to return copied string lengh
> including NUL
> - update this helper description's Return bits.
>
> v3->v4: addressed Daniel's feedback
> - fix missing fdput()
> - move fd2path from kernel/bpf/trace.c to kernel/trace/bpf_trace.c
> - move fd2path's test code to another patch
> - add comment to explain why use fdget_raw instead of fdget
>
> v2->v3: addressed Yonghong's feedback
> - remove unnecessary LOCKDOWN_BPF_READ
> - refactor error handling section for enhanced readability
> - provide a test case in tools/testing/selftests/bpf
>
> v1->v2: addressed Daniel's feedback
> - fix backward compatibility
> - add this helper description
> - fix signed-off name
> ---

See nit below, but I'm fine with the current state as well.

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  /* 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 f50bf19f7a05..fc9f577e65f5 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -683,6 +683,53 @@ static const struct bpf_func_proto bpf_send_signal_proto = {
>         .arg1_type      = ARG_ANYTHING,
>  };
>
> +BPF_CALL_3(bpf_get_file_path, char *, dst, u32, size, int, fd)
> +{
> +       struct fd f;
> +       char *p;
> +       int ret = -EBADF;
> +
> +       /* Use fdget_raw instead of fdget to support O_PATH, and
> +        * fdget_raw doesn't have any sleepable code, so it's ok
> +        * to be here.
> +        */
> +       f = fdget_raw(fd);
> +       if (!f.file)
> +               goto error;
> +
> +       /* d_path doesn't have any sleepable code, so it's ok to
> +        * be here. But it uses the current macro to get fs_struct
> +        * (current->fs). So this helper shouldn't be called in
> +        * interrupt context.
> +        */
> +       p = d_path(&f.file->f_path, dst, size);
> +       if (IS_ERR(p)) {
> +               ret = PTR_ERR(p);
> +               fdput(f);
> +               goto error;
> +       }
> +
> +       ret = strlen(p);
> +       memmove(dst, p, ret);
> +       dst[ret++] = '\0';
> +       fdput(f);
> +       goto end;
> +
> +error:
> +       memset(dst, '0', size);
> +end:
> +       return ret;

nit: I'd avoid unnecessary goto end (and end label itself) by having
two explicit returns:

    return 0;
error:
    memset(...);
    return ret;

> +}
> +
> +static const struct bpf_func_proto bpf_get_file_path_proto = {
> +       .func       = bpf_get_file_path,
> +       .gpl_only   = true,
> +       .ret_type   = RET_INTEGER,
> +       .arg1_type  = ARG_PTR_TO_UNINIT_MEM,
> +       .arg2_type  = ARG_CONST_SIZE,
> +       .arg3_type  = ARG_ANYTHING,
> +};
> +
>  static const struct bpf_func_proto *
>  tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>  {
> @@ -735,6 +782,8 @@ tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>  #endif
>         case BPF_FUNC_send_signal:
>                 return &bpf_send_signal_proto;
> +       case BPF_FUNC_get_file_path:
> +               return &bpf_get_file_path_proto;

This seems like a rather useful helper not just in tracing context. So
if maintainers are ok with that, maybe you can follow up with patch
that adds it in more BPF program types.

>         default:
>                 return NULL;
>         }

[...]
Wenbo Zhang Nov. 3, 2019, 4:08 a.m. UTC | #2 
> nit: I'd avoid unnecessary goto end (and end label itself) by having
> two explicit returns:

>    return 0;
> error:
>    memset(...);

Agree, I'll send a new patch with explicit returns.  BTW Yonghong has
pointed this on v1 too, but I didn't get the whole point at that time,
 just made a part of change. Thank you both.

> This seems like a rather useful helper not just in tracing context. So
> if maintainers are ok with that, maybe you can follow up with patch
> that adds it in more BPF program types.

Glad to hear this, I'll keep following up with it.

Andrii Nakryiko <andrii.nakryiko@gmail.com> 于2019年11月2日周六 下午1:37写道：
>
> On Fri, Nov 1, 2019 at 5:57 AM Wenbo Zhang <ethercflow@gmail.com> wrote:
> >
> > When people want to identify which file system files are being opened,
> > read, and written to, they can use this helper with file descriptor as
> > input to achieve this goal. Other pseudo filesystems are also supported.
> >
> > This requirement is mainly discussed here:
> >
> >   https://github.com/iovisor/bcc/issues/237
> >
> > v4->v5: addressed Andrii and Daniel's feedback
> > - rename bpf_fd2path to bpf_get_file_path to be consistent with other
> > helper's names
> > - when fdget_raw fails, set ret to -EBADF instead of -EINVAL
> > - remove fdput from fdget_raw's error path
> > - use IS_ERR instead of IS_ERR_OR_NULL as d_path ether returns a pointer
> > into the buffer or an error code if the path was too long
> > - modify the normal path's return value to return copied string lengh
> > including NUL
> > - update this helper description's Return bits.
> >
> > v3->v4: addressed Daniel's feedback
> > - fix missing fdput()
> > - move fd2path from kernel/bpf/trace.c to kernel/trace/bpf_trace.c
> > - move fd2path's test code to another patch
> > - add comment to explain why use fdget_raw instead of fdget
> >
> > v2->v3: addressed Yonghong's feedback
> > - remove unnecessary LOCKDOWN_BPF_READ
> > - refactor error handling section for enhanced readability
> > - provide a test case in tools/testing/selftests/bpf
> >
> > v1->v2: addressed Daniel's feedback
> > - fix backward compatibility
> > - add this helper description
> > - fix signed-off name
> > ---
>
> See nit below, but I'm fine with the current state as well.
>
> Acked-by: Andrii Nakryiko <andriin@fb.com>
>
> >  /* 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 f50bf19f7a05..fc9f577e65f5 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -683,6 +683,53 @@ static const struct bpf_func_proto bpf_send_signal_proto = {
> >         .arg1_type      = ARG_ANYTHING,
> >  };
> >
> > +BPF_CALL_3(bpf_get_file_path, char *, dst, u32, size, int, fd)
> > +{
> > +       struct fd f;
> > +       char *p;
> > +       int ret = -EBADF;
> > +
> > +       /* Use fdget_raw instead of fdget to support O_PATH, and
> > +        * fdget_raw doesn't have any sleepable code, so it's ok
> > +        * to be here.
> > +        */
> > +       f = fdget_raw(fd);
> > +       if (!f.file)
> > +               goto error;
> > +
> > +       /* d_path doesn't have any sleepable code, so it's ok to
> > +        * be here. But it uses the current macro to get fs_struct
> > +        * (current->fs). So this helper shouldn't be called in
> > +        * interrupt context.
> > +        */
> > +       p = d_path(&f.file->f_path, dst, size);
> > +       if (IS_ERR(p)) {
> > +               ret = PTR_ERR(p);
> > +               fdput(f);
> > +               goto error;
> > +       }
> > +
> > +       ret = strlen(p);
> > +       memmove(dst, p, ret);
> > +       dst[ret++] = '\0';
> > +       fdput(f);
> > +       goto end;
> > +
> > +error:
> > +       memset(dst, '0', size);
> > +end:
> > +       return ret;
>
> nit: I'd avoid unnecessary goto end (and end label itself) by having
> two explicit returns:
>
>     return 0;
> error:
>     memset(...);
>     return ret;
>
> > +}
> > +
> > +static const struct bpf_func_proto bpf_get_file_path_proto = {
> > +       .func       = bpf_get_file_path,
> > +       .gpl_only   = true,
> > +       .ret_type   = RET_INTEGER,
> > +       .arg1_type  = ARG_PTR_TO_UNINIT_MEM,
> > +       .arg2_type  = ARG_CONST_SIZE,
> > +       .arg3_type  = ARG_ANYTHING,
> > +};
> > +
> >  static const struct bpf_func_proto *
> >  tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> >  {
> > @@ -735,6 +782,8 @@ tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> >  #endif
> >         case BPF_FUNC_send_signal:
> >                 return &bpf_send_signal_proto;
> > +       case BPF_FUNC_get_file_path:
> > +               return &bpf_get_file_path_proto;
>
> This seems like a rather useful helper not just in tracing context. So
> if maintainers are ok with that, maybe you can follow up with patch
> that adds it in more BPF program types.
>
> >         default:
> >                 return NULL;
> >         }
>
> [...]
diff mbox series

Patch

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index a6bf19dabaab..d618a914c6fe 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -2777,6 +2777,18 @@  union bpf_attr {
  * 		restricted to raw_tracepoint bpf programs.
  * 	Return
  * 		0 on success, or a negative error in case of failure.
+ *
+ * int bpf_get_file_path(char *path, u32 size, int fd)
+ *	Description
+ *		Get **file** atrribute from the current task by *fd*, then call
+ *		**d_path** to get it's absolute path and copy it as string into
+ *		*path* of *size*. The **path** also support pseudo filesystems
+ *		(whether or not it can be mounted). The *size* must be strictly
+ *		positive. On success, the helper makes sure that the *path* is
+ *		NUL-terminated. On failure, it is filled with zeroes.
+ *	Return
+ *		On success, returns the length of the copied string INCLUDING
+ *		the trailing NUL, or a negative error in case of failure.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -2890,7 +2902,8 @@  union bpf_attr {
 	FN(sk_storage_delete),		\
 	FN(send_signal),		\
 	FN(tcp_gen_syncookie),		\
-	FN(skb_output),
+	FN(skb_output),			\
+	FN(get_file_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 f50bf19f7a05..fc9f577e65f5 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -683,6 +683,53 @@  static const struct bpf_func_proto bpf_send_signal_proto = {
 	.arg1_type	= ARG_ANYTHING,
 };
 
+BPF_CALL_3(bpf_get_file_path, char *, dst, u32, size, int, fd)
+{
+	struct fd f;
+	char *p;
+	int ret = -EBADF;
+
+	/* Use fdget_raw instead of fdget to support O_PATH, and
+	 * fdget_raw doesn't have any sleepable code, so it's ok
+	 * to be here.
+	 */
+	f = fdget_raw(fd);
+	if (!f.file)
+		goto error;
+
+	/* d_path doesn't have any sleepable code, so it's ok to
+	 * be here. But it uses the current macro to get fs_struct
+	 * (current->fs). So this helper shouldn't be called in
+	 * interrupt context.
+	 */
+	p = d_path(&f.file->f_path, dst, size);
+	if (IS_ERR(p)) {
+		ret = PTR_ERR(p);
+		fdput(f);
+		goto error;
+	}
+
+	ret = strlen(p);
+	memmove(dst, p, ret);
+	dst[ret++] = '\0';
+	fdput(f);
+	goto end;
+
+error:
+	memset(dst, '0', size);
+end:
+	return ret;
+}
+
+static const struct bpf_func_proto bpf_get_file_path_proto = {
+	.func       = bpf_get_file_path,
+	.gpl_only   = true,
+	.ret_type   = RET_INTEGER,
+	.arg1_type  = ARG_PTR_TO_UNINIT_MEM,
+	.arg2_type  = ARG_CONST_SIZE,
+	.arg3_type  = ARG_ANYTHING,
+};
+
 static const struct bpf_func_proto *
 tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
@@ -735,6 +782,8 @@  tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 #endif
 	case BPF_FUNC_send_signal:
 		return &bpf_send_signal_proto;
+	case BPF_FUNC_get_file_path:
+		return &bpf_get_file_path_proto;
 	default:
 		return NULL;
 	}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index a6bf19dabaab..d618a914c6fe 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -2777,6 +2777,18 @@  union bpf_attr {
  * 		restricted to raw_tracepoint bpf programs.
  * 	Return
  * 		0 on success, or a negative error in case of failure.
+ *
+ * int bpf_get_file_path(char *path, u32 size, int fd)
+ *	Description
+ *		Get **file** atrribute from the current task by *fd*, then call
+ *		**d_path** to get it's absolute path and copy it as string into
+ *		*path* of *size*. The **path** also support pseudo filesystems
+ *		(whether or not it can be mounted). The *size* must be strictly
+ *		positive. On success, the helper makes sure that the *path* is
+ *		NUL-terminated. On failure, it is filled with zeroes.
+ *	Return
+ *		On success, returns the length of the copied string INCLUDING
+ *		the trailing NUL, or a negative error in case of failure.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -2890,7 +2902,8 @@  union bpf_attr {
 	FN(sk_storage_delete),		\
 	FN(send_signal),		\
 	FN(tcp_gen_syncookie),		\
-	FN(skb_output),
+	FN(skb_output),			\
+	FN(get_file_path),
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call