[PATCH v2 bpf-next 1/2] bpf: include sub program tags in bpf_prog_info

[PATCH v2 bpf-next 1/2] bpf: include sub program tags in bpf_prog_info

[Song Liu]

Changes from v1:
1. Fix error path as Martin suggested.

This patch adds nr_prog_tags and prog_tags to bpf_prog_info. This is a
reliable way for user space to get tags of all sub programs. Before this
patch, user space need to find sub program tags via kallsyms.

This feature will be used in BPF introspection, where user space queries
information about BPF programs via sys_bpf.

Signed-off-by: Song Liu <songliubraving@fb.com>
---
 include/uapi/linux/bpf.h |  2 ++
 kernel/bpf/syscall.c     | 27 +++++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 1bee1135866a..368d185aa32f 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -2703,6 +2703,8 @@ struct bpf_prog_info {
 	__u32 jited_line_info_cnt;
 	__u32 line_info_rec_size;
 	__u32 jited_line_info_rec_size;
+	__u32 nr_prog_tags;
+	__aligned_u64 prog_tags;
 } __attribute__((aligned(8)));

 struct bpf_map_info {
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 19c88cff7880..49cb59177db9 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2322,6 +2322,33 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
 		}
 	}

+	ulen = info.nr_prog_tags;
+	info.nr_prog_tags = prog->aux->func_cnt ? : 1;
+	if (ulen) {
+		if (bpf_dump_raw_ok()) {
+			__u8 __user (*user_prog_tags)[BPF_TAG_SIZE];
+			u32 i;
+
+			user_prog_tags = u64_to_user_ptr(info.prog_tags);
+			ulen = min_t(u32, info.nr_prog_tags, ulen);
+			if (prog->aux->func_cnt) {
+				for (i = 0; i < ulen; i++) {
+					if (copy_to_user(
+						    user_prog_tags[i],
+						    prog->aux->func[i]->tag,
+						    BPF_TAG_SIZE))
+						return -EFAULT;
+				}
+			} else {
+				if (copy_to_user(user_prog_tags[0],
+						 prog->tag, BPF_TAG_SIZE))
+					return -EFAULT;
+			}
+		} else {
+			info.prog_tags = 0;
+		}
+	}
+
 done:
 	if (copy_to_user(uinfo, &info, info_len) ||
 	    put_user(info_len, &uattr->info.info_len))

[PATCH v2 bpf-next 2/2] bpf: sync tools/include/uapi/linux/bpf.h

[Song Liu]

Sync bpf.h for nr_prog_tags and prog_tags.

Signed-off-by: Song Liu <songliubraving@fb.com>
---
 tools/include/uapi/linux/bpf.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 6ad50b6471d3..7b04fd8d89f9 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -2702,6 +2702,8 @@ struct bpf_prog_info {
 	__u32 jited_line_info_cnt;
 	__u32 line_info_rec_size;
 	__u32 jited_line_info_rec_size;
+	__u32 nr_prog_tags;
+	__aligned_u64 prog_tags;
 } __attribute__((aligned(8)));
 
 struct bpf_map_info {
-- 
2.17.1

Re: [PATCH v2 bpf-next 1/2] bpf: include sub program tags in bpf_prog_info

[Daniel Borkmann]

On 12/11/2018 09:18 AM, Song Liu wrote:
> Changes from v1:
> 1. Fix error path as Martin suggested.
> 
> This patch adds nr_prog_tags and prog_tags to bpf_prog_info. This is a
> reliable way for user space to get tags of all sub programs. Before this
> patch, user space need to find sub program tags via kallsyms.
> 
> This feature will be used in BPF introspection, where user space queries
> information about BPF programs via sys_bpf.

Should bpftool be updated as well to support this facility?

Thanks,
Daniel

Re: [PATCH v2 bpf-next 1/2] bpf: include sub program tags in bpf_prog_info

[Song Liu]

> On Dec 11, 2018, at 7:54 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> 
> On 12/11/2018 09:18 AM, Song Liu wrote:
>> Changes from v1:
>> 1. Fix error path as Martin suggested.
>> 
>> This patch adds nr_prog_tags and prog_tags to bpf_prog_info. This is a
>> reliable way for user space to get tags of all sub programs. Before this
>> patch, user space need to find sub program tags via kallsyms.
>> 
>> This feature will be used in BPF introspection, where user space queries
>> information about BPF programs via sys_bpf.
> 
> Should bpftool be updated as well to support this facility?
> 
> Thanks,
> Daniel

Currently, bpftool gets sub program tags from kallsyms (kernel_syms_load, 
kernel_syms_search). So it doesn't need this urgently. In longer term, 
we may replace kernel_syms_* functions with information from tag and BTF. 

Thanks,
Song

Re: [PATCH v2 bpf-next 1/2] bpf: include sub program tags in bpf_prog_info

[Daniel Borkmann]

On 12/11/2018 09:18 AM, Song Liu wrote:
> Changes from v1:
> 1. Fix error path as Martin suggested.
> 
> This patch adds nr_prog_tags and prog_tags to bpf_prog_info. This is a
> reliable way for user space to get tags of all sub programs. Before this
> patch, user space need to find sub program tags via kallsyms.
> 
> This feature will be used in BPF introspection, where user space queries
> information about BPF programs via sys_bpf.
> 
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---
>  include/uapi/linux/bpf.h |  2 ++
>  kernel/bpf/syscall.c     | 27 +++++++++++++++++++++++++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 1bee1135866a..368d185aa32f 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -2703,6 +2703,8 @@ struct bpf_prog_info {
>  	__u32 jited_line_info_cnt;
>  	__u32 line_info_rec_size;
>  	__u32 jited_line_info_rec_size;
> +	__u32 nr_prog_tags;
> +	__aligned_u64 prog_tags;
>  } __attribute__((aligned(8)));
> 
>  struct bpf_map_info {
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 19c88cff7880..49cb59177db9 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -2322,6 +2322,33 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
>  		}
>  	}
> 
> +	ulen = info.nr_prog_tags;
> +	info.nr_prog_tags = prog->aux->func_cnt ? : 1;
> +	if (ulen) {
> +		if (bpf_dump_raw_ok()) {

Hm, why is bpf_dump_raw_ok() needed here? tag is not exposing a kernel
address. prog tag is in general also visible from fdinfo from unpriv.

Just looking at the recently merged func_info / line_info I'm not sure
this is needed there either ... Martin, is there a specific reason this
was added there as well?

> +			__u8 __user (*user_prog_tags)[BPF_TAG_SIZE];
> +			u32 i;
> +
> +			user_prog_tags = u64_to_user_ptr(info.prog_tags);
> +			ulen = min_t(u32, info.nr_prog_tags, ulen);
> +			if (prog->aux->func_cnt) {
> +				for (i = 0; i < ulen; i++) {
> +					if (copy_to_user(
> +						    user_prog_tags[i],
> +						    prog->aux->func[i]->tag,
> +						    BPF_TAG_SIZE))
> +						return -EFAULT;
> +				}
> +			} else {
> +				if (copy_to_user(user_prog_tags[0],
> +						 prog->tag, BPF_TAG_SIZE))
> +					return -EFAULT;
> +			}
> +		} else {
> +			info.prog_tags = 0;
> +		}
> +	}
> +
>  done:
>  	if (copy_to_user(uinfo, &info, info_len) ||
>  	    put_user(info_len, &uattr->info.info_len))
> --
> 2.17.1
> 

Re: [PATCH v2 bpf-next 1/2] bpf: include sub program tags in bpf_prog_info

[Martin Lau]

On Wed, Dec 12, 2018 at 03:22:38AM +0100, Daniel Borkmann wrote:
> On 12/11/2018 09:18 AM, Song Liu wrote:
> > Changes from v1:
> > 1. Fix error path as Martin suggested.
> > 
> > This patch adds nr_prog_tags and prog_tags to bpf_prog_info. This is a
> > reliable way for user space to get tags of all sub programs. Before this
> > patch, user space need to find sub program tags via kallsyms.
> > 
> > This feature will be used in BPF introspection, where user space queries
> > information about BPF programs via sys_bpf.
> > 
> > Signed-off-by: Song Liu <songliubraving@fb.com>
> > ---
> >  include/uapi/linux/bpf.h |  2 ++
> >  kernel/bpf/syscall.c     | 27 +++++++++++++++++++++++++++
> >  2 files changed, 29 insertions(+)
> > 
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 1bee1135866a..368d185aa32f 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -2703,6 +2703,8 @@ struct bpf_prog_info {
> >  	__u32 jited_line_info_cnt;
> >  	__u32 line_info_rec_size;
> >  	__u32 jited_line_info_rec_size;
> > +	__u32 nr_prog_tags;
> > +	__aligned_u64 prog_tags;
> >  } __attribute__((aligned(8)));
> > 
> >  struct bpf_map_info {
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index 19c88cff7880..49cb59177db9 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -2322,6 +2322,33 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
> >  		}
> >  	}
> > 
> > +	ulen = info.nr_prog_tags;
> > +	info.nr_prog_tags = prog->aux->func_cnt ? : 1;
> > +	if (ulen) {
> > +		if (bpf_dump_raw_ok()) {
> 
> Hm, why is bpf_dump_raw_ok() needed here? tag is not exposing a kernel
> address. prog tag is in general also visible from fdinfo from unpriv.
> 
> Just looking at the recently merged func_info / line_info I'm not sure
> this is needed there either ... Martin, is there a specific reason this
> was added there as well?
It is mostly to follow info.jited_func_lens which also tests
bpf_dump_raw_ok().  We thought the check iss there even it is
not exposing the kernel address because jited_func_lens is not
useful in general without jited_ksyms.

Same go for func_info when dumping jited insn.  func_info used to only
support jited insn dump.

Considering func_info was later made to support xlated insn also,
I think it makes sense to remove the bpf_dump_raw_ok() check
for func_info, line_info and prog_tags and ask the
userspace to decide if it has all needed details before dumping
info for the xlated insn and jited insn?

The bpf_dump_raw_ok() check for jited_line_info will stay
because jited_line_info contains kernel address.

> > +			__u8 __user (*user_prog_tags)[BPF_TAG_SIZE];
> > +			u32 i;
> > +
> > +			user_prog_tags = u64_to_user_ptr(info.prog_tags);
> > +			ulen = min_t(u32, info.nr_prog_tags, ulen);
> > +			if (prog->aux->func_cnt) {
> > +				for (i = 0; i < ulen; i++) {
> > +					if (copy_to_user(
> > +						    user_prog_tags[i],
> > +						    prog->aux->func[i]->tag,
> > +						    BPF_TAG_SIZE))
> > +						return -EFAULT;
> > +				}
> > +			} else {
> > +				if (copy_to_user(user_prog_tags[0],
> > +						 prog->tag, BPF_TAG_SIZE))
> > +					return -EFAULT;
> > +			}
> > +		} else {
> > +			info.prog_tags = 0;
> > +		}
> > +	}
> > +
> >  done:
> >  	if (copy_to_user(uinfo, &info, info_len) ||
> >  	    put_user(info_len, &uattr->info.info_len))
> > --
> > 2.17.1
> > 
> 

Re: [PATCH v2 bpf-next 1/2] bpf: include sub program tags in bpf_prog_info

[Daniel Borkmann]

On 12/12/2018 05:38 AM, Martin Lau wrote:
> On Wed, Dec 12, 2018 at 03:22:38AM +0100, Daniel Borkmann wrote:
>> On 12/11/2018 09:18 AM, Song Liu wrote:
>>> Changes from v1:
>>> 1. Fix error path as Martin suggested.
>>>
>>> This patch adds nr_prog_tags and prog_tags to bpf_prog_info. This is a
>>> reliable way for user space to get tags of all sub programs. Before this
>>> patch, user space need to find sub program tags via kallsyms.
>>>
>>> This feature will be used in BPF introspection, where user space queries
>>> information about BPF programs via sys_bpf.
>>>
>>> Signed-off-by: Song Liu <songliubraving@fb.com>
>>> ---
>>>  include/uapi/linux/bpf.h |  2 ++
>>>  kernel/bpf/syscall.c     | 27 +++++++++++++++++++++++++++
>>>  2 files changed, 29 insertions(+)
>>>
>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>> index 1bee1135866a..368d185aa32f 100644
>>> --- a/include/uapi/linux/bpf.h
>>> +++ b/include/uapi/linux/bpf.h
>>> @@ -2703,6 +2703,8 @@ struct bpf_prog_info {
>>>  	__u32 jited_line_info_cnt;
>>>  	__u32 line_info_rec_size;
>>>  	__u32 jited_line_info_rec_size;
>>> +	__u32 nr_prog_tags;
>>> +	__aligned_u64 prog_tags;
>>>  } __attribute__((aligned(8)));
>>>
>>>  struct bpf_map_info {
>>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>>> index 19c88cff7880..49cb59177db9 100644
>>> --- a/kernel/bpf/syscall.c
>>> +++ b/kernel/bpf/syscall.c
>>> @@ -2322,6 +2322,33 @@ static int bpf_prog_get_info_by_fd(struct bpf_prog *prog,
>>>  		}
>>>  	}
>>>
>>> +	ulen = info.nr_prog_tags;
>>> +	info.nr_prog_tags = prog->aux->func_cnt ? : 1;
>>> +	if (ulen) {
>>> +		if (bpf_dump_raw_ok()) {
>>
>> Hm, why is bpf_dump_raw_ok() needed here? tag is not exposing a kernel
>> address. prog tag is in general also visible from fdinfo from unpriv.
>>
>> Just looking at the recently merged func_info / line_info I'm not sure
>> this is needed there either ... Martin, is there a specific reason this
>> was added there as well?
> It is mostly to follow info.jited_func_lens which also tests
> bpf_dump_raw_ok().  We thought the check iss there even it is
> not exposing the kernel address because jited_func_lens is not
> useful in general without jited_ksyms.

Yes, agree, makes sense to me there.

> Same go for func_info when dumping jited insn.  func_info used to only
> support jited insn dump.

Ok, sounds reasonable for the JIT case. (In the xlated one we just zero
the addresses through bpf_insn_prepare_dump() so the dump is available
also on !bpf_dump_raw_ok().)

> Considering func_info was later made to support xlated insn also,
> I think it makes sense to remove the bpf_dump_raw_ok() check
> for func_info, line_info and prog_tags and ask the

Yeah lets remove it from there.

> userspace to decide if it has all needed details before dumping
> info for the xlated insn and jited insn?

E.g. in case of !bpf_dump_raw_ok(), user space should still be able to
do the C / line-info annotation for the xlated insns. If it finds that
for JIT it cannot dump anything, we could print an error to the user in
bpftool telling that if the information is wanted nevertheless then the
kptr_restrict setting needs to be adapted.

I think the sub-prog_tags and func_info could still be useful e.g. in
bpftool case for dumping all kernel symbols related to a specific
prog-id, for example, or if we add a new mode in bpftool to dump all
tags from a user provided object file, so they can be correlated with
the one active in the kernel.

> The bpf_dump_raw_ok() check for jited_line_info will stay
> because jited_line_info contains kernel address.

+1

Thanks,
Daniel