* Re: [PATCH net-next] ebpf: move CONFIG_BPF_SYSCALL-only function declarations
@ 2015-03-02 17:35 Alexei Starovoitov
2015-03-02 17:41 ` Daniel Borkmann
2015-03-03 3:12 ` Masami Hiramatsu
0 siblings, 2 replies; 6+ messages in thread
From: Alexei Starovoitov @ 2015-03-02 17:35 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: David S. Miller, Masami Hiramatsu, Network Development
On Mon, Mar 2, 2015 at 6:21 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> Masami noted that it would be better to hide the remaining CONFIG_BPF_SYSCALL-only
> function declarations within the BPF header ifdef, w/o else path dummy alternatives
> since these functions are not supposed to have a user outside of CONFIG_BPF_SYSCALL.
So far we didn't have anyone trying to add new map types
outside of kernel/bpf/, so this patch is a defensive move.
Such potential future abuser will get compile error for
missing bpf_register_map_type() instead of linker error for the same.
Not sure that's really needed.
Also bpf_map_put() and bpf_map_get() are only used
by bpf syscall and verifier. imo moving them under ifdef is overkill.
I think ifdef should only be used for function that have
real and dummy bodies.
Today we have three:
bpf_register_prog_type(), bpf_prog_get() and bpf_prog_put()
and that makes sense.
Hiding *map*() functions seems unnecessary.
I think linker error is good enough.
> Suggested-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Reference: http://article.gmane.org/gmane.linux.kernel.api/8658
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
> include/linux/bpf.h | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index a1a7ff2..a884f5a 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -42,10 +42,6 @@ struct bpf_map_type_list {
> enum bpf_map_type type;
> };
>
> -void bpf_register_map_type(struct bpf_map_type_list *tl);
> -void bpf_map_put(struct bpf_map *map);
> -struct bpf_map *bpf_map_get(struct fd f);
> -
> /* function argument constraints */
> enum bpf_arg_type {
> ARG_ANYTHING = 0, /* any argument is ok */
> @@ -126,9 +122,16 @@ struct bpf_prog_aux {
>
> #ifdef CONFIG_BPF_SYSCALL
> void bpf_register_prog_type(struct bpf_prog_type_list *tl);
> +void bpf_register_map_type(struct bpf_map_type_list *tl);
>
> -void bpf_prog_put(struct bpf_prog *prog);
> struct bpf_prog *bpf_prog_get(u32 ufd);
> +void bpf_prog_put(struct bpf_prog *prog);
> +
> +struct bpf_map *bpf_map_get(struct fd f);
> +void bpf_map_put(struct bpf_map *map);
> +
> +/* verify correctness of eBPF program */
> +int bpf_check(struct bpf_prog *fp, union bpf_attr *attr);
> #else
> static inline void bpf_register_prog_type(struct bpf_prog_type_list *tl)
> {
> @@ -142,10 +145,7 @@ static inline struct bpf_prog *bpf_prog_get(u32 ufd)
> static inline void bpf_prog_put(struct bpf_prog *prog)
> {
> }
> -#endif
> -
> -/* verify correctness of eBPF program */
> -int bpf_check(struct bpf_prog *fp, union bpf_attr *attr);
> +#endif /* CONFIG_BPF_SYSCALL */
>
> /* verifier prototypes for helper functions called from eBPF programs */
> extern const struct bpf_func_proto bpf_map_lookup_elem_proto;
> --
> 1.9.3
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net-next] ebpf: move CONFIG_BPF_SYSCALL-only function declarations
2015-03-02 17:35 [PATCH net-next] ebpf: move CONFIG_BPF_SYSCALL-only function declarations Alexei Starovoitov
@ 2015-03-02 17:41 ` Daniel Borkmann
2015-03-03 3:12 ` Masami Hiramatsu
1 sibling, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2015-03-02 17:41 UTC (permalink / raw)
To: Alexei Starovoitov; +Cc: David S. Miller, Masami Hiramatsu, Network Development
On 03/02/2015 06:35 PM, Alexei Starovoitov wrote:
> On Mon, Mar 2, 2015 at 6:21 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> Masami noted that it would be better to hide the remaining CONFIG_BPF_SYSCALL-only
>> function declarations within the BPF header ifdef, w/o else path dummy alternatives
>> since these functions are not supposed to have a user outside of CONFIG_BPF_SYSCALL.
>
> So far we didn't have anyone trying to add new map types
> outside of kernel/bpf/, so this patch is a defensive move.
> Such potential future abuser will get compile error for
> missing bpf_register_map_type() instead of linker error for the same.
> Not sure that's really needed.
> Also bpf_map_put() and bpf_map_get() are only used
> by bpf syscall and verifier. imo moving them under ifdef is overkill.
>
> I think ifdef should only be used for function that have
> real and dummy bodies.
> Today we have three:
> bpf_register_prog_type(), bpf_prog_get() and bpf_prog_put()
> and that makes sense.
>
> Hiding *map*() functions seems unnecessary.
> I think linker error is good enough.
I don't have a strong opinion here, we do similar hiding elsewhere,
e.g. in CONFIG_BPF_JIT. If it's for the sake that Masami can read the
code better, I'm okay with it, perhaps that will lead him to write
patches for eBPF code. ;)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re: [PATCH net-next] ebpf: move CONFIG_BPF_SYSCALL-only function declarations
2015-03-02 17:35 [PATCH net-next] ebpf: move CONFIG_BPF_SYSCALL-only function declarations Alexei Starovoitov
2015-03-02 17:41 ` Daniel Borkmann
@ 2015-03-03 3:12 ` Masami Hiramatsu
2015-03-03 3:15 ` Alexei Starovoitov
1 sibling, 1 reply; 6+ messages in thread
From: Masami Hiramatsu @ 2015-03-03 3:12 UTC (permalink / raw)
To: Alexei Starovoitov; +Cc: Daniel Borkmann, David S. Miller, Network Development
(2015/03/03 2:35), Alexei Starovoitov wrote:
> On Mon, Mar 2, 2015 at 6:21 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> Masami noted that it would be better to hide the remaining CONFIG_BPF_SYSCALL-only
>> function declarations within the BPF header ifdef, w/o else path dummy alternatives
>> since these functions are not supposed to have a user outside of CONFIG_BPF_SYSCALL.
>
> So far we didn't have anyone trying to add new map types
> outside of kernel/bpf/, so this patch is a defensive move.
> Such potential future abuser will get compile error for
> missing bpf_register_map_type() instead of linker error for the same.
> Not sure that's really needed.
> Also bpf_map_put() and bpf_map_get() are only used
> by bpf syscall and verifier. imo moving them under ifdef is overkill.
No, not overkill. This patch becomes good description about the code to
reader who learns the eBPF. It tells them what functions are provided
when the kconfig is set or not.
And compiler error is better since abuser would get it before
compiling all other object :)
> I think ifdef should only be used for function that have
> real and dummy bodies.
> Today we have three:
> bpf_register_prog_type(), bpf_prog_get() and bpf_prog_put()
> and that makes sense.
>
> Hiding *map*() functions seems unnecessary.
> I think linker error is good enough.
But, then, why we need to move ONLY bpf_prog* functions?
I just thought it was half-way.
Thank you,
>
>> Suggested-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>> Reference: http://article.gmane.org/gmane.linux.kernel.api/8658
>> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
>> ---
>> include/linux/bpf.h | 18 +++++++++---------
>> 1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index a1a7ff2..a884f5a 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -42,10 +42,6 @@ struct bpf_map_type_list {
>> enum bpf_map_type type;
>> };
>>
>> -void bpf_register_map_type(struct bpf_map_type_list *tl);
>> -void bpf_map_put(struct bpf_map *map);
>> -struct bpf_map *bpf_map_get(struct fd f);
>> -
>> /* function argument constraints */
>> enum bpf_arg_type {
>> ARG_ANYTHING = 0, /* any argument is ok */
>> @@ -126,9 +122,16 @@ struct bpf_prog_aux {
>>
>> #ifdef CONFIG_BPF_SYSCALL
>> void bpf_register_prog_type(struct bpf_prog_type_list *tl);
>> +void bpf_register_map_type(struct bpf_map_type_list *tl);
>>
>> -void bpf_prog_put(struct bpf_prog *prog);
>> struct bpf_prog *bpf_prog_get(u32 ufd);
>> +void bpf_prog_put(struct bpf_prog *prog);
>> +
>> +struct bpf_map *bpf_map_get(struct fd f);
>> +void bpf_map_put(struct bpf_map *map);
>> +
>> +/* verify correctness of eBPF program */
>> +int bpf_check(struct bpf_prog *fp, union bpf_attr *attr);
>> #else
>> static inline void bpf_register_prog_type(struct bpf_prog_type_list *tl)
>> {
>> @@ -142,10 +145,7 @@ static inline struct bpf_prog *bpf_prog_get(u32 ufd)
>> static inline void bpf_prog_put(struct bpf_prog *prog)
>> {
>> }
>> -#endif
>> -
>> -/* verify correctness of eBPF program */
>> -int bpf_check(struct bpf_prog *fp, union bpf_attr *attr);
>> +#endif /* CONFIG_BPF_SYSCALL */
>>
>> /* verifier prototypes for helper functions called from eBPF programs */
>> extern const struct bpf_func_proto bpf_map_lookup_elem_proto;
>> --
>> 1.9.3
>>
>
--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Re: [PATCH net-next] ebpf: move CONFIG_BPF_SYSCALL-only function declarations
2015-03-03 3:12 ` Masami Hiramatsu
@ 2015-03-03 3:15 ` Alexei Starovoitov
0 siblings, 0 replies; 6+ messages in thread
From: Alexei Starovoitov @ 2015-03-03 3:15 UTC (permalink / raw)
To: Masami Hiramatsu; +Cc: Daniel Borkmann, David S. Miller, Network Development
On Mon, Mar 2, 2015 at 7:12 PM, Masami Hiramatsu
<masami.hiramatsu.pt@hitachi.com> wrote:
> (2015/03/03 2:35), Alexei Starovoitov wrote:
>> On Mon, Mar 2, 2015 at 6:21 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>>> Masami noted that it would be better to hide the remaining CONFIG_BPF_SYSCALL-only
>>> function declarations within the BPF header ifdef, w/o else path dummy alternatives
>>> since these functions are not supposed to have a user outside of CONFIG_BPF_SYSCALL.
>>
>> So far we didn't have anyone trying to add new map types
>> outside of kernel/bpf/, so this patch is a defensive move.
>> Such potential future abuser will get compile error for
>> missing bpf_register_map_type() instead of linker error for the same.
>> Not sure that's really needed.
>> Also bpf_map_put() and bpf_map_get() are only used
>> by bpf syscall and verifier. imo moving them under ifdef is overkill.
>
> No, not overkill. This patch becomes good description about the code to
> reader who learns the eBPF. It tells them what functions are provided
> when the kconfig is set or not.
>
> And compiler error is better since abuser would get it before
> compiling all other object :)
Just as Daniel, I don't have strong preference one way or another.
Dave already applied it to net-next, so hopefully now you can
do some more eBPF coding :)
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net-next] ebpf: move CONFIG_BPF_SYSCALL-only function declarations
@ 2015-03-02 14:21 Daniel Borkmann
2015-03-02 20:09 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2015-03-02 14:21 UTC (permalink / raw)
To: davem; +Cc: ast, masami.hiramatsu.pt, netdev, Daniel Borkmann
Masami noted that it would be better to hide the remaining CONFIG_BPF_SYSCALL-only
function declarations within the BPF header ifdef, w/o else path dummy alternatives
since these functions are not supposed to have a user outside of CONFIG_BPF_SYSCALL.
Suggested-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Reference: http://article.gmane.org/gmane.linux.kernel.api/8658
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
include/linux/bpf.h | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index a1a7ff2..a884f5a 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -42,10 +42,6 @@ struct bpf_map_type_list {
enum bpf_map_type type;
};
-void bpf_register_map_type(struct bpf_map_type_list *tl);
-void bpf_map_put(struct bpf_map *map);
-struct bpf_map *bpf_map_get(struct fd f);
-
/* function argument constraints */
enum bpf_arg_type {
ARG_ANYTHING = 0, /* any argument is ok */
@@ -126,9 +122,16 @@ struct bpf_prog_aux {
#ifdef CONFIG_BPF_SYSCALL
void bpf_register_prog_type(struct bpf_prog_type_list *tl);
+void bpf_register_map_type(struct bpf_map_type_list *tl);
-void bpf_prog_put(struct bpf_prog *prog);
struct bpf_prog *bpf_prog_get(u32 ufd);
+void bpf_prog_put(struct bpf_prog *prog);
+
+struct bpf_map *bpf_map_get(struct fd f);
+void bpf_map_put(struct bpf_map *map);
+
+/* verify correctness of eBPF program */
+int bpf_check(struct bpf_prog *fp, union bpf_attr *attr);
#else
static inline void bpf_register_prog_type(struct bpf_prog_type_list *tl)
{
@@ -142,10 +145,7 @@ static inline struct bpf_prog *bpf_prog_get(u32 ufd)
static inline void bpf_prog_put(struct bpf_prog *prog)
{
}
-#endif
-
-/* verify correctness of eBPF program */
-int bpf_check(struct bpf_prog *fp, union bpf_attr *attr);
+#endif /* CONFIG_BPF_SYSCALL */
/* verifier prototypes for helper functions called from eBPF programs */
extern const struct bpf_func_proto bpf_map_lookup_elem_proto;
--
1.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net-next] ebpf: move CONFIG_BPF_SYSCALL-only function declarations
2015-03-02 14:21 Daniel Borkmann
@ 2015-03-02 20:09 ` David Miller
0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2015-03-02 20:09 UTC (permalink / raw)
To: daniel; +Cc: ast, masami.hiramatsu.pt, netdev
From: Daniel Borkmann <daniel@iogearbox.net>
Date: Mon, 2 Mar 2015 15:21:55 +0100
> Masami noted that it would be better to hide the remaining CONFIG_BPF_SYSCALL-only
> function declarations within the BPF header ifdef, w/o else path dummy alternatives
> since these functions are not supposed to have a user outside of CONFIG_BPF_SYSCALL.
>
> Suggested-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Reference: http://article.gmane.org/gmane.linux.kernel.api/8658
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Applied, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-03-03 3:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-03-02 17:35 [PATCH net-next] ebpf: move CONFIG_BPF_SYSCALL-only function declarations Alexei Starovoitov
2015-03-02 17:41 ` Daniel Borkmann
2015-03-03 3:12 ` Masami Hiramatsu
2015-03-03 3:15 ` Alexei Starovoitov
-- strict thread matches above, loose matches on Subject: below --
2015-03-02 14:21 Daniel Borkmann
2015-03-02 20:09 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).