* Re: [PATCH bpf-next v12 1/2] bpf: add new helper get_file_path for mapping a file descriptor to a pathname
2019-12-15 4:01 ` [PATCH bpf-next v12 1/2] bpf: add new helper get_file_path for mapping a file descriptor to a pathname Wenbo Zhang
@ 2019-12-15 16:05 ` Yonghong Song
2019-12-17 6:26 ` Wenbo Zhang
2019-12-15 16:10 ` Yonghong Song
` (2 subsequent siblings)
3 siblings, 1 reply; 52+ messages in thread
From: Yonghong Song @ 2019-12-15 16:05 UTC (permalink / raw)
To: Wenbo Zhang, bpf; +Cc: ast, daniel, andrii.nakryiko, netdev
On 12/14/19 8:01 PM, Wenbo Zhang 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
>
> v11->v12: addressed Alexei's feedback
> - only allow tracepoints to make sure it won't dead lock
>
> v10->v11: addressed Al and Alexei's feedback
> - fix missing fput()
>
> v9->v10: addressed Andrii's feedback
> - send this patch together with the patch selftests as one patch series
>
> v8->v9:
> - format helper description
>
> v7->v8: addressed Alexei's feedback
> - use fget_raw instead of fdget_raw, as fdget_raw is only used inside fs/
> - ensure we're in user context which is safe fot the help to run
> - filter unmountable pseudo filesystem, because they don't have real path
> - supplement the description of this helper function
>
> v6->v7:
> - fix missing signed-off-by line
>
> v5->v6: addressed Andrii's feedback
> - avoid unnecessary goto end by having two explicit returns
>
> 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 length
> 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
>
> Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
> ---
> include/uapi/linux/bpf.h | 29 +++++++++++++-
> kernel/trace/bpf_trace.c | 70 ++++++++++++++++++++++++++++++++++
> tools/include/uapi/linux/bpf.h | 29 +++++++++++++-
> 3 files changed, 126 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index dbbcf0b02970..71d9705df120 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -2821,6 +2821,32 @@ union bpf_attr {
> * Return
> * On success, the strictly positive length of the string, including
> * the trailing NUL character. On error, a negative value.
> + *
> + * 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*. Notice the **path** don't support unmountable
> + * pseudo filesystems as they don't have path (eg: SOCKFS, PIPEFS).
> + * The *size* must be strictly positive. On success, the helper
> + * makes sure that the *path* is NUL-terminated, and the buffer
> + * could be:
> + * - a regular full path (include mountable fs eg: /proc, /sys)
> + * - a regular full path with "(deleted)" at the end.
Let us say with " (deleted)" is appended to be consistent with comments
in d_path() and is more clear to user what the format will looks like.
> + * On failure, it is filled with zeroes.
> + * Return
> + * On success, returns the length of the copied string INCLUDING
> + * the trailing NUL.
trailing '\0'.
> + *
> + * On failure, the returned value is one of the following:
> + *
> + * **-EPERM** if no permission to get the path (eg: in irq ctx).
> + *
> + * **-EBADF** if *fd* is invalid.
> + *
> + * **-EINVAL** if *fd* corresponds to a unmountable pseudo fs
> + *
> + * **-ENAMETOOLONG** if full path is longer than *size*
> */
> #define __BPF_FUNC_MAPPER(FN) \
> FN(unspec), \
> @@ -2938,7 +2964,8 @@ union bpf_attr {
> FN(probe_read_user), \
> FN(probe_read_kernel), \
> FN(probe_read_user_str), \
> - FN(probe_read_kernel_str),
> + FN(probe_read_kernel_str), \
> + 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 e5ef4ae9edb5..db9c0ec46a5d 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -762,6 +762,72 @@ 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 file *f;
> + char *p;
> + int ret = -EBADF;
> +
> + /* Ensure we're in user context which is safe for the helper to
> + * run. This helper has no business in a kthread.
> + */
> + if (unlikely(in_interrupt() ||
> + current->flags & (PF_KTHREAD | PF_EXITING))) {
> + ret = -EPERM;
> + goto error;
> + }
> +
> + /* Use fget_raw instead of fget to support O_PATH, and it doesn't
> + * have any sleepable code, so it's ok to be here.
> + */
> + f = fget_raw(fd);
> + if (!f)
> + goto error;
> +
> + /* For unmountable pseudo filesystem, it seems to have no meaning
> + * to get their fake paths as they don't have path, and to be no
> + * way to validate this function pointer can be always safe to call
> + * in the current context.
> + */
> + if (f->f_path.dentry->d_op && f->f_path.dentry->d_op->d_dname) {
> + ret = -EINVAL;
> + fput(f);
> + goto error;
> + }
> +
> + /* After filter unmountable pseudo filesytem, d_path won't call
> + * dentry->d_op->d_name(), the normally path doesn't have any
> + * sleepable code, and despite it uses the current macro to get
> + * fs_struct (current->fs), we've already ensured we're in user
> + * context, so it's ok to be here.
> + */
> + p = d_path(&f->f_path, dst, size);
> + if (IS_ERR(p)) {
> + ret = PTR_ERR(p);
> + fput(f);
> + goto error;
> + }
> +
> + ret = strlen(p);
> + memmove(dst, p, ret);
> + dst[ret++] = '\0';
nit: you could do memmove(dst, p, ret + 1)?
> + fput(f);
> + return ret;
The description says the return value length including trailing '\0'.
The above 'ret' does not include trailing '\0'.
> +
> +error:
> + memset(dst, '0', size);
> + 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)
> {
> @@ -953,6 +1019,8 @@ tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> return &bpf_get_stackid_proto_tp;
> case BPF_FUNC_get_stack:
> return &bpf_get_stack_proto_tp;
> + case BPF_FUNC_get_file_path:
> + return &bpf_get_file_path_proto;
> default:
> return tracing_func_proto(func_id, prog);
> }
> @@ -1146,6 +1214,8 @@ raw_tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> return &bpf_get_stackid_proto_raw_tp;
> case BPF_FUNC_get_stack:
> return &bpf_get_stack_proto_raw_tp;
> + case BPF_FUNC_get_file_path:
> + return &bpf_get_file_path_proto;
> default:
> return tracing_func_proto(func_id, prog);
> }
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index dbbcf0b02970..71d9705df120 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -2821,6 +2821,32 @@ union bpf_attr {
> * Return
> * On success, the strictly positive length of the string, including
> * the trailing NUL character. On error, a negative value.
> + *
> + * 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*. Notice the **path** don't support unmountable
> + * pseudo filesystems as they don't have path (eg: SOCKFS, PIPEFS).
> + * The *size* must be strictly positive. On success, the helper
> + * makes sure that the *path* is NUL-terminated, and the buffer
> + * could be:
> + * - a regular full path (include mountable fs eg: /proc, /sys)
> + * - a regular full path with "(deleted)" at the end.
ditto
> + * On failure, it is filled with zeroes.
> + * Return
> + * On success, returns the length of the copied string INCLUDING
> + * the trailing NUL.
ditto
> + *
> + * On failure, the returned value is one of the following:
> + *
> + * **-EPERM** if no permission to get the path (eg: in irq ctx).
> + *
> + * **-EBADF** if *fd* is invalid.
> + *
> + * **-EINVAL** if *fd* corresponds to a unmountable pseudo fs
> + *
> + * **-ENAMETOOLONG** if full path is longer than *size*
> */
> #define __BPF_FUNC_MAPPER(FN) \
> FN(unspec), \
> @@ -2938,7 +2964,8 @@ union bpf_attr {
> FN(probe_read_user), \
> FN(probe_read_kernel), \
> FN(probe_read_user_str), \
> - FN(probe_read_kernel_str),
> + FN(probe_read_kernel_str), \
> + FN(get_file_path),
>
> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> * function eBPF program intends to call
>
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH bpf-next v12 1/2] bpf: add new helper get_file_path for mapping a file descriptor to a pathname
2019-12-15 16:05 ` Yonghong Song
@ 2019-12-17 6:26 ` Wenbo Zhang
2019-12-17 6:33 ` Yonghong Song
0 siblings, 1 reply; 52+ messages in thread
From: Wenbo Zhang @ 2019-12-17 6:26 UTC (permalink / raw)
To: Yonghong Song; +Cc: bpf, ast, daniel, andrii.nakryiko, netdev
> > + * - a regular full path (include mountable fs eg: /proc, /sys)
> > + * - a regular full path with "(deleted)" at the end.
> Let us say with " (deleted)" is appended to be consistent with comments
> in d_path() and is more clear to user what the format will looks like.
Thank you, I'll fix this.
> > + ret = strlen(p);
> > + memmove(dst, p, ret);
> > + dst[ret++] = '\0';
> nit: you could do memmove(dst, p, ret + 1)?
I did with `dst[ret++]='\0';` to return value length including
trailing '\0'. as you mentioned below:
> > + fput(f);
> > + return ret;
> The description says the return value length including trailing '\0'.
> The above 'ret' does not include trailing '\0'.
It seems `[ret++]` not very clear to read and '\0' can be done by
`memmove`. I think I'll refactor to
```
ret = strlen(p) + 1;
memmove(dst, p, ret);
fput(f);
return ret;
```
Is this better?
Yonghong Song <yhs@fb.com> 于2019年12月16日周一 上午12:06写道：
>
>
>
> On 12/14/19 8:01 PM, Wenbo Zhang 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
> >
> > v11->v12: addressed Alexei's feedback
> > - only allow tracepoints to make sure it won't dead lock
> >
> > v10->v11: addressed Al and Alexei's feedback
> > - fix missing fput()
> >
> > v9->v10: addressed Andrii's feedback
> > - send this patch together with the patch selftests as one patch series
> >
> > v8->v9:
> > - format helper description
> >
> > v7->v8: addressed Alexei's feedback
> > - use fget_raw instead of fdget_raw, as fdget_raw is only used inside fs/
> > - ensure we're in user context which is safe fot the help to run
> > - filter unmountable pseudo filesystem, because they don't have real path
> > - supplement the description of this helper function
> >
> > v6->v7:
> > - fix missing signed-off-by line
> >
> > v5->v6: addressed Andrii's feedback
> > - avoid unnecessary goto end by having two explicit returns
> >
> > 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 length
> > 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
> >
> > Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
> > ---
> > include/uapi/linux/bpf.h | 29 +++++++++++++-
> > kernel/trace/bpf_trace.c | 70 ++++++++++++++++++++++++++++++++++
> > tools/include/uapi/linux/bpf.h | 29 +++++++++++++-
> > 3 files changed, 126 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index dbbcf0b02970..71d9705df120 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -2821,6 +2821,32 @@ union bpf_attr {
> > * Return
> > * On success, the strictly positive length of the string, including
> > * the trailing NUL character. On error, a negative value.
> > + *
> > + * 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*. Notice the **path** don't support unmountable
> > + * pseudo filesystems as they don't have path (eg: SOCKFS, PIPEFS).
> > + * The *size* must be strictly positive. On success, the helper
> > + * makes sure that the *path* is NUL-terminated, and the buffer
> > + * could be:
> > + * - a regular full path (include mountable fs eg: /proc, /sys)
> > + * - a regular full path with "(deleted)" at the end.
>
> Let us say with " (deleted)" is appended to be consistent with comments
> in d_path() and is more clear to user what the format will looks like.
>
> > + * On failure, it is filled with zeroes.
> > + * Return
> > + * On success, returns the length of the copied string INCLUDING
> > + * the trailing NUL.
>
> trailing '\0'.
>
> > + *
> > + * On failure, the returned value is one of the following:
> > + *
> > + * **-EPERM** if no permission to get the path (eg: in irq ctx).
> > + *
> > + * **-EBADF** if *fd* is invalid.
> > + *
> > + * **-EINVAL** if *fd* corresponds to a unmountable pseudo fs
> > + *
> > + * **-ENAMETOOLONG** if full path is longer than *size*
> > */
> > #define __BPF_FUNC_MAPPER(FN) \
> > FN(unspec), \
> > @@ -2938,7 +2964,8 @@ union bpf_attr {
> > FN(probe_read_user), \
> > FN(probe_read_kernel), \
> > FN(probe_read_user_str), \
> > - FN(probe_read_kernel_str),
> > + FN(probe_read_kernel_str), \
> > + 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 e5ef4ae9edb5..db9c0ec46a5d 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -762,6 +762,72 @@ 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 file *f;
> > + char *p;
> > + int ret = -EBADF;
> > +
> > + /* Ensure we're in user context which is safe for the helper to
> > + * run. This helper has no business in a kthread.
> > + */
> > + if (unlikely(in_interrupt() ||
> > + current->flags & (PF_KTHREAD | PF_EXITING))) {
> > + ret = -EPERM;
> > + goto error;
> > + }
> > +
> > + /* Use fget_raw instead of fget to support O_PATH, and it doesn't
> > + * have any sleepable code, so it's ok to be here.
> > + */
> > + f = fget_raw(fd);
> > + if (!f)
> > + goto error;
> > +
> > + /* For unmountable pseudo filesystem, it seems to have no meaning
> > + * to get their fake paths as they don't have path, and to be no
> > + * way to validate this function pointer can be always safe to call
> > + * in the current context.
> > + */
> > + if (f->f_path.dentry->d_op && f->f_path.dentry->d_op->d_dname) {
> > + ret = -EINVAL;
> > + fput(f);
> > + goto error;
> > + }
> > +
> > + /* After filter unmountable pseudo filesytem, d_path won't call
> > + * dentry->d_op->d_name(), the normally path doesn't have any
> > + * sleepable code, and despite it uses the current macro to get
> > + * fs_struct (current->fs), we've already ensured we're in user
> > + * context, so it's ok to be here.
> > + */
> > + p = d_path(&f->f_path, dst, size);
> > + if (IS_ERR(p)) {
> > + ret = PTR_ERR(p);
> > + fput(f);
> > + goto error;
> > + }
> > +
> > + ret = strlen(p);
> > + memmove(dst, p, ret);
> > + dst[ret++] = '\0';
>
> nit: you could do memmove(dst, p, ret + 1)?
>
> > + fput(f);
> > + return ret;
>
> The description says the return value length including trailing '\0'.
> The above 'ret' does not include trailing '\0'.
>
> > +
> > +error:
> > + memset(dst, '0', size);
> > + 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)
> > {
> > @@ -953,6 +1019,8 @@ tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> > return &bpf_get_stackid_proto_tp;
> > case BPF_FUNC_get_stack:
> > return &bpf_get_stack_proto_tp;
> > + case BPF_FUNC_get_file_path:
> > + return &bpf_get_file_path_proto;
> > default:
> > return tracing_func_proto(func_id, prog);
> > }
> > @@ -1146,6 +1214,8 @@ raw_tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> > return &bpf_get_stackid_proto_raw_tp;
> > case BPF_FUNC_get_stack:
> > return &bpf_get_stack_proto_raw_tp;
> > + case BPF_FUNC_get_file_path:
> > + return &bpf_get_file_path_proto;
> > default:
> > return tracing_func_proto(func_id, prog);
> > }
> > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > index dbbcf0b02970..71d9705df120 100644
> > --- a/tools/include/uapi/linux/bpf.h
> > +++ b/tools/include/uapi/linux/bpf.h
> > @@ -2821,6 +2821,32 @@ union bpf_attr {
> > * Return
> > * On success, the strictly positive length of the string, including
> > * the trailing NUL character. On error, a negative value.
> > + *
> > + * 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*. Notice the **path** don't support unmountable
> > + * pseudo filesystems as they don't have path (eg: SOCKFS, PIPEFS).
> > + * The *size* must be strictly positive. On success, the helper
> > + * makes sure that the *path* is NUL-terminated, and the buffer
> > + * could be:
> > + * - a regular full path (include mountable fs eg: /proc, /sys)
> > + * - a regular full path with "(deleted)" at the end.
>
> ditto
>
> > + * On failure, it is filled with zeroes.
> > + * Return
> > + * On success, returns the length of the copied string INCLUDING
> > + * the trailing NUL.
>
> ditto
>
> > + *
> > + * On failure, the returned value is one of the following:
> > + *
> > + * **-EPERM** if no permission to get the path (eg: in irq ctx).
> > + *
> > + * **-EBADF** if *fd* is invalid.
> > + *
> > + * **-EINVAL** if *fd* corresponds to a unmountable pseudo fs
> > + *
> > + * **-ENAMETOOLONG** if full path is longer than *size*
> > */
> > #define __BPF_FUNC_MAPPER(FN) \
> > FN(unspec), \
> > @@ -2938,7 +2964,8 @@ union bpf_attr {
> > FN(probe_read_user), \
> > FN(probe_read_kernel), \
> > FN(probe_read_user_str), \
> > - FN(probe_read_kernel_str),
> > + FN(probe_read_kernel_str), \
> > + FN(get_file_path),
> >
> > /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > * function eBPF program intends to call
> >
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH bpf-next v12 1/2] bpf: add new helper get_file_path for mapping a file descriptor to a pathname
2019-12-17 6:26 ` Wenbo Zhang
@ 2019-12-17 6:33 ` Yonghong Song
0 siblings, 0 replies; 52+ messages in thread
From: Yonghong Song @ 2019-12-17 6:33 UTC (permalink / raw)
To: Wenbo Zhang; +Cc: bpf, ast, daniel, andrii.nakryiko, netdev
On 12/16/19 10:26 PM, Wenbo Zhang wrote:
>>> + * - a regular full path (include mountable fs eg: /proc, /sys)
>>> + * - a regular full path with "(deleted)" at the end.
>
>> Let us say with " (deleted)" is appended to be consistent with comments
>> in d_path() and is more clear to user what the format will looks like.
>
> Thank you, I'll fix this.
>
>>> + ret = strlen(p);
>>> + memmove(dst, p, ret);
>>> + dst[ret++] = '\0';
>
>> nit: you could do memmove(dst, p, ret + 1)?
>
> I did with `dst[ret++]='\0';` to return value length including
> trailing '\0'. as you mentioned below:
>
>>> + fput(f);
>>> + return ret;
>
>> The description says the return value length including trailing '\0'.
>> The above 'ret' does not include trailing '\0'.
>
> It seems `[ret++]` not very clear to read and '\0' can be done by
> `memmove`. I think I'll refactor to
>
> ```
> ret = strlen(p) + 1;
> memmove(dst, p, ret);
> fput(f);
> return ret;
> ```
>
> Is this better?
Ah, I missed ret++ in dst[ret++]. Indeed the above code is better.
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH bpf-next v12 1/2] bpf: add new helper get_file_path for mapping a file descriptor to a pathname
2019-12-15 4:01 ` [PATCH bpf-next v12 1/2] bpf: add new helper get_file_path for mapping a file descriptor to a pathname Wenbo Zhang
2019-12-15 16:05 ` Yonghong Song
@ 2019-12-15 16:10 ` Yonghong Song
2019-12-17 6:27 ` Wenbo Zhang
2019-12-16 22:09 ` Brendan Gregg
2019-12-17 9:47 ` [PATCH bpf-next v13 0/2] bpf: adding get_fd_path helper Wenbo Zhang
3 siblings, 1 reply; 52+ messages in thread
From: Yonghong Song @ 2019-12-15 16:10 UTC (permalink / raw)
To: Wenbo Zhang, bpf; +Cc: ast, daniel, andrii.nakryiko, netdev
On 12/14/19 8:01 PM, Wenbo Zhang 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
>
> v11->v12: addressed Alexei's feedback
> - only allow tracepoints to make sure it won't dead lock
>
> v10->v11: addressed Al and Alexei's feedback
> - fix missing fput()
>
> v9->v10: addressed Andrii's feedback
> - send this patch together with the patch selftests as one patch series
>
> v8->v9:
> - format helper description
>
> v7->v8: addressed Alexei's feedback
> - use fget_raw instead of fdget_raw, as fdget_raw is only used inside fs/
> - ensure we're in user context which is safe fot the help to run
> - filter unmountable pseudo filesystem, because they don't have real path
> - supplement the description of this helper function
>
> v6->v7:
> - fix missing signed-off-by line
>
> v5->v6: addressed Andrii's feedback
> - avoid unnecessary goto end by having two explicit returns
>
> 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 length
> 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
>
> Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
> ---
> include/uapi/linux/bpf.h | 29 +++++++++++++-
> kernel/trace/bpf_trace.c | 70 ++++++++++++++++++++++++++++++++++
> tools/include/uapi/linux/bpf.h | 29 +++++++++++++-
> 3 files changed, 126 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index dbbcf0b02970..71d9705df120 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -2821,6 +2821,32 @@ union bpf_attr {
> * Return
> * On success, the strictly positive length of the string, including
> * the trailing NUL character. On error, a negative value.
> + *
> + * 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*. Notice the **path** don't support unmountable
> + * pseudo filesystems as they don't have path (eg: SOCKFS, PIPEFS).
> + * The *size* must be strictly positive. On success, the helper
> + * makes sure that the *path* is NUL-terminated, and the buffer
> + * could be:
> + * - a regular full path (include mountable fs eg: /proc, /sys)
> + * - a regular full path with "(deleted)" at the end.
> + * On failure, it is filled with zeroes.
> + * Return
> + * On success, returns the length of the copied string INCLUDING
> + * the trailing NUL.
> + *
> + * On failure, the returned value is one of the following:
> + *
> + * **-EPERM** if no permission to get the path (eg: in irq ctx).
> + *
> + * **-EBADF** if *fd* is invalid.
> + *
> + * **-EINVAL** if *fd* corresponds to a unmountable pseudo fs
> + *
> + * **-ENAMETOOLONG** if full path is longer than *size*
> */
> #define __BPF_FUNC_MAPPER(FN) \
> FN(unspec), \
> @@ -2938,7 +2964,8 @@ union bpf_attr {
> FN(probe_read_user), \
> FN(probe_read_kernel), \
> FN(probe_read_user_str), \
> - FN(probe_read_kernel_str),
> + FN(probe_read_kernel_str), \
> + 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 e5ef4ae9edb5..db9c0ec46a5d 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -762,6 +762,72 @@ 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 file *f;
> + char *p;
> + int ret = -EBADF;
please try to use reverse Christmas tree for declarations.
> +
> + /* Ensure we're in user context which is safe for the helper to
> + * run. This helper has no business in a kthread.
> + */
> + if (unlikely(in_interrupt() ||
> + current->flags & (PF_KTHREAD | PF_EXITING))) {
> + ret = -EPERM;
> + goto error;
> + }
> +
> + /* Use fget_raw instead of fget to support O_PATH, and it doesn't
> + * have any sleepable code, so it's ok to be here.
> + */
> + f = fget_raw(fd);
> + if (!f)
> + goto error;
> +
> + /* For unmountable pseudo filesystem, it seems to have no meaning
> + * to get their fake paths as they don't have path, and to be no
> + * way to validate this function pointer can be always safe to call
> + * in the current context.
> + */
> + if (f->f_path.dentry->d_op && f->f_path.dentry->d_op->d_dname) {
> + ret = -EINVAL;
> + fput(f);
> + goto error;
> + }
> +
> + /* After filter unmountable pseudo filesytem, d_path won't call
> + * dentry->d_op->d_name(), the normally path doesn't have any
> + * sleepable code, and despite it uses the current macro to get
> + * fs_struct (current->fs), we've already ensured we're in user
> + * context, so it's ok to be here.
> + */
> + p = d_path(&f->f_path, dst, size);
> + if (IS_ERR(p)) {
> + ret = PTR_ERR(p);
> + fput(f);
> + goto error;
> + }
> +
> + ret = strlen(p);
> + memmove(dst, p, ret);
> + dst[ret++] = '\0';
> + fput(f);
> + return ret;
> +
> +error:
> + memset(dst, '0', size);
> + return ret;
> +}
> +
[...]
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH bpf-next v12 1/2] bpf: add new helper get_file_path for mapping a file descriptor to a pathname
2019-12-15 16:10 ` Yonghong Song
@ 2019-12-17 6:27 ` Wenbo Zhang
0 siblings, 0 replies; 52+ messages in thread
From: Wenbo Zhang @ 2019-12-17 6:27 UTC (permalink / raw)
To: Yonghong Song; +Cc: bpf, ast, daniel, andrii.nakryiko, netdev
> > +BPF_CALL_3(bpf_get_file_path, char *, dst, u32, size, int, fd)
> > +{
> > + struct file *f;
> > + char *p;
> > + int ret = -EBADF;
> please try to use reverse Christmas tree for declarations.
Thank you, I'll fix this.
Yonghong Song <yhs@fb.com> 于2019年12月16日周一 上午12:10写道：
>
>
>
> On 12/14/19 8:01 PM, Wenbo Zhang 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
> >
> > v11->v12: addressed Alexei's feedback
> > - only allow tracepoints to make sure it won't dead lock
> >
> > v10->v11: addressed Al and Alexei's feedback
> > - fix missing fput()
> >
> > v9->v10: addressed Andrii's feedback
> > - send this patch together with the patch selftests as one patch series
> >
> > v8->v9:
> > - format helper description
> >
> > v7->v8: addressed Alexei's feedback
> > - use fget_raw instead of fdget_raw, as fdget_raw is only used inside fs/
> > - ensure we're in user context which is safe fot the help to run
> > - filter unmountable pseudo filesystem, because they don't have real path
> > - supplement the description of this helper function
> >
> > v6->v7:
> > - fix missing signed-off-by line
> >
> > v5->v6: addressed Andrii's feedback
> > - avoid unnecessary goto end by having two explicit returns
> >
> > 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 length
> > 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
> >
> > Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
> > ---
> > include/uapi/linux/bpf.h | 29 +++++++++++++-
> > kernel/trace/bpf_trace.c | 70 ++++++++++++++++++++++++++++++++++
> > tools/include/uapi/linux/bpf.h | 29 +++++++++++++-
> > 3 files changed, 126 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index dbbcf0b02970..71d9705df120 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -2821,6 +2821,32 @@ union bpf_attr {
> > * Return
> > * On success, the strictly positive length of the string, including
> > * the trailing NUL character. On error, a negative value.
> > + *
> > + * 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*. Notice the **path** don't support unmountable
> > + * pseudo filesystems as they don't have path (eg: SOCKFS, PIPEFS).
> > + * The *size* must be strictly positive. On success, the helper
> > + * makes sure that the *path* is NUL-terminated, and the buffer
> > + * could be:
> > + * - a regular full path (include mountable fs eg: /proc, /sys)
> > + * - a regular full path with "(deleted)" at the end.
> > + * On failure, it is filled with zeroes.
> > + * Return
> > + * On success, returns the length of the copied string INCLUDING
> > + * the trailing NUL.
> > + *
> > + * On failure, the returned value is one of the following:
> > + *
> > + * **-EPERM** if no permission to get the path (eg: in irq ctx).
> > + *
> > + * **-EBADF** if *fd* is invalid.
> > + *
> > + * **-EINVAL** if *fd* corresponds to a unmountable pseudo fs
> > + *
> > + * **-ENAMETOOLONG** if full path is longer than *size*
> > */
> > #define __BPF_FUNC_MAPPER(FN) \
> > FN(unspec), \
> > @@ -2938,7 +2964,8 @@ union bpf_attr {
> > FN(probe_read_user), \
> > FN(probe_read_kernel), \
> > FN(probe_read_user_str), \
> > - FN(probe_read_kernel_str),
> > + FN(probe_read_kernel_str), \
> > + 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 e5ef4ae9edb5..db9c0ec46a5d 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -762,6 +762,72 @@ 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 file *f;
> > + char *p;
> > + int ret = -EBADF;
>
> please try to use reverse Christmas tree for declarations.
>
> > +
> > + /* Ensure we're in user context which is safe for the helper to
> > + * run. This helper has no business in a kthread.
> > + */
> > + if (unlikely(in_interrupt() ||
> > + current->flags & (PF_KTHREAD | PF_EXITING))) {
> > + ret = -EPERM;
> > + goto error;
> > + }
> > +
> > + /* Use fget_raw instead of fget to support O_PATH, and it doesn't
> > + * have any sleepable code, so it's ok to be here.
> > + */
> > + f = fget_raw(fd);
> > + if (!f)
> > + goto error;
> > +
> > + /* For unmountable pseudo filesystem, it seems to have no meaning
> > + * to get their fake paths as they don't have path, and to be no
> > + * way to validate this function pointer can be always safe to call
> > + * in the current context.
> > + */
> > + if (f->f_path.dentry->d_op && f->f_path.dentry->d_op->d_dname) {
> > + ret = -EINVAL;
> > + fput(f);
> > + goto error;
> > + }
> > +
> > + /* After filter unmountable pseudo filesytem, d_path won't call
> > + * dentry->d_op->d_name(), the normally path doesn't have any
> > + * sleepable code, and despite it uses the current macro to get
> > + * fs_struct (current->fs), we've already ensured we're in user
> > + * context, so it's ok to be here.
> > + */
> > + p = d_path(&f->f_path, dst, size);
> > + if (IS_ERR(p)) {
> > + ret = PTR_ERR(p);
> > + fput(f);
> > + goto error;
> > + }
> > +
> > + ret = strlen(p);
> > + memmove(dst, p, ret);
> > + dst[ret++] = '\0';
> > + fput(f);
> > + return ret;
> > +
> > +error:
> > + memset(dst, '0', size);
> > + return ret;
> > +}
> > +
> [...]
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH bpf-next v12 1/2] bpf: add new helper get_file_path for mapping a file descriptor to a pathname
2019-12-15 4:01 ` [PATCH bpf-next v12 1/2] bpf: add new helper get_file_path for mapping a file descriptor to a pathname Wenbo Zhang
2019-12-15 16:05 ` Yonghong Song
2019-12-15 16:10 ` Yonghong Song
@ 2019-12-16 22:09 ` Brendan Gregg
2019-12-17 4:05 ` Wenbo Zhang
2019-12-17 9:47 ` [PATCH bpf-next v13 0/2] bpf: adding get_fd_path helper Wenbo Zhang
3 siblings, 1 reply; 52+ messages in thread
From: Brendan Gregg @ 2019-12-16 22:09 UTC (permalink / raw)
To: Wenbo Zhang
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Yonghong Song,
andrii.nakryiko, netdev
On Sat, Dec 14, 2019 at 8:01 PM 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
>
> v11->v12: addressed Alexei's feedback
> - only allow tracepoints to make sure it won't dead lock
>
> v10->v11: addressed Al and Alexei's feedback
> - fix missing fput()
>
> v9->v10: addressed Andrii's feedback
> - send this patch together with the patch selftests as one patch series
>
> v8->v9:
> - format helper description
>
> v7->v8: addressed Alexei's feedback
> - use fget_raw instead of fdget_raw, as fdget_raw is only used inside fs/
> - ensure we're in user context which is safe fot the help to run
> - filter unmountable pseudo filesystem, because they don't have real path
> - supplement the description of this helper function
>
> v6->v7:
> - fix missing signed-off-by line
>
> v5->v6: addressed Andrii's feedback
> - avoid unnecessary goto end by having two explicit returns
>
> 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 length
> 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
>
> Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
> ---
> include/uapi/linux/bpf.h | 29 +++++++++++++-
> kernel/trace/bpf_trace.c | 70 ++++++++++++++++++++++++++++++++++
> tools/include/uapi/linux/bpf.h | 29 +++++++++++++-
> 3 files changed, 126 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index dbbcf0b02970..71d9705df120 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -2821,6 +2821,32 @@ union bpf_attr {
> * Return
> * On success, the strictly positive length of the string, including
> * the trailing NUL character. On error, a negative value.
> + *
> + * 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*. Notice the **path** don't support unmountable
> + * pseudo filesystems as they don't have path (eg: SOCKFS, PIPEFS).
> + * The *size* must be strictly positive. On success, the helper
> + * makes sure that the *path* is NUL-terminated, and the buffer
> + * could be:
> + * - a regular full path (include mountable fs eg: /proc, /sys)
> + * - a regular full path with "(deleted)" at the end.
> + * On failure, it is filled with zeroes.
> + * Return
> + * On success, returns the length of the copied string INCLUDING
> + * the trailing NUL.
> + *
> + * On failure, the returned value is one of the following:
> + *
> + * **-EPERM** if no permission to get the path (eg: in irq ctx).
> + *
> + * **-EBADF** if *fd* is invalid.
> + *
> + * **-EINVAL** if *fd* corresponds to a unmountable pseudo fs
> + *
> + * **-ENAMETOOLONG** if full path is longer than *size*
> */
> #define __BPF_FUNC_MAPPER(FN) \
> FN(unspec), \
> @@ -2938,7 +2964,8 @@ union bpf_attr {
> FN(probe_read_user), \
> FN(probe_read_kernel), \
> FN(probe_read_user_str), \
> - FN(probe_read_kernel_str),
> + FN(probe_read_kernel_str), \
> + 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 e5ef4ae9edb5..db9c0ec46a5d 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -762,6 +762,72 @@ 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 file *f;
> + char *p;
> + int ret = -EBADF;
> +
> + /* Ensure we're in user context which is safe for the helper to
> + * run. This helper has no business in a kthread.
> + */
> + if (unlikely(in_interrupt() ||
> + current->flags & (PF_KTHREAD | PF_EXITING))) {
> + ret = -EPERM;
> + goto error;
> + }
> +
> + /* Use fget_raw instead of fget to support O_PATH, and it doesn't
> + * have any sleepable code, so it's ok to be here.
> + */
> + f = fget_raw(fd);
> + if (!f)
> + goto error;
> +
> + /* For unmountable pseudo filesystem, it seems to have no meaning
> + * to get their fake paths as they don't have path, and to be no
> + * way to validate this function pointer can be always safe to call
> + * in the current context.
> + */
> + if (f->f_path.dentry->d_op && f->f_path.dentry->d_op->d_dname) {
> + ret = -EINVAL;
> + fput(f);
> + goto error;
> + }
> +
> + /* After filter unmountable pseudo filesytem, d_path won't call
> + * dentry->d_op->d_name(), the normally path doesn't have any
> + * sleepable code, and despite it uses the current macro to get
> + * fs_struct (current->fs), we've already ensured we're in user
> + * context, so it's ok to be here.
> + */
> + p = d_path(&f->f_path, dst, size);
> + if (IS_ERR(p)) {
> + ret = PTR_ERR(p);
> + fput(f);
> + goto error;
> + }
> +
> + ret = strlen(p);
> + memmove(dst, p, ret);
> + dst[ret++] = '\0';
> + fput(f);
> + return ret;
> +
> +error:
> + memset(dst, '0', size);
> + 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)
> {
> @@ -953,6 +1019,8 @@ tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> return &bpf_get_stackid_proto_tp;
> case BPF_FUNC_get_stack:
> return &bpf_get_stack_proto_tp;
> + case BPF_FUNC_get_file_path:
> + return &bpf_get_file_path_proto;
> default:
> return tracing_func_proto(func_id, prog);
> }
> @@ -1146,6 +1214,8 @@ raw_tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> return &bpf_get_stackid_proto_raw_tp;
> case BPF_FUNC_get_stack:
> return &bpf_get_stack_proto_raw_tp;
> + case BPF_FUNC_get_file_path:
> + return &bpf_get_file_path_proto;
> default:
> return tracing_func_proto(func_id, prog);
> }
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index dbbcf0b02970..71d9705df120 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -2821,6 +2821,32 @@ union bpf_attr {
> * Return
> * On success, the strictly positive length of the string, including
> * the trailing NUL character. On error, a negative value.
> + *
> + * 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*. Notice the **path** don't support unmountable
> + * pseudo filesystems as they don't have path (eg: SOCKFS, PIPEFS).
> + * The *size* must be strictly positive. On success, the helper
> + * makes sure that the *path* is NUL-terminated, and the buffer
> + * could be:
> + * - a regular full path (include mountable fs eg: /proc, /sys)
> + * - a regular full path with "(deleted)" at the end.
> + * On failure, it is filled with zeroes.
> + * Return
> + * On success, returns the length of the copied string INCLUDING
> + * the trailing NUL.
> + *
> + * On failure, the returned value is one of the following:
> + *
> + * **-EPERM** if no permission to get the path (eg: in irq ctx).
> + *
> + * **-EBADF** if *fd* is invalid.
> + *
> + * **-EINVAL** if *fd* corresponds to a unmountable pseudo fs
> + *
> + * **-ENAMETOOLONG** if full path is longer than *size*
> */
> #define __BPF_FUNC_MAPPER(FN) \
> FN(unspec), \
> @@ -2938,7 +2964,8 @@ union bpf_attr {
> FN(probe_read_user), \
> FN(probe_read_kernel), \
> FN(probe_read_user_str), \
> - FN(probe_read_kernel_str),
> + FN(probe_read_kernel_str), \
> + FN(get_file_path),
I just realized that among my tools that want the path, the input is either:
A) syscall tracepoints: int fd
B) kprobes: struct file *
This serves (A). If we ever add a different helper for (B), we might
think that this helper was misnamed. Should it be called get_fd_path
instead? That leaves get_file_path available for a later "struct file
*" -> pathname helper.
Brendan
--
Brendan Gregg, Senior Performance Architect, Netflix
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH bpf-next v12 1/2] bpf: add new helper get_file_path for mapping a file descriptor to a pathname
2019-12-16 22:09 ` Brendan Gregg
@ 2019-12-17 4:05 ` Wenbo Zhang
0 siblings, 0 replies; 52+ messages in thread
From: Wenbo Zhang @ 2019-12-17 4:05 UTC (permalink / raw)
To: Brendan Gregg
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Yonghong Song,
Andrii Nakryiko, Networking
> I just realized that among my tools that want the path, the input is either:
> A) syscall tracepoints: int fd
> B) kprobes: struct file *
> This serves (A). If we ever add a different helper for (B), we might
> think that this helper was misnamed. Should it be called get_fd_path
> instead? That leaves get_file_path available for a later "struct file
> *" -> pathname helper.
+1, I'll rename it in the next version.
Brendan Gregg <bgregg@netflix.com> 于2019年12月17日周二 上午6:09写道：
>
> On Sat, Dec 14, 2019 at 8:01 PM 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
> >
> > v11->v12: addressed Alexei's feedback
> > - only allow tracepoints to make sure it won't dead lock
> >
> > v10->v11: addressed Al and Alexei's feedback
> > - fix missing fput()
> >
> > v9->v10: addressed Andrii's feedback
> > - send this patch together with the patch selftests as one patch series
> >
> > v8->v9:
> > - format helper description
> >
> > v7->v8: addressed Alexei's feedback
> > - use fget_raw instead of fdget_raw, as fdget_raw is only used inside fs/
> > - ensure we're in user context which is safe fot the help to run
> > - filter unmountable pseudo filesystem, because they don't have real path
> > - supplement the description of this helper function
> >
> > v6->v7:
> > - fix missing signed-off-by line
> >
> > v5->v6: addressed Andrii's feedback
> > - avoid unnecessary goto end by having two explicit returns
> >
> > 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 length
> > 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
> >
> > Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
> > ---
> > include/uapi/linux/bpf.h | 29 +++++++++++++-
> > kernel/trace/bpf_trace.c | 70 ++++++++++++++++++++++++++++++++++
> > tools/include/uapi/linux/bpf.h | 29 +++++++++++++-
> > 3 files changed, 126 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index dbbcf0b02970..71d9705df120 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -2821,6 +2821,32 @@ union bpf_attr {
> > * Return
> > * On success, the strictly positive length of the string, including
> > * the trailing NUL character. On error, a negative value.
> > + *
> > + * 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*. Notice the **path** don't support unmountable
> > + * pseudo filesystems as they don't have path (eg: SOCKFS, PIPEFS).
> > + * The *size* must be strictly positive. On success, the helper
> > + * makes sure that the *path* is NUL-terminated, and the buffer
> > + * could be:
> > + * - a regular full path (include mountable fs eg: /proc, /sys)
> > + * - a regular full path with "(deleted)" at the end.
> > + * On failure, it is filled with zeroes.
> > + * Return
> > + * On success, returns the length of the copied string INCLUDING
> > + * the trailing NUL.
> > + *
> > + * On failure, the returned value is one of the following:
> > + *
> > + * **-EPERM** if no permission to get the path (eg: in irq ctx).
> > + *
> > + * **-EBADF** if *fd* is invalid.
> > + *
> > + * **-EINVAL** if *fd* corresponds to a unmountable pseudo fs
> > + *
> > + * **-ENAMETOOLONG** if full path is longer than *size*
> > */
> > #define __BPF_FUNC_MAPPER(FN) \
> > FN(unspec), \
> > @@ -2938,7 +2964,8 @@ union bpf_attr {
> > FN(probe_read_user), \
> > FN(probe_read_kernel), \
> > FN(probe_read_user_str), \
> > - FN(probe_read_kernel_str),
> > + FN(probe_read_kernel_str), \
> > + 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 e5ef4ae9edb5..db9c0ec46a5d 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -762,6 +762,72 @@ 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 file *f;
> > + char *p;
> > + int ret = -EBADF;
> > +
> > + /* Ensure we're in user context which is safe for the helper to
> > + * run. This helper has no business in a kthread.
> > + */
> > + if (unlikely(in_interrupt() ||
> > + current->flags & (PF_KTHREAD | PF_EXITING))) {
> > + ret = -EPERM;
> > + goto error;
> > + }
> > +
> > + /* Use fget_raw instead of fget to support O_PATH, and it doesn't
> > + * have any sleepable code, so it's ok to be here.
> > + */
> > + f = fget_raw(fd);
> > + if (!f)
> > + goto error;
> > +
> > + /* For unmountable pseudo filesystem, it seems to have no meaning
> > + * to get their fake paths as they don't have path, and to be no
> > + * way to validate this function pointer can be always safe to call
> > + * in the current context.
> > + */
> > + if (f->f_path.dentry->d_op && f->f_path.dentry->d_op->d_dname) {
> > + ret = -EINVAL;
> > + fput(f);
> > + goto error;
> > + }
> > +
> > + /* After filter unmountable pseudo filesytem, d_path won't call
> > + * dentry->d_op->d_name(), the normally path doesn't have any
> > + * sleepable code, and despite it uses the current macro to get
> > + * fs_struct (current->fs), we've already ensured we're in user
> > + * context, so it's ok to be here.
> > + */
> > + p = d_path(&f->f_path, dst, size);
> > + if (IS_ERR(p)) {
> > + ret = PTR_ERR(p);
> > + fput(f);
> > + goto error;
> > + }
> > +
> > + ret = strlen(p);
> > + memmove(dst, p, ret);
> > + dst[ret++] = '\0';
> > + fput(f);
> > + return ret;
> > +
> > +error:
> > + memset(dst, '0', size);
> > + 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)
> > {
> > @@ -953,6 +1019,8 @@ tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> > return &bpf_get_stackid_proto_tp;
> > case BPF_FUNC_get_stack:
> > return &bpf_get_stack_proto_tp;
> > + case BPF_FUNC_get_file_path:
> > + return &bpf_get_file_path_proto;
> > default:
> > return tracing_func_proto(func_id, prog);
> > }
> > @@ -1146,6 +1214,8 @@ raw_tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> > return &bpf_get_stackid_proto_raw_tp;
> > case BPF_FUNC_get_stack:
> > return &bpf_get_stack_proto_raw_tp;
> > + case BPF_FUNC_get_file_path:
> > + return &bpf_get_file_path_proto;
> > default:
> > return tracing_func_proto(func_id, prog);
> > }
> > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > index dbbcf0b02970..71d9705df120 100644
> > --- a/tools/include/uapi/linux/bpf.h
> > +++ b/tools/include/uapi/linux/bpf.h
> > @@ -2821,6 +2821,32 @@ union bpf_attr {
> > * Return
> > * On success, the strictly positive length of the string, including
> > * the trailing NUL character. On error, a negative value.
> > + *
> > + * 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*. Notice the **path** don't support unmountable
> > + * pseudo filesystems as they don't have path (eg: SOCKFS, PIPEFS).
> > + * The *size* must be strictly positive. On success, the helper
> > + * makes sure that the *path* is NUL-terminated, and the buffer
> > + * could be:
> > + * - a regular full path (include mountable fs eg: /proc, /sys)
> > + * - a regular full path with "(deleted)" at the end.
> > + * On failure, it is filled with zeroes.
> > + * Return
> > + * On success, returns the length of the copied string INCLUDING
> > + * the trailing NUL.
> > + *
> > + * On failure, the returned value is one of the following:
> > + *
> > + * **-EPERM** if no permission to get the path (eg: in irq ctx).
> > + *
> > + * **-EBADF** if *fd* is invalid.
> > + *
> > + * **-EINVAL** if *fd* corresponds to a unmountable pseudo fs
> > + *
> > + * **-ENAMETOOLONG** if full path is longer than *size*
> > */
> > #define __BPF_FUNC_MAPPER(FN) \
> > FN(unspec), \
> > @@ -2938,7 +2964,8 @@ union bpf_attr {
> > FN(probe_read_user), \
> > FN(probe_read_kernel), \
> > FN(probe_read_user_str), \
> > - FN(probe_read_kernel_str),
> > + FN(probe_read_kernel_str), \
> > + FN(get_file_path),
>
>
> I just realized that among my tools that want the path, the input is either:
>
> A) syscall tracepoints: int fd
> B) kprobes: struct file *
>
> This serves (A). If we ever add a different helper for (B), we might
> think that this helper was misnamed. Should it be called get_fd_path
> instead? That leaves get_file_path available for a later "struct file
> *" -> pathname helper.
>
> Brendan
>
> --
> Brendan Gregg, Senior Performance Architect, Netflix
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH bpf-next v13 0/2] bpf: adding get_fd_path helper
2019-12-15 4:01 ` [PATCH bpf-next v12 1/2] bpf: add new helper get_file_path for mapping a file descriptor to a pathname Wenbo Zhang
` (2 preceding siblings ...)
2019-12-16 22:09 ` Brendan Gregg
@ 2019-12-17 9:47 ` Wenbo Zhang
2019-12-17 9:47 ` [PATCH bpf-next v13 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname Wenbo Zhang
2019-12-17 9:47 ` [PATCH bpf-next v13 " Wenbo Zhang
3 siblings, 2 replies; 52+ messages in thread
From: Wenbo Zhang @ 2019-12-17 9:47 UTC (permalink / raw)
To: bpf; +Cc: ast, daniel, yhs, bgregg, andrii.nakryiko, netdev
This patch series introduce a bpf helper that can be used to map a file
descriptor to a pathname.
This requirement is mainly discussed here:
https://github.com/iovisor/bcc/issues/237
This implementation supports both local and mountable pseudo file systems,
and ensure we're in user context which is safe for this helper to run.
Changes since v12:
* Rename to get_fd_patch
* Fix test issue on big-endian machines
Changes since v11:
* Only allow tracepoints to make sure it won't dead lock
Changes since v10:
* Fix missing fput
Changes since v9:
* Associate help patch with its selftests patch to this series
* Refactor selftests code for further simplification
Changes since v8:
* Format helper description
Changes since v7:
* Use fget_raw instead of fdget_raw, as fdget_raw is only used inside fs/
* Ensure we're in user context which is safe fot the help to run
* Filter unmountable pseudo filesystem, because they don't have real path
* Supplement the description of this helper function
Changes since v6:
* Fix missing signed-off-by line
Changes since v5:
* Refactor helper avoid unnecessary goto end by having two explicit returns
Changes since v4:
* 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 length
including NUL
* Update helper description's Return bits.
* Refactor selftests code for further simplification
Changes since v3:
* Remove unnecessary LOCKDOWN_BPF_READ
* Refactor error handling section for enhanced readability
* Provide a test case in tools/testing/selftests/bpf
* Refactor sefltests code to use real global variables instead of maps
Changes since v2:
* Fix backward compatibility
* Add helper description
* Refactor selftests use global data instead of perf_buffer to simplified
code
* Fix signed-off name
Wenbo Zhang (2):
bpf: add new helper get_fd_path for mapping a file descriptor to a
pathname
selftests/bpf: test for bpf_get_fd_path() from tracepoint
include/uapi/linux/bpf.h | 29 ++-
kernel/trace/bpf_trace.c | 69 +++++++
tools/include/uapi/linux/bpf.h | 29 ++-
.../selftests/bpf/prog_tests/get_fd_path.c | 171 ++++++++++++++++++
.../selftests/bpf/progs/test_get_fd_path.c | 43 +++++
5 files changed, 339 insertions(+), 2 deletions(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/get_fd_path.c
create mode 100644 tools/testing/selftests/bpf/progs/test_get_fd_path.c
--
2.17.1
^ permalink raw reply [flat|nested] 52+ messages in thread* [PATCH bpf-next v13 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname
2019-12-17 9:47 ` [PATCH bpf-next v13 0/2] bpf: adding get_fd_path helper Wenbo Zhang
@ 2019-12-17 9:47 ` Wenbo Zhang
2019-12-17 16:29 ` Yonghong Song
2019-12-18 0:56 ` [PATCH bpf-next v14 0/2] bpf: adding get_fd_path helper Wenbo Zhang
2019-12-17 9:47 ` [PATCH bpf-next v13 " Wenbo Zhang
1 sibling, 2 replies; 52+ messages in thread
From: Wenbo Zhang @ 2019-12-17 9:47 UTC (permalink / raw)
To: bpf; +Cc: ast, daniel, yhs, bgregg, andrii.nakryiko, netdev
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
v12->v13: addressed Gregg and Yonghong's feedback
- rename to get_fd_path
- refactor code & comment to be clearer and more compliant
v11->v12: addressed Alexei's feedback
- only allow tracepoints to make sure it won't dead lock
v10->v11: addressed Al and Alexei's feedback
- fix missing fput()
v9->v10: addressed Andrii's feedback
- send this patch together with the patch selftests as one patch series
v8->v9:
- format helper description
v7->v8: addressed Alexei's feedback
- use fget_raw instead of fdget_raw, as fdget_raw is only used inside fs/
- ensure we're in user context which is safe fot the help to run
- filter unmountable pseudo filesystem, because they don't have real path
- supplement the description of this helper function
v6->v7:
- fix missing signed-off-by line
v5->v6: addressed Andrii's feedback
- avoid unnecessary goto end by having two explicit returns
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 length
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
Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
---
include/uapi/linux/bpf.h | 29 +++++++++++++-
kernel/trace/bpf_trace.c | 69 ++++++++++++++++++++++++++++++++++
tools/include/uapi/linux/bpf.h | 29 +++++++++++++-
3 files changed, 125 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index dbbcf0b02970..c1e4fd286614 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -2821,6 +2821,32 @@ union bpf_attr {
* Return
* On success, the strictly positive length of the string, including
* the trailing NUL character. On error, a negative value.
+ *
+ * int bpf_get_fd_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*. Notice the **path** don't support unmountable
+ * pseudo filesystems as they don't have path (eg: SOCKFS, PIPEFS).
+ * The *size* must be strictly positive. On success, the helper
+ * makes sure that the *path* is NUL-terminated, and the buffer
+ * could be:
+ * - a regular full path (include mountable fs eg: /proc, /sys)
+ * - a regular full path with "(deleted)" is appended.
+ * On failure, it is filled with zeroes.
+ * Return
+ * On success, returns the length of the copied string INCLUDING
+ * the trailing '\0'.
+ *
+ * On failure, the returned value is one of the following:
+ *
+ * **-EPERM** if no permission to get the path (eg: in irq ctx).
+ *
+ * **-EBADF** if *fd* is invalid.
+ *
+ * **-EINVAL** if *fd* corresponds to a unmountable pseudo fs
+ *
+ * **-ENAMETOOLONG** if full path is longer than *size*
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -2938,7 +2964,8 @@ union bpf_attr {
FN(probe_read_user), \
FN(probe_read_kernel), \
FN(probe_read_user_str), \
- FN(probe_read_kernel_str),
+ FN(probe_read_kernel_str), \
+ FN(get_fd_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 e5ef4ae9edb5..43a6aa6ad967 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -762,6 +762,71 @@ static const struct bpf_func_proto bpf_send_signal_proto = {
.arg1_type = ARG_ANYTHING,
};
+BPF_CALL_3(bpf_get_fd_path, char *, dst, u32, size, int, fd)
+{
+ int ret = -EBADF;
+ struct file *f;
+ char *p;
+
+ /* Ensure we're in user context which is safe for the helper to
+ * run. This helper has no business in a kthread.
+ */
+ if (unlikely(in_interrupt() ||
+ current->flags & (PF_KTHREAD | PF_EXITING))) {
+ ret = -EPERM;
+ goto error;
+ }
+
+ /* Use fget_raw instead of fget to support O_PATH, and it doesn't
+ * have any sleepable code, so it's ok to be here.
+ */
+ f = fget_raw(fd);
+ if (!f)
+ goto error;
+
+ /* For unmountable pseudo filesystem, it seems to have no meaning
+ * to get their fake paths as they don't have path, and to be no
+ * way to validate this function pointer can be always safe to call
+ * in the current context.
+ */
+ if (f->f_path.dentry->d_op && f->f_path.dentry->d_op->d_dname) {
+ ret = -EINVAL;
+ fput(f);
+ goto error;
+ }
+
+ /* After filter unmountable pseudo filesytem, d_path won't call
+ * dentry->d_op->d_name(), the normally path doesn't have any
+ * sleepable code, and despite it uses the current macro to get
+ * fs_struct (current->fs), we've already ensured we're in user
+ * context, so it's ok to be here.
+ */
+ p = d_path(&f->f_path, dst, size);
+ if (IS_ERR(p)) {
+ ret = PTR_ERR(p);
+ fput(f);
+ goto error;
+ }
+
+ ret = strlen(p) + 1;
+ memmove(dst, p, ret);
+ fput(f);
+ return ret;
+
+error:
+ memset(dst, '0', size);
+ return ret;
+}
+
+static const struct bpf_func_proto bpf_get_fd_path_proto = {
+ .func = bpf_get_fd_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)
{
@@ -953,6 +1018,8 @@ tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_get_stackid_proto_tp;
case BPF_FUNC_get_stack:
return &bpf_get_stack_proto_tp;
+ case BPF_FUNC_get_fd_path:
+ return &bpf_get_fd_path_proto;
default:
return tracing_func_proto(func_id, prog);
}
@@ -1146,6 +1213,8 @@ raw_tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_get_stackid_proto_raw_tp;
case BPF_FUNC_get_stack:
return &bpf_get_stack_proto_raw_tp;
+ case BPF_FUNC_get_fd_path:
+ return &bpf_get_fd_path_proto;
default:
return tracing_func_proto(func_id, prog);
}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index dbbcf0b02970..c1e4fd286614 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -2821,6 +2821,32 @@ union bpf_attr {
* Return
* On success, the strictly positive length of the string, including
* the trailing NUL character. On error, a negative value.
+ *
+ * int bpf_get_fd_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*. Notice the **path** don't support unmountable
+ * pseudo filesystems as they don't have path (eg: SOCKFS, PIPEFS).
+ * The *size* must be strictly positive. On success, the helper
+ * makes sure that the *path* is NUL-terminated, and the buffer
+ * could be:
+ * - a regular full path (include mountable fs eg: /proc, /sys)
+ * - a regular full path with "(deleted)" is appended.
+ * On failure, it is filled with zeroes.
+ * Return
+ * On success, returns the length of the copied string INCLUDING
+ * the trailing '\0'.
+ *
+ * On failure, the returned value is one of the following:
+ *
+ * **-EPERM** if no permission to get the path (eg: in irq ctx).
+ *
+ * **-EBADF** if *fd* is invalid.
+ *
+ * **-EINVAL** if *fd* corresponds to a unmountable pseudo fs
+ *
+ * **-ENAMETOOLONG** if full path is longer than *size*
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -2938,7 +2964,8 @@ union bpf_attr {
FN(probe_read_user), \
FN(probe_read_kernel), \
FN(probe_read_user_str), \
- FN(probe_read_kernel_str),
+ FN(probe_read_kernel_str), \
+ FN(get_fd_path),
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
* function eBPF program intends to call
--
2.17.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH bpf-next v13 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname
2019-12-17 9:47 ` [PATCH bpf-next v13 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname Wenbo Zhang
@ 2019-12-17 16:29 ` Yonghong Song
2019-12-17 19:39 ` Daniel Borkmann
2019-12-18 0:06 ` Wenbo Zhang
2019-12-18 0:56 ` [PATCH bpf-next v14 0/2] bpf: adding get_fd_path helper Wenbo Zhang
1 sibling, 2 replies; 52+ messages in thread
From: Yonghong Song @ 2019-12-17 16:29 UTC (permalink / raw)
To: Wenbo Zhang, bpf; +Cc: ast, daniel, bgregg, andrii.nakryiko, netdev
On 12/17/19 1:47 AM, Wenbo Zhang 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
>
> v12->v13: addressed Gregg and Yonghong's feedback
> - rename to get_fd_path
> - refactor code & comment to be clearer and more compliant
>
> v11->v12: addressed Alexei's feedback
> - only allow tracepoints to make sure it won't dead lock
>
> v10->v11: addressed Al and Alexei's feedback
> - fix missing fput()
>
> v9->v10: addressed Andrii's feedback
> - send this patch together with the patch selftests as one patch series
>
> v8->v9:
> - format helper description
>
> v7->v8: addressed Alexei's feedback
> - use fget_raw instead of fdget_raw, as fdget_raw is only used inside fs/
> - ensure we're in user context which is safe fot the help to run
> - filter unmountable pseudo filesystem, because they don't have real path
> - supplement the description of this helper function
>
> v6->v7:
> - fix missing signed-off-by line
>
> v5->v6: addressed Andrii's feedback
> - avoid unnecessary goto end by having two explicit returns
>
> 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 length
> 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
>
> Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
Ack with still the minor issue below, not sure whether another revision
will be needed or not or the maintainer can just fix up before merging.
Acked-by: Yonghong Song <yhs@fb.com>
> ---
> include/uapi/linux/bpf.h | 29 +++++++++++++-
> kernel/trace/bpf_trace.c | 69 ++++++++++++++++++++++++++++++++++
> tools/include/uapi/linux/bpf.h | 29 +++++++++++++-
> 3 files changed, 125 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index dbbcf0b02970..c1e4fd286614 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -2821,6 +2821,32 @@ union bpf_attr {
> * Return
> * On success, the strictly positive length of the string, including
> * the trailing NUL character. On error, a negative value.
> + *
> + * int bpf_get_fd_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*. Notice the **path** don't support unmountable
> + * pseudo filesystems as they don't have path (eg: SOCKFS, PIPEFS).
> + * The *size* must be strictly positive. On success, the helper
> + * makes sure that the *path* is NUL-terminated, and the buffer
> + * could be:
> + * - a regular full path (include mountable fs eg: /proc, /sys)
> + * - a regular full path with "(deleted)" is appended.
Sorry about a little pedantic. In d_path() function comments, we have:
* Convert a dentry into an ASCII path name. If the entry has been deleted
* the string " (deleted)" is appended. Note that this is ambiguous.
Note that there is a space before "(deleted)". I would like to the above
changed to
- a regular full path with " (deleted)" is appended.
> + * On failure, it is filled with zeroes.
> + * Return
> + * On success, returns the length of the copied string INCLUDING
> + * the trailing '\0'.
> + *
> + * On failure, the returned value is one of the following:
> + *
> + * **-EPERM** if no permission to get the path (eg: in irq ctx).
> + *
> + * **-EBADF** if *fd* is invalid.
> + *
> + * **-EINVAL** if *fd* corresponds to a unmountable pseudo fs
> + *
> + * **-ENAMETOOLONG** if full path is longer than *size*
> */
> #define __BPF_FUNC_MAPPER(FN) \
> FN(unspec), \
> @@ -2938,7 +2964,8 @@ union bpf_attr {
> FN(probe_read_user), \
> FN(probe_read_kernel), \
> FN(probe_read_user_str), \
> - FN(probe_read_kernel_str),
> + FN(probe_read_kernel_str), \
> + FN(get_fd_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 e5ef4ae9edb5..43a6aa6ad967 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -762,6 +762,71 @@ static const struct bpf_func_proto bpf_send_signal_proto = {
> .arg1_type = ARG_ANYTHING,
> };
>
> +BPF_CALL_3(bpf_get_fd_path, char *, dst, u32, size, int, fd)
> +{
> + int ret = -EBADF;
> + struct file *f;
> + char *p;
> +
> + /* Ensure we're in user context which is safe for the helper to
> + * run. This helper has no business in a kthread.
> + */
> + if (unlikely(in_interrupt() ||
> + current->flags & (PF_KTHREAD | PF_EXITING))) {
> + ret = -EPERM;
> + goto error;
> + }
> +
> + /* Use fget_raw instead of fget to support O_PATH, and it doesn't
> + * have any sleepable code, so it's ok to be here.
> + */
> + f = fget_raw(fd);
> + if (!f)
> + goto error;
> +
> + /* For unmountable pseudo filesystem, it seems to have no meaning
> + * to get their fake paths as they don't have path, and to be no
> + * way to validate this function pointer can be always safe to call
> + * in the current context.
> + */
> + if (f->f_path.dentry->d_op && f->f_path.dentry->d_op->d_dname) {
> + ret = -EINVAL;
> + fput(f);
> + goto error;
> + }
> +
> + /* After filter unmountable pseudo filesytem, d_path won't call
> + * dentry->d_op->d_name(), the normally path doesn't have any
> + * sleepable code, and despite it uses the current macro to get
> + * fs_struct (current->fs), we've already ensured we're in user
> + * context, so it's ok to be here.
> + */
> + p = d_path(&f->f_path, dst, size);
> + if (IS_ERR(p)) {
> + ret = PTR_ERR(p);
> + fput(f);
> + goto error;
> + }
> +
> + ret = strlen(p) + 1;
> + memmove(dst, p, ret);
> + fput(f);
> + return ret;
> +
> +error:
> + memset(dst, '0', size);
> + return ret;
> +}
> +
> +static const struct bpf_func_proto bpf_get_fd_path_proto = {
> + .func = bpf_get_fd_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)
> {
> @@ -953,6 +1018,8 @@ tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> return &bpf_get_stackid_proto_tp;
> case BPF_FUNC_get_stack:
> return &bpf_get_stack_proto_tp;
> + case BPF_FUNC_get_fd_path:
> + return &bpf_get_fd_path_proto;
> default:
> return tracing_func_proto(func_id, prog);
> }
> @@ -1146,6 +1213,8 @@ raw_tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> return &bpf_get_stackid_proto_raw_tp;
> case BPF_FUNC_get_stack:
> return &bpf_get_stack_proto_raw_tp;
> + case BPF_FUNC_get_fd_path:
> + return &bpf_get_fd_path_proto;
> default:
> return tracing_func_proto(func_id, prog);
> }
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index dbbcf0b02970..c1e4fd286614 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -2821,6 +2821,32 @@ union bpf_attr {
> * Return
> * On success, the strictly positive length of the string, including
> * the trailing NUL character. On error, a negative value.
> + *
> + * int bpf_get_fd_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*. Notice the **path** don't support unmountable
> + * pseudo filesystems as they don't have path (eg: SOCKFS, PIPEFS).
> + * The *size* must be strictly positive. On success, the helper
> + * makes sure that the *path* is NUL-terminated, and the buffer
> + * could be:
> + * - a regular full path (include mountable fs eg: /proc, /sys)
> + * - a regular full path with "(deleted)" is appended.
> + * On failure, it is filled with zeroes.
> + * Return
> + * On success, returns the length of the copied string INCLUDING
> + * the trailing '\0'.
> + *
> + * On failure, the returned value is one of the following:
> + *
> + * **-EPERM** if no permission to get the path (eg: in irq ctx).
> + *
> + * **-EBADF** if *fd* is invalid.
> + *
> + * **-EINVAL** if *fd* corresponds to a unmountable pseudo fs
> + *
> + * **-ENAMETOOLONG** if full path is longer than *size*
> */
> #define __BPF_FUNC_MAPPER(FN) \
> FN(unspec), \
> @@ -2938,7 +2964,8 @@ union bpf_attr {
> FN(probe_read_user), \
> FN(probe_read_kernel), \
> FN(probe_read_user_str), \
> - FN(probe_read_kernel_str),
> + FN(probe_read_kernel_str), \
> + FN(get_fd_path),
>
> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> * function eBPF program intends to call
>
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH bpf-next v13 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname
2019-12-17 16:29 ` Yonghong Song
@ 2019-12-17 19:39 ` Daniel Borkmann
2019-12-18 0:11 ` Wenbo Zhang
2019-12-18 0:06 ` Wenbo Zhang
1 sibling, 1 reply; 52+ messages in thread
From: Daniel Borkmann @ 2019-12-17 19:39 UTC (permalink / raw)
To: Yonghong Song, Wenbo Zhang, bpf; +Cc: ast, bgregg, andrii.nakryiko, netdev
On 12/17/19 5:29 PM, Yonghong Song wrote:
> On 12/17/19 1:47 AM, Wenbo Zhang wrote:
[...]
>> + * On failure, it is filled with zeroes.
[...]
>> */
>> #define __BPF_FUNC_MAPPER(FN) \
>> FN(unspec), \
>> @@ -2938,7 +2964,8 @@ union bpf_attr {
>> FN(probe_read_user), \
>> FN(probe_read_kernel), \
>> FN(probe_read_user_str), \
>> - FN(probe_read_kernel_str),
>> + FN(probe_read_kernel_str), \
>> + FN(get_fd_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 e5ef4ae9edb5..43a6aa6ad967 100644
>> --- a/kernel/trace/bpf_trace.c
>> +++ b/kernel/trace/bpf_trace.c
>> @@ -762,6 +762,71 @@ static const struct bpf_func_proto bpf_send_signal_proto = {
>> .arg1_type = ARG_ANYTHING,
>> };
>>
>> +BPF_CALL_3(bpf_get_fd_path, char *, dst, u32, size, int, fd)
>> +{
>> + int ret = -EBADF;
>> + struct file *f;
>> + char *p;
>> +
>> + /* Ensure we're in user context which is safe for the helper to
>> + * run. This helper has no business in a kthread.
>> + */
>> + if (unlikely(in_interrupt() ||
>> + current->flags & (PF_KTHREAD | PF_EXITING))) {
>> + ret = -EPERM;
>> + goto error;
>> + }
>> +
>> + /* Use fget_raw instead of fget to support O_PATH, and it doesn't
>> + * have any sleepable code, so it's ok to be here.
>> + */
>> + f = fget_raw(fd);
>> + if (!f)
>> + goto error;
>> +
>> + /* For unmountable pseudo filesystem, it seems to have no meaning
>> + * to get their fake paths as they don't have path, and to be no
>> + * way to validate this function pointer can be always safe to call
>> + * in the current context.
>> + */
>> + if (f->f_path.dentry->d_op && f->f_path.dentry->d_op->d_dname) {
>> + ret = -EINVAL;
>> + fput(f);
>> + goto error;
>> + }
>> +
>> + /* After filter unmountable pseudo filesytem, d_path won't call
>> + * dentry->d_op->d_name(), the normally path doesn't have any
>> + * sleepable code, and despite it uses the current macro to get
>> + * fs_struct (current->fs), we've already ensured we're in user
>> + * context, so it's ok to be here.
>> + */
>> + p = d_path(&f->f_path, dst, size);
>> + if (IS_ERR(p)) {
>> + ret = PTR_ERR(p);
>> + fput(f);
>> + goto error;
>> + }
>> +
>> + ret = strlen(p) + 1;
>> + memmove(dst, p, ret);
>> + fput(f);
>> + return ret;
>> +
>> +error:
>> + memset(dst, '0', size);
You fill it with 0x30's ...
>> + return ret;
>> +}
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH bpf-next v13 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname
2019-12-17 19:39 ` Daniel Borkmann
@ 2019-12-18 0:11 ` Wenbo Zhang
0 siblings, 0 replies; 52+ messages in thread
From: Wenbo Zhang @ 2019-12-18 0:11 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: Yonghong Song, bpf, ast, bgregg, andrii.nakryiko, netdev
> [...]
>>> + * On failure, it is filled with zeroes.
> [...]
> You fill it with 0x30's ...
So sorry about this, I'll submit another revision to fix this. Thanks
again for your
preciseness and patience.
Daniel Borkmann <daniel@iogearbox.net> 于2019年12月18日周三 上午3:39写道：
>
> On 12/17/19 5:29 PM, Yonghong Song wrote:
> > On 12/17/19 1:47 AM, Wenbo Zhang wrote:
> [...]
> >> + * On failure, it is filled with zeroes.
> [...]
> >> */
> >> #define __BPF_FUNC_MAPPER(FN) \
> >> FN(unspec), \
> >> @@ -2938,7 +2964,8 @@ union bpf_attr {
> >> FN(probe_read_user), \
> >> FN(probe_read_kernel), \
> >> FN(probe_read_user_str), \
> >> - FN(probe_read_kernel_str),
> >> + FN(probe_read_kernel_str), \
> >> + FN(get_fd_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 e5ef4ae9edb5..43a6aa6ad967 100644
> >> --- a/kernel/trace/bpf_trace.c
> >> +++ b/kernel/trace/bpf_trace.c
> >> @@ -762,6 +762,71 @@ static const struct bpf_func_proto bpf_send_signal_proto = {
> >> .arg1_type = ARG_ANYTHING,
> >> };
> >>
> >> +BPF_CALL_3(bpf_get_fd_path, char *, dst, u32, size, int, fd)
> >> +{
> >> + int ret = -EBADF;
> >> + struct file *f;
> >> + char *p;
> >> +
> >> + /* Ensure we're in user context which is safe for the helper to
> >> + * run. This helper has no business in a kthread.
> >> + */
> >> + if (unlikely(in_interrupt() ||
> >> + current->flags & (PF_KTHREAD | PF_EXITING))) {
> >> + ret = -EPERM;
> >> + goto error;
> >> + }
> >> +
> >> + /* Use fget_raw instead of fget to support O_PATH, and it doesn't
> >> + * have any sleepable code, so it's ok to be here.
> >> + */
> >> + f = fget_raw(fd);
> >> + if (!f)
> >> + goto error;
> >> +
> >> + /* For unmountable pseudo filesystem, it seems to have no meaning
> >> + * to get their fake paths as they don't have path, and to be no
> >> + * way to validate this function pointer can be always safe to call
> >> + * in the current context.
> >> + */
> >> + if (f->f_path.dentry->d_op && f->f_path.dentry->d_op->d_dname) {
> >> + ret = -EINVAL;
> >> + fput(f);
> >> + goto error;
> >> + }
> >> +
> >> + /* After filter unmountable pseudo filesytem, d_path won't call
> >> + * dentry->d_op->d_name(), the normally path doesn't have any
> >> + * sleepable code, and despite it uses the current macro to get
> >> + * fs_struct (current->fs), we've already ensured we're in user
> >> + * context, so it's ok to be here.
> >> + */
> >> + p = d_path(&f->f_path, dst, size);
> >> + if (IS_ERR(p)) {
> >> + ret = PTR_ERR(p);
> >> + fput(f);
> >> + goto error;
> >> + }
> >> +
> >> + ret = strlen(p) + 1;
> >> + memmove(dst, p, ret);
> >> + fput(f);
> >> + return ret;
> >> +
> >> +error:
> >> + memset(dst, '0', size);
>
> You fill it with 0x30's ...
>
> >> + return ret;
> >> +}
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH bpf-next v13 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname
2019-12-17 16:29 ` Yonghong Song
2019-12-17 19:39 ` Daniel Borkmann
@ 2019-12-18 0:06 ` Wenbo Zhang
1 sibling, 0 replies; 52+ messages in thread
From: Wenbo Zhang @ 2019-12-18 0:06 UTC (permalink / raw)
To: Yonghong Song; +Cc: bpf, ast, daniel, bgregg, andrii.nakryiko, netdev
> Sorry about a little pedantic. In d_path() function comments, we have:
> * Convert a dentry into an ASCII path name. If the entry has been deleted
> * the string " (deleted)" is appended. Note that this is ambiguous.
> Note that there is a space before "(deleted)". I would like to the above
> changed to
> - a regular full path with " (deleted)" is appended.
Ah, so sorry about this, I should be more preciseness and thanks again for your
preciseness and patience.I'll submit another revision to fix this.
Yonghong Song <yhs@fb.com> 于2019年12月18日周三 上午12:29写道：
>
>
>
> On 12/17/19 1:47 AM, Wenbo Zhang 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
> >
> > v12->v13: addressed Gregg and Yonghong's feedback
> > - rename to get_fd_path
> > - refactor code & comment to be clearer and more compliant
> >
> > v11->v12: addressed Alexei's feedback
> > - only allow tracepoints to make sure it won't dead lock
> >
> > v10->v11: addressed Al and Alexei's feedback
> > - fix missing fput()
> >
> > v9->v10: addressed Andrii's feedback
> > - send this patch together with the patch selftests as one patch series
> >
> > v8->v9:
> > - format helper description
> >
> > v7->v8: addressed Alexei's feedback
> > - use fget_raw instead of fdget_raw, as fdget_raw is only used inside fs/
> > - ensure we're in user context which is safe fot the help to run
> > - filter unmountable pseudo filesystem, because they don't have real path
> > - supplement the description of this helper function
> >
> > v6->v7:
> > - fix missing signed-off-by line
> >
> > v5->v6: addressed Andrii's feedback
> > - avoid unnecessary goto end by having two explicit returns
> >
> > 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 length
> > 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
> >
> > Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
>
> Ack with still the minor issue below, not sure whether another revision
> will be needed or not or the maintainer can just fix up before merging.
>
> Acked-by: Yonghong Song <yhs@fb.com>
>
>
> > ---
> > include/uapi/linux/bpf.h | 29 +++++++++++++-
> > kernel/trace/bpf_trace.c | 69 ++++++++++++++++++++++++++++++++++
> > tools/include/uapi/linux/bpf.h | 29 +++++++++++++-
> > 3 files changed, 125 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index dbbcf0b02970..c1e4fd286614 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -2821,6 +2821,32 @@ union bpf_attr {
> > * Return
> > * On success, the strictly positive length of the string, including
> > * the trailing NUL character. On error, a negative value.
> > + *
> > + * int bpf_get_fd_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*. Notice the **path** don't support unmountable
> > + * pseudo filesystems as they don't have path (eg: SOCKFS, PIPEFS).
> > + * The *size* must be strictly positive. On success, the helper
> > + * makes sure that the *path* is NUL-terminated, and the buffer
> > + * could be:
> > + * - a regular full path (include mountable fs eg: /proc, /sys)
> > + * - a regular full path with "(deleted)" is appended.
>
> Sorry about a little pedantic. In d_path() function comments, we have:
> * Convert a dentry into an ASCII path name. If the entry has been deleted
> * the string " (deleted)" is appended. Note that this is ambiguous.
>
> Note that there is a space before "(deleted)". I would like to the above
> changed to
> - a regular full path with " (deleted)" is appended.
>
> > + * On failure, it is filled with zeroes.
> > + * Return
> > + * On success, returns the length of the copied string INCLUDING
> > + * the trailing '\0'.
> > + *
> > + * On failure, the returned value is one of the following:
> > + *
> > + * **-EPERM** if no permission to get the path (eg: in irq ctx).
> > + *
> > + * **-EBADF** if *fd* is invalid.
> > + *
> > + * **-EINVAL** if *fd* corresponds to a unmountable pseudo fs
> > + *
> > + * **-ENAMETOOLONG** if full path is longer than *size*
> > */
> > #define __BPF_FUNC_MAPPER(FN) \
> > FN(unspec), \
> > @@ -2938,7 +2964,8 @@ union bpf_attr {
> > FN(probe_read_user), \
> > FN(probe_read_kernel), \
> > FN(probe_read_user_str), \
> > - FN(probe_read_kernel_str),
> > + FN(probe_read_kernel_str), \
> > + FN(get_fd_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 e5ef4ae9edb5..43a6aa6ad967 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -762,6 +762,71 @@ static const struct bpf_func_proto bpf_send_signal_proto = {
> > .arg1_type = ARG_ANYTHING,
> > };
> >
> > +BPF_CALL_3(bpf_get_fd_path, char *, dst, u32, size, int, fd)
> > +{
> > + int ret = -EBADF;
> > + struct file *f;
> > + char *p;
> > +
> > + /* Ensure we're in user context which is safe for the helper to
> > + * run. This helper has no business in a kthread.
> > + */
> > + if (unlikely(in_interrupt() ||
> > + current->flags & (PF_KTHREAD | PF_EXITING))) {
> > + ret = -EPERM;
> > + goto error;
> > + }
> > +
> > + /* Use fget_raw instead of fget to support O_PATH, and it doesn't
> > + * have any sleepable code, so it's ok to be here.
> > + */
> > + f = fget_raw(fd);
> > + if (!f)
> > + goto error;
> > +
> > + /* For unmountable pseudo filesystem, it seems to have no meaning
> > + * to get their fake paths as they don't have path, and to be no
> > + * way to validate this function pointer can be always safe to call
> > + * in the current context.
> > + */
> > + if (f->f_path.dentry->d_op && f->f_path.dentry->d_op->d_dname) {
> > + ret = -EINVAL;
> > + fput(f);
> > + goto error;
> > + }
> > +
> > + /* After filter unmountable pseudo filesytem, d_path won't call
> > + * dentry->d_op->d_name(), the normally path doesn't have any
> > + * sleepable code, and despite it uses the current macro to get
> > + * fs_struct (current->fs), we've already ensured we're in user
> > + * context, so it's ok to be here.
> > + */
> > + p = d_path(&f->f_path, dst, size);
> > + if (IS_ERR(p)) {
> > + ret = PTR_ERR(p);
> > + fput(f);
> > + goto error;
> > + }
> > +
> > + ret = strlen(p) + 1;
> > + memmove(dst, p, ret);
> > + fput(f);
> > + return ret;
> > +
> > +error:
> > + memset(dst, '0', size);
> > + return ret;
> > +}
> > +
> > +static const struct bpf_func_proto bpf_get_fd_path_proto = {
> > + .func = bpf_get_fd_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)
> > {
> > @@ -953,6 +1018,8 @@ tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> > return &bpf_get_stackid_proto_tp;
> > case BPF_FUNC_get_stack:
> > return &bpf_get_stack_proto_tp;
> > + case BPF_FUNC_get_fd_path:
> > + return &bpf_get_fd_path_proto;
> > default:
> > return tracing_func_proto(func_id, prog);
> > }
> > @@ -1146,6 +1213,8 @@ raw_tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> > return &bpf_get_stackid_proto_raw_tp;
> > case BPF_FUNC_get_stack:
> > return &bpf_get_stack_proto_raw_tp;
> > + case BPF_FUNC_get_fd_path:
> > + return &bpf_get_fd_path_proto;
> > default:
> > return tracing_func_proto(func_id, prog);
> > }
> > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > index dbbcf0b02970..c1e4fd286614 100644
> > --- a/tools/include/uapi/linux/bpf.h
> > +++ b/tools/include/uapi/linux/bpf.h
> > @@ -2821,6 +2821,32 @@ union bpf_attr {
> > * Return
> > * On success, the strictly positive length of the string, including
> > * the trailing NUL character. On error, a negative value.
> > + *
> > + * int bpf_get_fd_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*. Notice the **path** don't support unmountable
> > + * pseudo filesystems as they don't have path (eg: SOCKFS, PIPEFS).
> > + * The *size* must be strictly positive. On success, the helper
> > + * makes sure that the *path* is NUL-terminated, and the buffer
> > + * could be:
> > + * - a regular full path (include mountable fs eg: /proc, /sys)
> > + * - a regular full path with "(deleted)" is appended.
> > + * On failure, it is filled with zeroes.
> > + * Return
> > + * On success, returns the length of the copied string INCLUDING
> > + * the trailing '\0'.
> > + *
> > + * On failure, the returned value is one of the following:
> > + *
> > + * **-EPERM** if no permission to get the path (eg: in irq ctx).
> > + *
> > + * **-EBADF** if *fd* is invalid.
> > + *
> > + * **-EINVAL** if *fd* corresponds to a unmountable pseudo fs
> > + *
> > + * **-ENAMETOOLONG** if full path is longer than *size*
> > */
> > #define __BPF_FUNC_MAPPER(FN) \
> > FN(unspec), \
> > @@ -2938,7 +2964,8 @@ union bpf_attr {
> > FN(probe_read_user), \
> > FN(probe_read_kernel), \
> > FN(probe_read_user_str), \
> > - FN(probe_read_kernel_str),
> > + FN(probe_read_kernel_str), \
> > + FN(get_fd_path),
> >
> > /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > * function eBPF program intends to call
> >
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH bpf-next v14 0/2] bpf: adding get_fd_path helper
2019-12-17 9:47 ` [PATCH bpf-next v13 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname Wenbo Zhang
2019-12-17 16:29 ` Yonghong Song
@ 2019-12-18 0:56 ` Wenbo Zhang
2019-12-18 0:56 ` [PATCH bpf-next v14 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname Wenbo Zhang
2019-12-18 0:56 ` [PATCH bpf-next v14 2/2] selftests/bpf: test for bpf_get_fd_path() from tracepoint Wenbo Zhang
1 sibling, 2 replies; 52+ messages in thread
From: Wenbo Zhang @ 2019-12-18 0:56 UTC (permalink / raw)
To: bpf; +Cc: ast, daniel, yhs, bgregg, andrii.nakryiko, netdev
This patch series introduce a bpf helper that can be used to map a file
descriptor to a pathname.
This requirement is mainly discussed here:
https://github.com/iovisor/bcc/issues/237
This implementation supports both local and mountable pseudo file systems,
and ensure we're in user context which is safe for this helper to run.
Changes since v13:
* Fix this helper's description to be consistent with comments in d_path
* Fix error handling logic fill zeroes not '0's
Changes since v12:
* Rename to get_fd_patch
* Fix test issue on big-endian machines
Changes since v11:
* Only allow tracepoints to make sure it won't dead lock
Changes since v10:
* Fix missing fput
Changes since v9:
* Associate help patch with its selftests patch to this series
* Refactor selftests code for further simplification
Changes since v8:
* Format helper description
Changes since v7:
* Use fget_raw instead of fdget_raw, as fdget_raw is only used inside fs/
* Ensure we're in user context which is safe fot the help to run
* Filter unmountable pseudo filesystem, because they don't have real path
* Supplement the description of this helper function
Changes since v6:
* Fix missing signed-off-by line
Changes since v5:
* Refactor helper avoid unnecessary goto end by having two explicit returns
Changes since v4:
* 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 length
including NUL
* Update helper description's Return bits.
* Refactor selftests code for further simplification
Changes since v3:
* Remove unnecessary LOCKDOWN_BPF_READ
* Refactor error handling section for enhanced readability
* Provide a test case in tools/testing/selftests/bpf
* Refactor sefltests code to use real global variables instead of maps
Changes since v2:
* Fix backward compatibility
* Add helper description
* Refactor selftests use global data instead of perf_buffer to simplified
code
* Fix signed-off name
Wenbo Zhang (2):
bpf: add new helper get_fd_path for mapping a file descriptor to a
pathname
selftests/bpf: test for bpf_get_fd_path() from tracepoint
include/uapi/linux/bpf.h | 29 ++-
kernel/trace/bpf_trace.c | 69 +++++++
tools/include/uapi/linux/bpf.h | 29 ++-
.../selftests/bpf/prog_tests/get_fd_path.c | 171 ++++++++++++++++++
.../selftests/bpf/progs/test_get_fd_path.c | 43 +++++
5 files changed, 339 insertions(+), 2 deletions(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/get_fd_path.c
create mode 100644 tools/testing/selftests/bpf/progs/test_get_fd_path.c
--
2.17.1
^ permalink raw reply [flat|nested] 52+ messages in thread* [PATCH bpf-next v14 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname
2019-12-18 0:56 ` [PATCH bpf-next v14 0/2] bpf: adding get_fd_path helper Wenbo Zhang
@ 2019-12-18 0:56 ` Wenbo Zhang
2019-12-18 3:27 ` Yonghong Song
2019-12-19 16:14 ` Daniel Borkmann
2019-12-18 0:56 ` [PATCH bpf-next v14 2/2] selftests/bpf: test for bpf_get_fd_path() from tracepoint Wenbo Zhang
1 sibling, 2 replies; 52+ messages in thread
From: Wenbo Zhang @ 2019-12-18 0:56 UTC (permalink / raw)
To: bpf; +Cc: ast, daniel, yhs, bgregg, andrii.nakryiko, netdev
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
v13->v14: addressed Yonghong and Daniel's feedback
- fix this helper's description to be consistent with comments in d_path
- fix error handling logic fill zeroes not '0's
v12->v13: addressed Brendan and Yonghong's feedback
- rename to get_fd_path
- refactor code & comment to be clearer and more compliant
v11->v12: addressed Alexei's feedback
- only allow tracepoints to make sure it won't dead lock
v10->v11: addressed Al and Alexei's feedback
- fix missing fput()
v9->v10: addressed Andrii's feedback
- send this patch together with the patch selftests as one patch series
v8->v9:
- format helper description
v7->v8: addressed Alexei's feedback
- use fget_raw instead of fdget_raw, as fdget_raw is only used inside fs/
- ensure we're in user context which is safe fot the help to run
- filter unmountable pseudo filesystem, because they don't have real path
- supplement the description of this helper function
v6->v7:
- fix missing signed-off-by line
v5->v6: addressed Andrii's feedback
- avoid unnecessary goto end by having two explicit returns
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 length
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
Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
---
include/uapi/linux/bpf.h | 29 +++++++++++++-
kernel/trace/bpf_trace.c | 69 ++++++++++++++++++++++++++++++++++
tools/include/uapi/linux/bpf.h | 29 +++++++++++++-
3 files changed, 125 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index dbbcf0b02970..4534ce49f838 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -2821,6 +2821,32 @@ union bpf_attr {
* Return
* On success, the strictly positive length of the string, including
* the trailing NUL character. On error, a negative value.
+ *
+ * int bpf_get_fd_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*. Notice the **path** don't support unmountable
+ * pseudo filesystems as they don't have path (eg: SOCKFS, PIPEFS).
+ * The *size* must be strictly positive. On success, the helper
+ * makes sure that the *path* is NUL-terminated, and the buffer
+ * could be:
+ * - a regular full path (include mountable fs eg: /proc, /sys)
+ * - a regular full path with " (deleted)" is appended.
+ * On failure, it is filled with zeroes.
+ * Return
+ * On success, returns the length of the copied string INCLUDING
+ * the trailing '\0'.
+ *
+ * On failure, the returned value is one of the following:
+ *
+ * **-EPERM** if no permission to get the path (eg: in irq ctx).
+ *
+ * **-EBADF** if *fd* is invalid.
+ *
+ * **-EINVAL** if *fd* corresponds to a unmountable pseudo fs
+ *
+ * **-ENAMETOOLONG** if full path is longer than *size*
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -2938,7 +2964,8 @@ union bpf_attr {
FN(probe_read_user), \
FN(probe_read_kernel), \
FN(probe_read_user_str), \
- FN(probe_read_kernel_str),
+ FN(probe_read_kernel_str), \
+ FN(get_fd_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 e5ef4ae9edb5..a2c18b193141 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -762,6 +762,71 @@ static const struct bpf_func_proto bpf_send_signal_proto = {
.arg1_type = ARG_ANYTHING,
};
+BPF_CALL_3(bpf_get_fd_path, char *, dst, u32, size, int, fd)
+{
+ int ret = -EBADF;
+ struct file *f;
+ char *p;
+
+ /* Ensure we're in user context which is safe for the helper to
+ * run. This helper has no business in a kthread.
+ */
+ if (unlikely(in_interrupt() ||
+ current->flags & (PF_KTHREAD | PF_EXITING))) {
+ ret = -EPERM;
+ goto error;
+ }
+
+ /* Use fget_raw instead of fget to support O_PATH, and it doesn't
+ * have any sleepable code, so it's ok to be here.
+ */
+ f = fget_raw(fd);
+ if (!f)
+ goto error;
+
+ /* For unmountable pseudo filesystem, it seems to have no meaning
+ * to get their fake paths as they don't have path, and to be no
+ * way to validate this function pointer can be always safe to call
+ * in the current context.
+ */
+ if (f->f_path.dentry->d_op && f->f_path.dentry->d_op->d_dname) {
+ ret = -EINVAL;
+ fput(f);
+ goto error;
+ }
+
+ /* After filter unmountable pseudo filesytem, d_path won't call
+ * dentry->d_op->d_name(), the normally path doesn't have any
+ * sleepable code, and despite it uses the current macro to get
+ * fs_struct (current->fs), we've already ensured we're in user
+ * context, so it's ok to be here.
+ */
+ p = d_path(&f->f_path, dst, size);
+ if (IS_ERR(p)) {
+ ret = PTR_ERR(p);
+ fput(f);
+ goto error;
+ }
+
+ ret = strlen(p) + 1;
+ memmove(dst, p, ret);
+ fput(f);
+ return ret;
+
+error:
+ memset(dst, 0, size);
+ return ret;
+}
+
+static const struct bpf_func_proto bpf_get_fd_path_proto = {
+ .func = bpf_get_fd_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)
{
@@ -953,6 +1018,8 @@ tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_get_stackid_proto_tp;
case BPF_FUNC_get_stack:
return &bpf_get_stack_proto_tp;
+ case BPF_FUNC_get_fd_path:
+ return &bpf_get_fd_path_proto;
default:
return tracing_func_proto(func_id, prog);
}
@@ -1146,6 +1213,8 @@ raw_tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_get_stackid_proto_raw_tp;
case BPF_FUNC_get_stack:
return &bpf_get_stack_proto_raw_tp;
+ case BPF_FUNC_get_fd_path:
+ return &bpf_get_fd_path_proto;
default:
return tracing_func_proto(func_id, prog);
}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index dbbcf0b02970..4534ce49f838 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -2821,6 +2821,32 @@ union bpf_attr {
* Return
* On success, the strictly positive length of the string, including
* the trailing NUL character. On error, a negative value.
+ *
+ * int bpf_get_fd_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*. Notice the **path** don't support unmountable
+ * pseudo filesystems as they don't have path (eg: SOCKFS, PIPEFS).
+ * The *size* must be strictly positive. On success, the helper
+ * makes sure that the *path* is NUL-terminated, and the buffer
+ * could be:
+ * - a regular full path (include mountable fs eg: /proc, /sys)
+ * - a regular full path with " (deleted)" is appended.
+ * On failure, it is filled with zeroes.
+ * Return
+ * On success, returns the length of the copied string INCLUDING
+ * the trailing '\0'.
+ *
+ * On failure, the returned value is one of the following:
+ *
+ * **-EPERM** if no permission to get the path (eg: in irq ctx).
+ *
+ * **-EBADF** if *fd* is invalid.
+ *
+ * **-EINVAL** if *fd* corresponds to a unmountable pseudo fs
+ *
+ * **-ENAMETOOLONG** if full path is longer than *size*
*/
#define __BPF_FUNC_MAPPER(FN) \
FN(unspec), \
@@ -2938,7 +2964,8 @@ union bpf_attr {
FN(probe_read_user), \
FN(probe_read_kernel), \
FN(probe_read_user_str), \
- FN(probe_read_kernel_str),
+ FN(probe_read_kernel_str), \
+ FN(get_fd_path),
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
* function eBPF program intends to call
--
2.17.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH bpf-next v14 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname
2019-12-18 0:56 ` [PATCH bpf-next v14 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname Wenbo Zhang
@ 2019-12-18 3:27 ` Yonghong Song
2019-12-19 16:14 ` Daniel Borkmann
1 sibling, 0 replies; 52+ messages in thread
From: Yonghong Song @ 2019-12-18 3:27 UTC (permalink / raw)
To: Wenbo Zhang, bpf; +Cc: ast, daniel, bgregg, andrii.nakryiko, netdev
On 12/17/19 4:56 PM, Wenbo Zhang 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
>
> v13->v14: addressed Yonghong and Daniel's feedback
> - fix this helper's description to be consistent with comments in d_path
> - fix error handling logic fill zeroes not '0's
>
> v12->v13: addressed Brendan and Yonghong's feedback
> - rename to get_fd_path
> - refactor code & comment to be clearer and more compliant
>
> v11->v12: addressed Alexei's feedback
> - only allow tracepoints to make sure it won't dead lock
>
> v10->v11: addressed Al and Alexei's feedback
> - fix missing fput()
>
> v9->v10: addressed Andrii's feedback
> - send this patch together with the patch selftests as one patch series
>
> v8->v9:
> - format helper description
>
> v7->v8: addressed Alexei's feedback
> - use fget_raw instead of fdget_raw, as fdget_raw is only used inside fs/
> - ensure we're in user context which is safe fot the help to run
> - filter unmountable pseudo filesystem, because they don't have real path
> - supplement the description of this helper function
>
> v6->v7:
> - fix missing signed-off-by line
>
> v5->v6: addressed Andrii's feedback
> - avoid unnecessary goto end by having two explicit returns
>
> 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 length
> 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
>
> Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
Acked-by: Yonghong Song <yhs@fb.com>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH bpf-next v14 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname
2019-12-18 0:56 ` [PATCH bpf-next v14 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname Wenbo Zhang
2019-12-18 3:27 ` Yonghong Song
@ 2019-12-19 16:14 ` Daniel Borkmann
2019-12-20 3:35 ` Wenbo Zhang
1 sibling, 1 reply; 52+ messages in thread
From: Daniel Borkmann @ 2019-12-19 16:14 UTC (permalink / raw)
To: Wenbo Zhang, bpf; +Cc: ast, yhs, bgregg, andrii.nakryiko, netdev, viro
[ Wenbo, please keep also Al (added here) in the loop since he was providing
feedback on prior submissions as well wrt vfs bits. ]
On 12/18/19 1:56 AM, Wenbo Zhang 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
>
> v13->v14: addressed Yonghong and Daniel's feedback
> - fix this helper's description to be consistent with comments in d_path
> - fix error handling logic fill zeroes not '0's
>
> v12->v13: addressed Brendan and Yonghong's feedback
> - rename to get_fd_path
> - refactor code & comment to be clearer and more compliant
>
> v11->v12: addressed Alexei's feedback
> - only allow tracepoints to make sure it won't dead lock
>
> v10->v11: addressed Al and Alexei's feedback
> - fix missing fput()
>
> v9->v10: addressed Andrii's feedback
> - send this patch together with the patch selftests as one patch series
>
> v8->v9:
> - format helper description
>
> v7->v8: addressed Alexei's feedback
> - use fget_raw instead of fdget_raw, as fdget_raw is only used inside fs/
> - ensure we're in user context which is safe fot the help to run
> - filter unmountable pseudo filesystem, because they don't have real path
> - supplement the description of this helper function
>
> v6->v7:
> - fix missing signed-off-by line
>
> v5->v6: addressed Andrii's feedback
> - avoid unnecessary goto end by having two explicit returns
>
> 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 length
> 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
>
> Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
> ---
> include/uapi/linux/bpf.h | 29 +++++++++++++-
> kernel/trace/bpf_trace.c | 69 ++++++++++++++++++++++++++++++++++
> tools/include/uapi/linux/bpf.h | 29 +++++++++++++-
> 3 files changed, 125 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index dbbcf0b02970..4534ce49f838 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -2821,6 +2821,32 @@ union bpf_attr {
> * Return
> * On success, the strictly positive length of the string, including
> * the trailing NUL character. On error, a negative value.
> + *
> + * int bpf_get_fd_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*. Notice the **path** don't support unmountable
> + * pseudo filesystems as they don't have path (eg: SOCKFS, PIPEFS).
> + * The *size* must be strictly positive. On success, the helper
> + * makes sure that the *path* is NUL-terminated, and the buffer
> + * could be:
> + * - a regular full path (include mountable fs eg: /proc, /sys)
> + * - a regular full path with " (deleted)" is appended.
> + * On failure, it is filled with zeroes.
> + * Return
> + * On success, returns the length of the copied string INCLUDING
> + * the trailing '\0'.
> + *
> + * On failure, the returned value is one of the following:
> + *
> + * **-EPERM** if no permission to get the path (eg: in irq ctx).
> + *
> + * **-EBADF** if *fd* is invalid.
> + *
> + * **-EINVAL** if *fd* corresponds to a unmountable pseudo fs
> + *
> + * **-ENAMETOOLONG** if full path is longer than *size*
> */
> #define __BPF_FUNC_MAPPER(FN) \
> FN(unspec), \
> @@ -2938,7 +2964,8 @@ union bpf_attr {
> FN(probe_read_user), \
> FN(probe_read_kernel), \
> FN(probe_read_user_str), \
> - FN(probe_read_kernel_str),
> + FN(probe_read_kernel_str), \
> + FN(get_fd_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 e5ef4ae9edb5..a2c18b193141 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -762,6 +762,71 @@ static const struct bpf_func_proto bpf_send_signal_proto = {
> .arg1_type = ARG_ANYTHING,
> };
>
> +BPF_CALL_3(bpf_get_fd_path, char *, dst, u32, size, int, fd)
> +{
> + int ret = -EBADF;
> + struct file *f;
> + char *p;
> +
> + /* Ensure we're in user context which is safe for the helper to
> + * run. This helper has no business in a kthread.
> + */
> + if (unlikely(in_interrupt() ||
> + current->flags & (PF_KTHREAD | PF_EXITING))) {
> + ret = -EPERM;
> + goto error;
> + }
> +
> + /* Use fget_raw instead of fget to support O_PATH, and it doesn't
> + * have any sleepable code, so it's ok to be here.
> + */
> + f = fget_raw(fd);
> + if (!f)
> + goto error;
> +
> + /* For unmountable pseudo filesystem, it seems to have no meaning
> + * to get their fake paths as they don't have path, and to be no
> + * way to validate this function pointer can be always safe to call
> + * in the current context.
> + */
> + if (f->f_path.dentry->d_op && f->f_path.dentry->d_op->d_dname) {
> + ret = -EINVAL;
> + fput(f);
> + goto error;
> + }
> +
> + /* After filter unmountable pseudo filesytem, d_path won't call
> + * dentry->d_op->d_name(), the normally path doesn't have any
> + * sleepable code, and despite it uses the current macro to get
> + * fs_struct (current->fs), we've already ensured we're in user
> + * context, so it's ok to be here.
> + */
> + p = d_path(&f->f_path, dst, size);
> + if (IS_ERR(p)) {
> + ret = PTR_ERR(p);
> + fput(f);
> + goto error;
> + }
> +
> + ret = strlen(p) + 1;
> + memmove(dst, p, ret);
> + fput(f);
> + return ret;
> +
> +error:
> + memset(dst, 0, size);
> + return ret;
> +}
> +
> +static const struct bpf_func_proto bpf_get_fd_path_proto = {
> + .func = bpf_get_fd_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)
> {
> @@ -953,6 +1018,8 @@ tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> return &bpf_get_stackid_proto_tp;
> case BPF_FUNC_get_stack:
> return &bpf_get_stack_proto_tp;
> + case BPF_FUNC_get_fd_path:
> + return &bpf_get_fd_path_proto;
> default:
> return tracing_func_proto(func_id, prog);
> }
> @@ -1146,6 +1213,8 @@ raw_tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> return &bpf_get_stackid_proto_raw_tp;
> case BPF_FUNC_get_stack:
> return &bpf_get_stack_proto_raw_tp;
> + case BPF_FUNC_get_fd_path:
> + return &bpf_get_fd_path_proto;
> default:
> return tracing_func_proto(func_id, prog);
> }
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index dbbcf0b02970..4534ce49f838 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -2821,6 +2821,32 @@ union bpf_attr {
> * Return
> * On success, the strictly positive length of the string, including
> * the trailing NUL character. On error, a negative value.
> + *
> + * int bpf_get_fd_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*. Notice the **path** don't support unmountable
> + * pseudo filesystems as they don't have path (eg: SOCKFS, PIPEFS).
> + * The *size* must be strictly positive. On success, the helper
> + * makes sure that the *path* is NUL-terminated, and the buffer
> + * could be:
> + * - a regular full path (include mountable fs eg: /proc, /sys)
> + * - a regular full path with " (deleted)" is appended.
> + * On failure, it is filled with zeroes.
> + * Return
> + * On success, returns the length of the copied string INCLUDING
> + * the trailing '\0'.
> + *
> + * On failure, the returned value is one of the following:
> + *
> + * **-EPERM** if no permission to get the path (eg: in irq ctx).
> + *
> + * **-EBADF** if *fd* is invalid.
> + *
> + * **-EINVAL** if *fd* corresponds to a unmountable pseudo fs
> + *
> + * **-ENAMETOOLONG** if full path is longer than *size*
> */
> #define __BPF_FUNC_MAPPER(FN) \
> FN(unspec), \
> @@ -2938,7 +2964,8 @@ union bpf_attr {
> FN(probe_read_user), \
> FN(probe_read_kernel), \
> FN(probe_read_user_str), \
> - FN(probe_read_kernel_str),
> + FN(probe_read_kernel_str), \
> + FN(get_fd_path),
>
> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> * function eBPF program intends to call
>
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH bpf-next v14 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname
2019-12-19 16:14 ` Daniel Borkmann
@ 2019-12-20 3:35 ` Wenbo Zhang
2020-01-16 8:59 ` Jiri Olsa
0 siblings, 1 reply; 52+ messages in thread
From: Wenbo Zhang @ 2019-12-20 3:35 UTC (permalink / raw)
To: Daniel Borkmann
Cc: bpf, Alexei Starovoitov, Yonghong Song, Brendan Gregg,
Andrii Nakryiko, Networking, viro
> [ Wenbo, please keep also Al (added here) in the loop since he was providing
> feedback on prior submissions as well wrt vfs bits. ]
Get it, thank you!
Daniel Borkmann <daniel@iogearbox.net> 于2019年12月20日周五 上午12:14写道：
>
> [ Wenbo, please keep also Al (added here) in the loop since he was providing
> feedback on prior submissions as well wrt vfs bits. ]
>
> On 12/18/19 1:56 AM, Wenbo Zhang 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
> >
> > v13->v14: addressed Yonghong and Daniel's feedback
> > - fix this helper's description to be consistent with comments in d_path
> > - fix error handling logic fill zeroes not '0's
> >
> > v12->v13: addressed Brendan and Yonghong's feedback
> > - rename to get_fd_path
> > - refactor code & comment to be clearer and more compliant
> >
> > v11->v12: addressed Alexei's feedback
> > - only allow tracepoints to make sure it won't dead lock
> >
> > v10->v11: addressed Al and Alexei's feedback
> > - fix missing fput()
> >
> > v9->v10: addressed Andrii's feedback
> > - send this patch together with the patch selftests as one patch series
> >
> > v8->v9:
> > - format helper description
> >
> > v7->v8: addressed Alexei's feedback
> > - use fget_raw instead of fdget_raw, as fdget_raw is only used inside fs/
> > - ensure we're in user context which is safe fot the help to run
> > - filter unmountable pseudo filesystem, because they don't have real path
> > - supplement the description of this helper function
> >
> > v6->v7:
> > - fix missing signed-off-by line
> >
> > v5->v6: addressed Andrii's feedback
> > - avoid unnecessary goto end by having two explicit returns
> >
> > 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 length
> > 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
> >
> > Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
> > ---
> > include/uapi/linux/bpf.h | 29 +++++++++++++-
> > kernel/trace/bpf_trace.c | 69 ++++++++++++++++++++++++++++++++++
> > tools/include/uapi/linux/bpf.h | 29 +++++++++++++-
> > 3 files changed, 125 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index dbbcf0b02970..4534ce49f838 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -2821,6 +2821,32 @@ union bpf_attr {
> > * Return
> > * On success, the strictly positive length of the string, including
> > * the trailing NUL character. On error, a negative value.
> > + *
> > + * int bpf_get_fd_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*. Notice the **path** don't support unmountable
> > + * pseudo filesystems as they don't have path (eg: SOCKFS, PIPEFS).
> > + * The *size* must be strictly positive. On success, the helper
> > + * makes sure that the *path* is NUL-terminated, and the buffer
> > + * could be:
> > + * - a regular full path (include mountable fs eg: /proc, /sys)
> > + * - a regular full path with " (deleted)" is appended.
> > + * On failure, it is filled with zeroes.
> > + * Return
> > + * On success, returns the length of the copied string INCLUDING
> > + * the trailing '\0'.
> > + *
> > + * On failure, the returned value is one of the following:
> > + *
> > + * **-EPERM** if no permission to get the path (eg: in irq ctx).
> > + *
> > + * **-EBADF** if *fd* is invalid.
> > + *
> > + * **-EINVAL** if *fd* corresponds to a unmountable pseudo fs
> > + *
> > + * **-ENAMETOOLONG** if full path is longer than *size*
> > */
> > #define __BPF_FUNC_MAPPER(FN) \
> > FN(unspec), \
> > @@ -2938,7 +2964,8 @@ union bpf_attr {
> > FN(probe_read_user), \
> > FN(probe_read_kernel), \
> > FN(probe_read_user_str), \
> > - FN(probe_read_kernel_str),
> > + FN(probe_read_kernel_str), \
> > + FN(get_fd_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 e5ef4ae9edb5..a2c18b193141 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -762,6 +762,71 @@ static const struct bpf_func_proto bpf_send_signal_proto = {
> > .arg1_type = ARG_ANYTHING,
> > };
> >
> > +BPF_CALL_3(bpf_get_fd_path, char *, dst, u32, size, int, fd)
> > +{
> > + int ret = -EBADF;
> > + struct file *f;
> > + char *p;
> > +
> > + /* Ensure we're in user context which is safe for the helper to
> > + * run. This helper has no business in a kthread.
> > + */
> > + if (unlikely(in_interrupt() ||
> > + current->flags & (PF_KTHREAD | PF_EXITING))) {
> > + ret = -EPERM;
> > + goto error;
> > + }
> > +
> > + /* Use fget_raw instead of fget to support O_PATH, and it doesn't
> > + * have any sleepable code, so it's ok to be here.
> > + */
> > + f = fget_raw(fd);
> > + if (!f)
> > + goto error;
> > +
> > + /* For unmountable pseudo filesystem, it seems to have no meaning
> > + * to get their fake paths as they don't have path, and to be no
> > + * way to validate this function pointer can be always safe to call
> > + * in the current context.
> > + */
> > + if (f->f_path.dentry->d_op && f->f_path.dentry->d_op->d_dname) {
> > + ret = -EINVAL;
> > + fput(f);
> > + goto error;
> > + }
> > +
> > + /* After filter unmountable pseudo filesytem, d_path won't call
> > + * dentry->d_op->d_name(), the normally path doesn't have any
> > + * sleepable code, and despite it uses the current macro to get
> > + * fs_struct (current->fs), we've already ensured we're in user
> > + * context, so it's ok to be here.
> > + */
> > + p = d_path(&f->f_path, dst, size);
> > + if (IS_ERR(p)) {
> > + ret = PTR_ERR(p);
> > + fput(f);
> > + goto error;
> > + }
> > +
> > + ret = strlen(p) + 1;
> > + memmove(dst, p, ret);
> > + fput(f);
> > + return ret;
> > +
> > +error:
> > + memset(dst, 0, size);
> > + return ret;
> > +}
> > +
> > +static const struct bpf_func_proto bpf_get_fd_path_proto = {
> > + .func = bpf_get_fd_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)
> > {
> > @@ -953,6 +1018,8 @@ tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> > return &bpf_get_stackid_proto_tp;
> > case BPF_FUNC_get_stack:
> > return &bpf_get_stack_proto_tp;
> > + case BPF_FUNC_get_fd_path:
> > + return &bpf_get_fd_path_proto;
> > default:
> > return tracing_func_proto(func_id, prog);
> > }
> > @@ -1146,6 +1213,8 @@ raw_tp_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> > return &bpf_get_stackid_proto_raw_tp;
> > case BPF_FUNC_get_stack:
> > return &bpf_get_stack_proto_raw_tp;
> > + case BPF_FUNC_get_fd_path:
> > + return &bpf_get_fd_path_proto;
> > default:
> > return tracing_func_proto(func_id, prog);
> > }
> > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > index dbbcf0b02970..4534ce49f838 100644
> > --- a/tools/include/uapi/linux/bpf.h
> > +++ b/tools/include/uapi/linux/bpf.h
> > @@ -2821,6 +2821,32 @@ union bpf_attr {
> > * Return
> > * On success, the strictly positive length of the string, including
> > * the trailing NUL character. On error, a negative value.
> > + *
> > + * int bpf_get_fd_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*. Notice the **path** don't support unmountable
> > + * pseudo filesystems as they don't have path (eg: SOCKFS, PIPEFS).
> > + * The *size* must be strictly positive. On success, the helper
> > + * makes sure that the *path* is NUL-terminated, and the buffer
> > + * could be:
> > + * - a regular full path (include mountable fs eg: /proc, /sys)
> > + * - a regular full path with " (deleted)" is appended.
> > + * On failure, it is filled with zeroes.
> > + * Return
> > + * On success, returns the length of the copied string INCLUDING
> > + * the trailing '\0'.
> > + *
> > + * On failure, the returned value is one of the following:
> > + *
> > + * **-EPERM** if no permission to get the path (eg: in irq ctx).
> > + *
> > + * **-EBADF** if *fd* is invalid.
> > + *
> > + * **-EINVAL** if *fd* corresponds to a unmountable pseudo fs
> > + *
> > + * **-ENAMETOOLONG** if full path is longer than *size*
> > */
> > #define __BPF_FUNC_MAPPER(FN) \
> > FN(unspec), \
> > @@ -2938,7 +2964,8 @@ union bpf_attr {
> > FN(probe_read_user), \
> > FN(probe_read_kernel), \
> > FN(probe_read_user_str), \
> > - FN(probe_read_kernel_str),
> > + FN(probe_read_kernel_str), \
> > + FN(get_fd_path),
> >
> > /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > * function eBPF program intends to call
> >
>
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH bpf-next v14 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname
2019-12-20 3:35 ` Wenbo Zhang
@ 2020-01-16 8:59 ` Jiri Olsa
2020-02-10 4:43 ` Brendan Gregg
0 siblings, 1 reply; 52+ messages in thread
From: Jiri Olsa @ 2020-01-16 8:59 UTC (permalink / raw)
To: Wenbo Zhang
Cc: Daniel Borkmann, bpf, Alexei Starovoitov, Yonghong Song,
Brendan Gregg, Andrii Nakryiko, Networking, viro
On Fri, Dec 20, 2019 at 11:35:08AM +0800, Wenbo Zhang wrote:
> > [ Wenbo, please keep also Al (added here) in the loop since he was providing
> > feedback on prior submissions as well wrt vfs bits. ]
>
> Get it, thank you!
hi,
is this stuck on review? I'd like to see this merged ;-)
we have bpftrace change using it already.. from that side:
Tested-by: Jiri Olsa <jolsa@kernel.org>
thanks,
jirka
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH bpf-next v14 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname
2020-01-16 8:59 ` Jiri Olsa
@ 2020-02-10 4:43 ` Brendan Gregg
2020-02-11 0:01 ` Daniel Borkmann
0 siblings, 1 reply; 52+ messages in thread
From: Brendan Gregg @ 2020-02-10 4:43 UTC (permalink / raw)
To: Jiri Olsa
Cc: Wenbo Zhang, Daniel Borkmann, bpf, Alexei Starovoitov,
Yonghong Song, Andrii Nakryiko, Networking, Al Viro
On Thu, Jan 16, 2020 at 12:59 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Fri, Dec 20, 2019 at 11:35:08AM +0800, Wenbo Zhang wrote:
> > > [ Wenbo, please keep also Al (added here) in the loop since he was providing
> > > feedback on prior submissions as well wrt vfs bits. ]
> >
> > Get it, thank you!
>
> hi,
> is this stuck on review? I'd like to see this merged ;-)
Is this still waiting on someone? I'm writing some docs on analyzing
file systems via syscall tracing and this will be a big improvement.
Thanks,
Brendan
>
> we have bpftrace change using it already.. from that side:
>
> Tested-by: Jiri Olsa <jolsa@kernel.org>
>
> thanks,
> jirka
>
--
Brendan Gregg, Senior Performance Architect, Netflix
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH bpf-next v14 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname
2020-02-10 4:43 ` Brendan Gregg
@ 2020-02-11 0:01 ` Daniel Borkmann
2020-02-12 15:21 ` Jiri Olsa
0 siblings, 1 reply; 52+ messages in thread
From: Daniel Borkmann @ 2020-02-11 0:01 UTC (permalink / raw)
To: Brendan Gregg, Jiri Olsa
Cc: Wenbo Zhang, bpf, Alexei Starovoitov, Yonghong Song,
Andrii Nakryiko, Networking, Al Viro
On 2/10/20 5:43 AM, Brendan Gregg wrote:
> On Thu, Jan 16, 2020 at 12:59 AM Jiri Olsa <jolsa@redhat.com> wrote:
>> On Fri, Dec 20, 2019 at 11:35:08AM +0800, Wenbo Zhang wrote:
>>>> [ Wenbo, please keep also Al (added here) in the loop since he was providing
>>>> feedback on prior submissions as well wrt vfs bits. ]
>>>
>>> Get it, thank you!
>>
>> hi,
>> is this stuck on review? I'd like to see this merged ;-)
>
> Is this still waiting on someone? I'm writing some docs on analyzing
> file systems via syscall tracing and this will be a big improvement.
It was waiting on final review/ACK from vfs folks, but given that didn't
happen so far :/, this series should get rebased for proceeding with merge.
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH bpf-next v14 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname
2020-02-11 0:01 ` Daniel Borkmann
@ 2020-02-12 15:21 ` Jiri Olsa
2020-06-01 14:17 ` Wenbo Zhang
0 siblings, 1 reply; 52+ messages in thread
From: Jiri Olsa @ 2020-02-12 15:21 UTC (permalink / raw)
To: Daniel Borkmann, Al Viro
Cc: Brendan Gregg, Wenbo Zhang, bpf, Alexei Starovoitov,
Yonghong Song, Andrii Nakryiko, Al Viro
On Tue, Feb 11, 2020 at 01:01:16AM +0100, Daniel Borkmann wrote:
> On 2/10/20 5:43 AM, Brendan Gregg wrote:
> > On Thu, Jan 16, 2020 at 12:59 AM Jiri Olsa <jolsa@redhat.com> wrote:
> > > On Fri, Dec 20, 2019 at 11:35:08AM +0800, Wenbo Zhang wrote:
> > > > > [ Wenbo, please keep also Al (added here) in the loop since he was providing
> > > > > feedback on prior submissions as well wrt vfs bits. ]
> > > >
> > > > Get it, thank you!
> > >
> > > hi,
> > > is this stuck on review? I'd like to see this merged ;-)
> >
> > Is this still waiting on someone? I'm writing some docs on analyzing
> > file systems via syscall tracing and this will be a big improvement.
>
> It was waiting on final review/ACK from vfs folks, but given that didn't
> happen so far :/, this series should get rebased for proceeding with merge.
>
Al Viro, any chance you could check on the latest version?
thanks,
jirka
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH bpf-next v14 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname
2020-02-12 15:21 ` Jiri Olsa
@ 2020-06-01 14:17 ` Wenbo Zhang
2020-06-01 16:38 ` Alexei Starovoitov
0 siblings, 1 reply; 52+ messages in thread
From: Wenbo Zhang @ 2020-06-01 14:17 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Jiri Olsa, Al Viro, Brendan Gregg, bpf, Alexei Starovoitov,
Yonghong Song, Andrii Nakryiko
Hi Daniel,
I find https://patchwork.ozlabs.org/project/netdev/patch/7464919bd9c15f2496ca29dceb6a4048b3199774.1576629200.git.ethercflow@gmail.com/
this PR's current state is Awaiting Upstream. I don't know much about
this state. I want to ask if this PR will be merged.
Thank you
Wenbo
Jiri Olsa <jolsa@redhat.com> 于2020年2月12日周三 下午11:22写道：
>
> On Tue, Feb 11, 2020 at 01:01:16AM +0100, Daniel Borkmann wrote:
> > On 2/10/20 5:43 AM, Brendan Gregg wrote:
> > > On Thu, Jan 16, 2020 at 12:59 AM Jiri Olsa <jolsa@redhat.com> wrote:
> > > > On Fri, Dec 20, 2019 at 11:35:08AM +0800, Wenbo Zhang wrote:
> > > > > > [ Wenbo, please keep also Al (added here) in the loop since he was providing
> > > > > > feedback on prior submissions as well wrt vfs bits. ]
> > > > >
> > > > > Get it, thank you!
> > > >
> > > > hi,
> > > > is this stuck on review? I'd like to see this merged ;-)
> > >
> > > Is this still waiting on someone? I'm writing some docs on analyzing
> > > file systems via syscall tracing and this will be a big improvement.
> >
> > It was waiting on final review/ACK from vfs folks, but given that didn't
> > happen so far :/, this series should get rebased for proceeding with merge.
> >
>
> Al Viro, any chance you could check on the latest version?
>
> thanks,
> jirka
>
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH bpf-next v14 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname
2020-06-01 14:17 ` Wenbo Zhang
@ 2020-06-01 16:38 ` Alexei Starovoitov
2020-06-02 3:04 ` Wenbo Zhang
0 siblings, 1 reply; 52+ messages in thread
From: Alexei Starovoitov @ 2020-06-01 16:38 UTC (permalink / raw)
To: Wenbo Zhang
Cc: Daniel Borkmann, Jiri Olsa, Al Viro, Brendan Gregg, bpf,
Alexei Starovoitov, Yonghong Song, Andrii Nakryiko
On Mon, Jun 1, 2020 at 7:17 AM Wenbo Zhang <ethercflow@gmail.com> wrote:
>
> Hi Daniel,
>
> I find https://patchwork.ozlabs.org/project/netdev/patch/7464919bd9c15f2496ca29dceb6a4048b3199774.1576629200.git.ethercflow@gmail.com/
> this PR's current state is Awaiting Upstream. I don't know much about
> this state. I want to ask if this PR will be merged.
This one won't be merged.
Jiri had sent patches based on whitelist approach.
That's a proper direction to address locking concerns.
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH bpf-next v14 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname
2020-06-01 16:38 ` Alexei Starovoitov
@ 2020-06-02 3:04 ` Wenbo Zhang
2020-06-02 8:14 ` Jiri Olsa
0 siblings, 1 reply; 52+ messages in thread
From: Wenbo Zhang @ 2020-06-02 3:04 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Daniel Borkmann, Jiri Olsa, Al Viro, Brendan Gregg, bpf,
Alexei Starovoitov, Yonghong Song, Andrii Nakryiko
Get it, I'll search Jiri's patches to see how to use that. Thanks.
Alexei Starovoitov <alexei.starovoitov@gmail.com> 于2020年6月2日周二 上午12:38写道：
>
> On Mon, Jun 1, 2020 at 7:17 AM Wenbo Zhang <ethercflow@gmail.com> wrote:
> >
> > Hi Daniel,
> >
> > I find https://patchwork.ozlabs.org/project/netdev/patch/7464919bd9c15f2496ca29dceb6a4048b3199774.1576629200.git.ethercflow@gmail.com/
> > this PR's current state is Awaiting Upstream. I don't know much about
> > this state. I want to ask if this PR will be merged.
>
> This one won't be merged.
> Jiri had sent patches based on whitelist approach.
> That's a proper direction to address locking concerns.
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH bpf-next v14 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname
2020-06-02 3:04 ` Wenbo Zhang
@ 2020-06-02 8:14 ` Jiri Olsa
0 siblings, 0 replies; 52+ messages in thread
From: Jiri Olsa @ 2020-06-02 8:14 UTC (permalink / raw)
To: Wenbo Zhang
Cc: Alexei Starovoitov, Daniel Borkmann, Al Viro, Brendan Gregg, bpf,
Alexei Starovoitov, Yonghong Song, Andrii Nakryiko
On Tue, Jun 02, 2020 at 11:04:02AM +0800, Wenbo Zhang wrote:
> Get it, I'll search Jiri's patches to see how to use that. Thanks.
I'll cc you in the next post
jirka
>
> Alexei Starovoitov <alexei.starovoitov@gmail.com> 于2020年6月2日周二 上午12:38写道：
> >
> > On Mon, Jun 1, 2020 at 7:17 AM Wenbo Zhang <ethercflow@gmail.com> wrote:
> > >
> > > Hi Daniel,
> > >
> > > I find https://patchwork.ozlabs.org/project/netdev/patch/7464919bd9c15f2496ca29dceb6a4048b3199774.1576629200.git.ethercflow@gmail.com/
> > > this PR's current state is Awaiting Upstream. I don't know much about
> > > this state. I want to ask if this PR will be merged.
> >
> > This one won't be merged.
> > Jiri had sent patches based on whitelist approach.
> > That's a proper direction to address locking concerns.
>
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH bpf-next v14 2/2] selftests/bpf: test for bpf_get_fd_path() from tracepoint
2019-12-18 0:56 ` [PATCH bpf-next v14 0/2] bpf: adding get_fd_path helper Wenbo Zhang
2019-12-18 0:56 ` [PATCH bpf-next v14 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname Wenbo Zhang
@ 2019-12-18 0:56 ` Wenbo Zhang
2019-12-18 3:27 ` Yonghong Song
1 sibling, 1 reply; 52+ messages in thread
From: Wenbo Zhang @ 2019-12-18 0:56 UTC (permalink / raw)
To: bpf; +Cc: ast, daniel, yhs, bgregg, andrii.nakryiko, netdev
trace fstat events by tracepoint syscalls/sys_enter_newfstat, and handle
events only produced by test_file_fd_path, which call fstat on several
different types of files to test bpf_fd_file_path's feature.
v5->v6: addressed Gregg and Yonghong's feedback
- rename to get_fd_path
- change sys_enter_newfstat_args's fd type to long to fix issue on
big-endian machines
v4->v5: addressed Andrii's feedback
- pass NULL for opts as bpf_object__open_file's PARAM2, as not really
using any
- modify patch subject to keep up with test code
- as this test is single-threaded, so use getpid instead of SYS_gettid
- remove unnecessary parens around check which after if (i < 3)
- in kern use bpf_get_current_pid_tgid() >> 32 to fit getpid() in
userspace part
- with the patch adding helper as one patch series
v3->v4: addressed Andrii's feedback
- use a set of fd instead of fds array
- use global variables instead of maps (in v3, I mistakenly thought that
the bpf maps are global variables.)
- remove uncessary global variable path_info_index
- remove fd compare as the fstat's order is fixed
v2->v3: addressed Andrii's feedback
- use global data instead of perf_buffer to simplified code
v1->v2: addressed Daniel's feedback
- rename bpf_fd2path to bpf_get_file_path to be consistent with other
helper's names
Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
---
.../selftests/bpf/prog_tests/get_fd_path.c | 171 ++++++++++++++++++
.../selftests/bpf/progs/test_get_fd_path.c | 43 +++++
2 files changed, 214 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/get_fd_path.c
create mode 100644 tools/testing/selftests/bpf/progs/test_get_fd_path.c
diff --git a/tools/testing/selftests/bpf/prog_tests/get_fd_path.c b/tools/testing/selftests/bpf/prog_tests/get_fd_path.c
new file mode 100644
index 000000000000..2846f0a4e84b
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/get_fd_path.c
@@ -0,0 +1,171 @@
+// 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_FDS 7
+#define MAX_EVENT_NUM 16
+
+static struct fd_path_test_data {
+ pid_t pid;
+ __u32 cnt;
+ __u32 fds[MAX_EVENT_NUM];
+ char paths[MAX_EVENT_NUM][MAX_PATH_LEN];
+} src, dst;
+
+static int set_pathname(int fd)
+{
+ char buf[MAX_PATH_LEN];
+
+ snprintf(buf, MAX_PATH_LEN, "/proc/%d/fd/%d", src.pid, fd);
+ src.fds[src.cnt] = fd;
+ return readlink(buf, src.paths[src.cnt++], MAX_PATH_LEN);
+}
+
+static int trigger_fstat_events(pid_t pid)
+{
+ int pipefd[2] = { -1, -1 };
+ int sockfd = -1, procfd = -1, devfd = -1;
+ int localfd = -1, indicatorfd = -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/fd2path_loadgen.txt", O_CREAT | O_RDONLY);
+ if (CHECK_FAIL(localfd < 0))
+ goto out_close;
+ /* bpf_get_fd_path will return path with (deleted) */
+ remove("/tmp/fd2path_loadgen.txt");
+ indicatorfd = open("/tmp/", O_PATH);
+ if (CHECK_FAIL(indicatorfd < 0))
+ goto out_close;
+
+ src.pid = pid;
+
+ ret = set_pathname(pipefd[0]);
+ if (CHECK_FAIL(ret < 0))
+ goto out_close;
+ ret = set_pathname(pipefd[1]);
+ if (CHECK_FAIL(ret < 0))
+ goto out_close;
+ ret = set_pathname(sockfd);
+ if (CHECK_FAIL(ret < 0))
+ goto out_close;
+ ret = set_pathname(procfd);
+ if (CHECK_FAIL(ret < 0))
+ goto out_close;
+ ret = set_pathname(devfd);
+ if (CHECK_FAIL(ret < 0))
+ goto out_close;
+ ret = set_pathname(localfd);
+ if (CHECK_FAIL(ret < 0))
+ goto out_close;
+ ret = set_pathname(indicatorfd);
+ if (CHECK_FAIL(ret < 0))
+ goto out_close;
+
+ 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:
+ close(indicatorfd);
+ close(localfd);
+ close(devfd);
+ close(procfd);
+ close(sockfd);
+ close(pipefd[1]);
+ close(pipefd[0]);
+
+ return ret;
+}
+
+void test_get_fd_path(void)
+{
+ const char *prog_name = "tracepoint/syscalls/sys_enter_newfstat";
+ const char *obj_file = "test_get_fd_path.o";
+ int err, results_map_fd, duration = 0;
+ struct bpf_program *tp_prog = NULL;
+ struct bpf_link *tp_link = 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;
+
+ tp_prog = bpf_object__find_program_by_title(obj, prog_name);
+ if (CHECK(!tp_prog, "find_tp",
+ "prog '%s' not found\n", prog_name))
+ goto cleanup;
+
+ err = bpf_object__load(obj);
+ if (CHECK(err, "obj_load", "err %d\n", err))
+ goto cleanup;
+
+ results_map_fd = bpf_find_map(__func__, obj, "test_get.bss");
+ if (CHECK(results_map_fd < 0, "find_bss_map",
+ "err %d\n", results_map_fd))
+ goto cleanup;
+
+ tp_link = bpf_program__attach_tracepoint(tp_prog, "syscalls",
+ "sys_enter_newfstat");
+ if (CHECK(IS_ERR(tp_link), "attach_tp",
+ "err %ld\n", PTR_ERR(tp_link))) {
+ tp_link = NULL;
+ 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_FDS; i++) {
+ if (i < 3)
+ CHECK((dst.paths[i][0] != 0), "get_fd_path",
+ "failed to filter fs [%d]: %u(%s) vs %u(%s)\n",
+ i, src.fds[i], src.paths[i], dst.fds[i],
+ dst.paths[i]);
+ else
+ CHECK(strncmp(src.paths[i], dst.paths[i], MAX_PATH_LEN),
+ "get_fd_path",
+ "failed to get path[%d]: %u(%s) vs %u(%s)\n",
+ i, src.fds[i], src.paths[i], dst.fds[i],
+ dst.paths[i]);
+ }
+
+cleanup:
+ bpf_link__destroy(tp_link);
+ bpf_object__close(obj);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_get_fd_path.c b/tools/testing/selftests/bpf/progs/test_get_fd_path.c
new file mode 100644
index 000000000000..8bb58f87755e
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_get_fd_path.c
@@ -0,0 +1,43 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include <linux/ptrace.h>
+#include <string.h>
+#include <unistd.h>
+#include "bpf_helpers.h"
+#include "bpf_tracing.h"
+
+#define MAX_PATH_LEN 128
+#define MAX_EVENT_NUM 16
+
+static struct fd_path_test_data {
+ pid_t pid;
+ __u32 cnt;
+ __u32 fds[MAX_EVENT_NUM];
+ char paths[MAX_EVENT_NUM][MAX_PATH_LEN];
+} data;
+
+struct sys_enter_newfstat_args {
+ unsigned long long pad1;
+ unsigned long long pad2;
+ unsigned long fd;
+};
+
+SEC("tracepoint/syscalls/sys_enter_newfstat")
+int bpf_prog(struct sys_enter_newfstat_args *args)
+{
+ pid_t pid = bpf_get_current_pid_tgid() >> 32;
+
+ if (pid != data.pid)
+ return 0;
+ if (data.cnt >= MAX_EVENT_NUM)
+ return 0;
+
+ data.fds[data.cnt] = args->fd;
+ bpf_get_fd_path(data.paths[data.cnt], MAX_PATH_LEN, args->fd);
+ data.cnt++;
+
+ return 0;
+}
+
+char _license[] SEC("license") = "GPL";
--
2.17.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH bpf-next v14 2/2] selftests/bpf: test for bpf_get_fd_path() from tracepoint
2019-12-18 0:56 ` [PATCH bpf-next v14 2/2] selftests/bpf: test for bpf_get_fd_path() from tracepoint Wenbo Zhang
@ 2019-12-18 3:27 ` Yonghong Song
0 siblings, 0 replies; 52+ messages in thread
From: Yonghong Song @ 2019-12-18 3:27 UTC (permalink / raw)
To: Wenbo Zhang, bpf; +Cc: ast, daniel, bgregg, andrii.nakryiko, netdev
On 12/17/19 4:56 PM, Wenbo Zhang wrote:
> trace fstat events by tracepoint syscalls/sys_enter_newfstat, and handle
> events only produced by test_file_fd_path, which call fstat on several
> different types of files to test bpf_fd_file_path's feature.
>
> v5->v6: addressed Gregg and Yonghong's feedback
> - rename to get_fd_path
> - change sys_enter_newfstat_args's fd type to long to fix issue on
> big-endian machines
>
> v4->v5: addressed Andrii's feedback
> - pass NULL for opts as bpf_object__open_file's PARAM2, as not really
> using any
> - modify patch subject to keep up with test code
> - as this test is single-threaded, so use getpid instead of SYS_gettid
> - remove unnecessary parens around check which after if (i < 3)
> - in kern use bpf_get_current_pid_tgid() >> 32 to fit getpid() in
> userspace part
> - with the patch adding helper as one patch series
>
> v3->v4: addressed Andrii's feedback
> - use a set of fd instead of fds array
> - use global variables instead of maps (in v3, I mistakenly thought that
> the bpf maps are global variables.)
> - remove uncessary global variable path_info_index
> - remove fd compare as the fstat's order is fixed
>
> v2->v3: addressed Andrii's feedback
> - use global data instead of perf_buffer to simplified code
>
> v1->v2: addressed Daniel's feedback
> - rename bpf_fd2path to bpf_get_file_path to be consistent with other
> helper's names
>
> Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
Acked-by: Yonghong Song <yhs@fb.com>
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH bpf-next v13 2/2] selftests/bpf: test for bpf_get_fd_path() from tracepoint
2019-12-17 9:47 ` [PATCH bpf-next v13 0/2] bpf: adding get_fd_path helper Wenbo Zhang
2019-12-17 9:47 ` [PATCH bpf-next v13 1/2] bpf: add new helper get_fd_path for mapping a file descriptor to a pathname Wenbo Zhang
@ 2019-12-17 9:47 ` Wenbo Zhang
2019-12-17 16:32 ` Yonghong Song
1 sibling, 1 reply; 52+ messages in thread
From: Wenbo Zhang @ 2019-12-17 9:47 UTC (permalink / raw)
To: bpf; +Cc: ast, daniel, yhs, bgregg, andrii.nakryiko, netdev
trace fstat events by tracepoint syscalls/sys_enter_newfstat, and handle
events only produced by test_file_fd_path, which call fstat on several
different types of files to test bpf_fd_file_path's feature.
v5->v6: addressed Gregg and Yonghong's feedback
- rename to get_fd_path
- change sys_enter_newfstat_args's fd type to long to fix issue on
big-endian machines
v4->v5: addressed Andrii's feedback
- pass NULL for opts as bpf_object__open_file's PARAM2, as not really
using any
- modify patch subject to keep up with test code
- as this test is single-threaded, so use getpid instead of SYS_gettid
- remove unnecessary parens around check which after if (i < 3)
- in kern use bpf_get_current_pid_tgid() >> 32 to fit getpid() in
userspace part
- with the patch adding helper as one patch series
v3->v4: addressed Andrii's feedback
- use a set of fd instead of fds array
- use global variables instead of maps (in v3, I mistakenly thought that
the bpf maps are global variables.)
- remove uncessary global variable path_info_index
- remove fd compare as the fstat's order is fixed
v2->v3: addressed Andrii's feedback
- use global data instead of perf_buffer to simplified code
v1->v2: addressed Daniel's feedback
- rename bpf_fd2path to bpf_get_file_path to be consistent with other
helper's names
Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
---
.../selftests/bpf/prog_tests/get_fd_path.c | 171 ++++++++++++++++++
.../selftests/bpf/progs/test_get_fd_path.c | 43 +++++
2 files changed, 214 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/get_fd_path.c
create mode 100644 tools/testing/selftests/bpf/progs/test_get_fd_path.c
diff --git a/tools/testing/selftests/bpf/prog_tests/get_fd_path.c b/tools/testing/selftests/bpf/prog_tests/get_fd_path.c
new file mode 100644
index 000000000000..5cf58028a8d2
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/get_fd_path.c
@@ -0,0 +1,171 @@
+// 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_FDS 7
+#define MAX_EVENT_NUM 16
+
+static struct fd_path_test_data {
+ pid_t pid;
+ __u32 cnt;
+ __u32 fds[MAX_EVENT_NUM];
+ char paths[MAX_EVENT_NUM][MAX_PATH_LEN];
+} src, dst;
+
+static int set_pathname(int fd)
+{
+ char buf[MAX_PATH_LEN];
+
+ snprintf(buf, MAX_PATH_LEN, "/proc/%d/fd/%d", src.pid, fd);
+ src.fds[src.cnt] = fd;
+ return readlink(buf, src.paths[src.cnt++], MAX_PATH_LEN);
+}
+
+static int trigger_fstat_events(pid_t pid)
+{
+ int pipefd[2] = { -1, -1 };
+ int sockfd = -1, procfd = -1, devfd = -1;
+ int localfd = -1, indicatorfd = -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/fd2path_loadgen.txt", O_CREAT | O_RDONLY);
+ if (CHECK_FAIL(localfd < 0))
+ goto out_close;
+ /* bpf_get_fd_path will return path with (deleted) */
+ remove("/tmp/fd2path_loadgen.txt");
+ indicatorfd = open("/tmp/", O_PATH);
+ if (CHECK_FAIL(indicatorfd < 0))
+ goto out_close;
+
+ src.pid = pid;
+
+ ret = set_pathname(pipefd[0]);
+ if (CHECK_FAIL(ret < 0))
+ goto out_close;
+ ret = set_pathname(pipefd[1]);
+ if (CHECK_FAIL(ret < 0))
+ goto out_close;
+ ret = set_pathname(sockfd);
+ if (CHECK_FAIL(ret < 0))
+ goto out_close;
+ ret = set_pathname(procfd);
+ if (CHECK_FAIL(ret < 0))
+ goto out_close;
+ ret = set_pathname(devfd);
+ if (CHECK_FAIL(ret < 0))
+ goto out_close;
+ ret = set_pathname(localfd);
+ if (CHECK_FAIL(ret < 0))
+ goto out_close;
+ ret = set_pathname(indicatorfd);
+ if (CHECK_FAIL(ret < 0))
+ goto out_close;
+
+ 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:
+ close(indicatorfd);
+ close(localfd);
+ close(devfd);
+ close(procfd);
+ close(sockfd);
+ close(pipefd[1]);
+ close(pipefd[0]);
+
+ return ret;
+}
+
+void test_get_fd_path(void)
+{
+ const char *prog_name = "tracepoint/syscalls/sys_enter_newfstat";
+ const char *obj_file = "test_get_fd_path.o";
+ int err, results_map_fd, duration = 0;
+ struct bpf_program *tp_prog = NULL;
+ struct bpf_link *tp_link = 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;
+
+ tp_prog = bpf_object__find_program_by_title(obj, prog_name);
+ if (CHECK(!tp_prog, "find_tp",
+ "prog '%s' not found\n", prog_name))
+ goto cleanup;
+
+ err = bpf_object__load(obj);
+ if (CHECK(err, "obj_load", "err %d\n", err))
+ goto cleanup;
+
+ results_map_fd = bpf_find_map(__func__, obj, "test_get.bss");
+ if (CHECK(results_map_fd < 0, "find_bss_map",
+ "err %d\n", results_map_fd))
+ goto cleanup;
+
+ tp_link = bpf_program__attach_tracepoint(tp_prog, "syscalls",
+ "sys_enter_newfstat");
+ if (CHECK(IS_ERR(tp_link), "attach_tp",
+ "err %ld\n", PTR_ERR(tp_link))) {
+ tp_link = NULL;
+ 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_FDS; i++) {
+ if (i < 3)
+ CHECK((dst.paths[i][0] != '0'), "get_fd_path",
+ "failed to filter fs [%d]: %u(%s) vs %u(%s)\n",
+ i, src.fds[i], src.paths[i], dst.fds[i],
+ dst.paths[i]);
+ else
+ CHECK(strncmp(src.paths[i], dst.paths[i], MAX_PATH_LEN),
+ "get_fd_path",
+ "failed to get path[%d]: %u(%s) vs %u(%s)\n",
+ i, src.fds[i], src.paths[i], dst.fds[i],
+ dst.paths[i]);
+ }
+
+cleanup:
+ bpf_link__destroy(tp_link);
+ bpf_object__close(obj);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_get_fd_path.c b/tools/testing/selftests/bpf/progs/test_get_fd_path.c
new file mode 100644
index 000000000000..8bb58f87755e
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_get_fd_path.c
@@ -0,0 +1,43 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include <linux/ptrace.h>
+#include <string.h>
+#include <unistd.h>
+#include "bpf_helpers.h"
+#include "bpf_tracing.h"
+
+#define MAX_PATH_LEN 128
+#define MAX_EVENT_NUM 16
+
+static struct fd_path_test_data {
+ pid_t pid;
+ __u32 cnt;
+ __u32 fds[MAX_EVENT_NUM];
+ char paths[MAX_EVENT_NUM][MAX_PATH_LEN];
+} data;
+
+struct sys_enter_newfstat_args {
+ unsigned long long pad1;
+ unsigned long long pad2;
+ unsigned long fd;
+};
+
+SEC("tracepoint/syscalls/sys_enter_newfstat")
+int bpf_prog(struct sys_enter_newfstat_args *args)
+{
+ pid_t pid = bpf_get_current_pid_tgid() >> 32;
+
+ if (pid != data.pid)
+ return 0;
+ if (data.cnt >= MAX_EVENT_NUM)
+ return 0;
+
+ data.fds[data.cnt] = args->fd;
+ bpf_get_fd_path(data.paths[data.cnt], MAX_PATH_LEN, args->fd);
+ data.cnt++;
+
+ return 0;
+}
+
+char _license[] SEC("license") = "GPL";
--
2.17.1
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH bpf-next v13 2/2] selftests/bpf: test for bpf_get_fd_path() from tracepoint
2019-12-17 9:47 ` [PATCH bpf-next v13 " Wenbo Zhang
@ 2019-12-17 16:32 ` Yonghong Song
0 siblings, 0 replies; 52+ messages in thread
From: Yonghong Song @ 2019-12-17 16:32 UTC (permalink / raw)
To: Wenbo Zhang, bpf; +Cc: ast, daniel, bgregg, andrii.nakryiko, netdev
On 12/17/19 1:47 AM, Wenbo Zhang wrote:
> trace fstat events by tracepoint syscalls/sys_enter_newfstat, and handle
> events only produced by test_file_fd_path, which call fstat on several
> different types of files to test bpf_fd_file_path's feature.
>
> v5->v6: addressed Gregg and Yonghong's feedback
> - rename to get_fd_path
> - change sys_enter_newfstat_args's fd type to long to fix issue on
> big-endian machines
>
> v4->v5: addressed Andrii's feedback
> - pass NULL for opts as bpf_object__open_file's PARAM2, as not really
> using any
> - modify patch subject to keep up with test code
> - as this test is single-threaded, so use getpid instead of SYS_gettid
> - remove unnecessary parens around check which after if (i < 3)
> - in kern use bpf_get_current_pid_tgid() >> 32 to fit getpid() in
> userspace part
> - with the patch adding helper as one patch series
>
> v3->v4: addressed Andrii's feedback
> - use a set of fd instead of fds array
> - use global variables instead of maps (in v3, I mistakenly thought that
> the bpf maps are global variables.)
> - remove uncessary global variable path_info_index
> - remove fd compare as the fstat's order is fixed
>
> v2->v3: addressed Andrii's feedback
> - use global data instead of perf_buffer to simplified code
>
> v1->v2: addressed Daniel's feedback
> - rename bpf_fd2path to bpf_get_file_path to be consistent with other
> helper's names
>
> Signed-off-by: Wenbo Zhang <ethercflow@gmail.com>
Acked-by: Yonghong Song <yhs@fb.com>
^ permalink raw reply [flat|nested] 52+ messages in thread