The newly added TCP and UDP handling fails to link when CONFIG_INET
is disabled:
net/core/filter.o: In function `sk_lookup':
filter.c:(.text+0x7ff8): undefined reference to `tcp_hashinfo'
filter.c:(.text+0x7ffc): undefined reference to `tcp_hashinfo'
filter.c:(.text+0x8020): undefined reference to `__inet_lookup_established'
filter.c:(.text+0x8058): undefined reference to `__inet_lookup_listener'
filter.c:(.text+0x8068): undefined reference to `udp_table'
filter.c:(.text+0x8070): undefined reference to `udp_table'
filter.c:(.text+0x808c): undefined reference to `__udp4_lib_lookup'
net/core/filter.o: In function `bpf_sk_release':
filter.c:(.text+0x82e8): undefined reference to `sock_gen_put'
The compiler can optimize it out and avoid those references for
the most part, but we are missing a few steps here:
- sk_lookup() should always have been marked 'static', this also
avoids a warning about a missing prototype when building with
'make W=1'.
- The BPF_CALL_x() macro needs a little change to allow marking
the unneeded BPF call as 'static' and having the compiler
drop them.
- The reference to the bpf_func_proto must be made conditional.
Fixes: 6acc9b432e67 ("bpf: Add helper to retrieve socket in BPF")
Signed-off-by: Arnd Bergmann <[email protected]>
---
include/linux/filter.h | 2 +-
net/core/filter.c | 18 +++++++++++-------
2 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/include/linux/filter.h b/include/linux/filter.h
index 6791a0ac0139..d9ec9d908bbe 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -428,9 +428,9 @@ struct sock_reuseport;
u64, __ur_3, u64, __ur_4, u64, __ur_5)
#define BPF_CALL_x(x, name, ...) \
+ u64 name(__BPF_REG(x, __BPF_DECL_REGS, __BPF_N, __VA_ARGS__)); \
static __always_inline \
u64 ____##name(__BPF_MAP(x, __BPF_DECL_ARGS, __BPF_V, __VA_ARGS__)); \
- u64 name(__BPF_REG(x, __BPF_DECL_REGS, __BPF_N, __VA_ARGS__)); \
u64 name(__BPF_REG(x, __BPF_DECL_REGS, __BPF_N, __VA_ARGS__)) \
{ \
return ____##name(__BPF_MAP(x,__BPF_CAST,__BPF_N,__VA_ARGS__));\
diff --git a/net/core/filter.c b/net/core/filter.c
index 30c6b2d3ef16..dd5fe021f44c 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4817,7 +4817,7 @@ static const struct bpf_func_proto bpf_lwt_seg6_adjust_srh_proto = {
};
#endif /* CONFIG_IPV6_SEG6_BPF */
-struct sock *sk_lookup(struct net *net, struct bpf_sock_tuple *tuple,
+static struct sock *sk_lookup(struct net *net, struct bpf_sock_tuple *tuple,
struct sk_buff *skb, u8 family, u8 proto)
{
int dif = skb->dev->ifindex;
@@ -4902,13 +4902,13 @@ bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
return (unsigned long) sk;
}
-BPF_CALL_5(bpf_sk_lookup_tcp, struct sk_buff *, skb,
+static BPF_CALL_5(bpf_sk_lookup_tcp, struct sk_buff *, skb,
struct bpf_sock_tuple *, tuple, u32, len, u64, netns_id, u64, flags)
{
return bpf_sk_lookup(skb, tuple, len, IPPROTO_TCP, netns_id, flags);
}
-static const struct bpf_func_proto bpf_sk_lookup_tcp_proto = {
+static const __maybe_unused struct bpf_func_proto bpf_sk_lookup_tcp_proto = {
.func = bpf_sk_lookup_tcp,
.gpl_only = false,
.pkt_access = true,
@@ -4920,13 +4920,13 @@ static const struct bpf_func_proto bpf_sk_lookup_tcp_proto = {
.arg5_type = ARG_ANYTHING,
};
-BPF_CALL_5(bpf_sk_lookup_udp, struct sk_buff *, skb,
+static BPF_CALL_5(bpf_sk_lookup_udp, struct sk_buff *, skb,
struct bpf_sock_tuple *, tuple, u32, len, u64, netns_id, u64, flags)
{
return bpf_sk_lookup(skb, tuple, len, IPPROTO_UDP, netns_id, flags);
}
-static const struct bpf_func_proto bpf_sk_lookup_udp_proto = {
+static const __maybe_unused struct bpf_func_proto bpf_sk_lookup_udp_proto = {
.func = bpf_sk_lookup_udp,
.gpl_only = false,
.pkt_access = true,
@@ -4938,14 +4938,14 @@ static const struct bpf_func_proto bpf_sk_lookup_udp_proto = {
.arg5_type = ARG_ANYTHING,
};
-BPF_CALL_1(bpf_sk_release, struct sock *, sk)
+static BPF_CALL_1(bpf_sk_release, struct sock *, sk)
{
if (!sock_flag(sk, SOCK_RCU_FREE))
sock_gen_put(sk);
return 0;
}
-static const struct bpf_func_proto bpf_sk_release_proto = {
+static const __maybe_unused struct bpf_func_proto bpf_sk_release_proto = {
.func = bpf_sk_release,
.gpl_only = false,
.ret_type = RET_INTEGER,
@@ -5158,12 +5158,14 @@ tc_cls_act_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
case BPF_FUNC_skb_ancestor_cgroup_id:
return &bpf_skb_ancestor_cgroup_id_proto;
#endif
+#ifdef CONFIG_INET
case BPF_FUNC_sk_lookup_tcp:
return &bpf_sk_lookup_tcp_proto;
case BPF_FUNC_sk_lookup_udp:
return &bpf_sk_lookup_udp_proto;
case BPF_FUNC_sk_release:
return &bpf_sk_release_proto;
+#endif
default:
return bpf_base_func_proto(func_id);
}
@@ -5264,12 +5266,14 @@ sk_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_sk_redirect_hash_proto;
case BPF_FUNC_get_local_storage:
return &bpf_get_local_storage_proto;
+#ifdef CONFIG_INET
case BPF_FUNC_sk_lookup_tcp:
return &bpf_sk_lookup_tcp_proto;
case BPF_FUNC_sk_lookup_udp:
return &bpf_sk_lookup_udp_proto;
case BPF_FUNC_sk_release:
return &bpf_sk_release_proto;
+#endif
default:
return bpf_base_func_proto(func_id);
}
--
2.18.0
On Fri, 5 Oct 2018 at 09:16, Arnd Bergmann <[email protected]> wrote:
>
> The newly added TCP and UDP handling fails to link when CONFIG_INET
> is disabled:
>
> net/core/filter.o: In function `sk_lookup':
> filter.c:(.text+0x7ff8): undefined reference to `tcp_hashinfo'
> filter.c:(.text+0x7ffc): undefined reference to `tcp_hashinfo'
> filter.c:(.text+0x8020): undefined reference to `__inet_lookup_established'
> filter.c:(.text+0x8058): undefined reference to `__inet_lookup_listener'
> filter.c:(.text+0x8068): undefined reference to `udp_table'
> filter.c:(.text+0x8070): undefined reference to `udp_table'
> filter.c:(.text+0x808c): undefined reference to `__udp4_lib_lookup'
> net/core/filter.o: In function `bpf_sk_release':
> filter.c:(.text+0x82e8): undefined reference to `sock_gen_put'
>
> The compiler can optimize it out and avoid those references for
> the most part, but we are missing a few steps here:
>
> - sk_lookup() should always have been marked 'static', this also
> avoids a warning about a missing prototype when building with
> 'make W=1'.
> - The BPF_CALL_x() macro needs a little change to allow marking
> the unneeded BPF call as 'static' and having the compiler
> drop them.
> - The reference to the bpf_func_proto must be made conditional.
>
> Fixes: 6acc9b432e67 ("bpf: Add helper to retrieve socket in BPF")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
Thanks for the fix.
> include/linux/filter.h | 2 +-
> net/core/filter.c | 18 +++++++++++-------
> 2 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 6791a0ac0139..d9ec9d908bbe 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -428,9 +428,9 @@ struct sock_reuseport;
> u64, __ur_3, u64, __ur_4, u64, __ur_5)
>
> #define BPF_CALL_x(x, name, ...) \
> + u64 name(__BPF_REG(x, __BPF_DECL_REGS, __BPF_N, __VA_ARGS__)); \
> static __always_inline \
> u64 ____##name(__BPF_MAP(x, __BPF_DECL_ARGS, __BPF_V, __VA_ARGS__)); \
> - u64 name(__BPF_REG(x, __BPF_DECL_REGS, __BPF_N, __VA_ARGS__)); \
> u64 name(__BPF_REG(x, __BPF_DECL_REGS, __BPF_N, __VA_ARGS__)) \
> { \
> return ____##name(__BPF_MAP(x,__BPF_CAST,__BPF_N,__VA_ARGS__));\
For what it's worth, other similar cases in net/core/filter.c avoid
this by just wrapping the relevant sections of the code in the #ifdef
as well.
Might be a bit simpler to follow that style (only checked with make
M=net/core W=1):
$ git di
diff --git a/net/core/filter.c b/net/core/filter.c
index 30c6b2d3ef16..4bbc6567fcb8 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4817,8 +4817,9 @@ static const struct bpf_func_proto
bpf_lwt_seg6_adjust_srh_proto = {
};
#endif /* CONFIG_IPV6_SEG6_BPF */
-struct sock *sk_lookup(struct net *net, struct bpf_sock_tuple *tuple,
- struct sk_buff *skb, u8 family, u8 proto)
+#ifdef CONFIG_INET
+static struct sock *sk_lookup(struct net *net, struct bpf_sock_tuple *tuple,
+ struct sk_buff *skb, u8 family, u8 proto)
{
int dif = skb->dev->ifindex;
bool refcounted = false;
@@ -4951,6 +4952,7 @@ static const struct bpf_func_proto
bpf_sk_release_proto = {
.ret_type = RET_INTEGER,
.arg1_type = ARG_PTR_TO_SOCKET,
};
+#endif /* CONFIG_INET */
bool bpf_helper_changes_pkt_data(void *func)
{
@@ -5158,12 +5160,14 @@ tc_cls_act_func_proto(enum bpf_func_id
func_id, const struct bpf_prog *prog)
case BPF_FUNC_skb_ancestor_cgroup_id:
return &bpf_skb_ancestor_cgroup_id_proto;
#endif
+#ifdef CONFIG_INET
case BPF_FUNC_sk_lookup_tcp:
return &bpf_sk_lookup_tcp_proto;
case BPF_FUNC_sk_lookup_udp:
return &bpf_sk_lookup_udp_proto;
case BPF_FUNC_sk_release:
return &bpf_sk_release_proto;
+#endif
default:
return bpf_base_func_proto(func_id);
}
@@ -5264,12 +5268,14 @@ sk_skb_func_proto(enum bpf_func_id func_id,
const struct bpf_prog *prog)
return &bpf_sk_redirect_hash_proto;
case BPF_FUNC_get_local_storage:
return &bpf_get_local_storage_proto;
+#ifdef CONFIG_INET
case BPF_FUNC_sk_lookup_tcp:
return &bpf_sk_lookup_tcp_proto;
case BPF_FUNC_sk_lookup_udp:
return &bpf_sk_lookup_udp_proto;
case BPF_FUNC_sk_release:
return &bpf_sk_release_proto;
+#endif
default:
return bpf_base_func_proto(func_id);
}
On Fri, Oct 5, 2018 at 9:18 AM Arnd Bergmann <[email protected]> wrote:
>
> The newly added TCP and UDP handling fails to link when CONFIG_INET
> is disabled:
>
> net/core/filter.o: In function `sk_lookup':
> filter.c:(.text+0x7ff8): undefined reference to `tcp_hashinfo'
> filter.c:(.text+0x7ffc): undefined reference to `tcp_hashinfo'
> filter.c:(.text+0x8020): undefined reference to `__inet_lookup_established'
> filter.c:(.text+0x8058): undefined reference to `__inet_lookup_listener'
> filter.c:(.text+0x8068): undefined reference to `udp_table'
> filter.c:(.text+0x8070): undefined reference to `udp_table'
> filter.c:(.text+0x808c): undefined reference to `__udp4_lib_lookup'
> net/core/filter.o: In function `bpf_sk_release':
> filter.c:(.text+0x82e8): undefined reference to `sock_gen_put'
>
> The compiler can optimize it out and avoid those references for
> the most part, but we are missing a few steps here:
>
> - sk_lookup() should always have been marked 'static', this also
> avoids a warning about a missing prototype when building with
> 'make W=1'.
> - The BPF_CALL_x() macro needs a little change to allow marking
> the unneeded BPF call as 'static' and having the compiler
> drop them.
> - The reference to the bpf_func_proto must be made conditional.
>
> Fixes: 6acc9b432e67 ("bpf: Add helper to retrieve socket in BPF")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> include/linux/filter.h | 2 +-
> net/core/filter.c | 18 +++++++++++-------
> 2 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index 6791a0ac0139..d9ec9d908bbe 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -428,9 +428,9 @@ struct sock_reuseport;
> u64, __ur_3, u64, __ur_4, u64, __ur_5)
>
> #define BPF_CALL_x(x, name, ...) \
> + u64 name(__BPF_REG(x, __BPF_DECL_REGS, __BPF_N, __VA_ARGS__)); \
> static __always_inline \
> u64 ____##name(__BPF_MAP(x, __BPF_DECL_ARGS, __BPF_V, __VA_ARGS__)); \
> - u64 name(__BPF_REG(x, __BPF_DECL_REGS, __BPF_N, __VA_ARGS__)); \
> u64 name(__BPF_REG(x, __BPF_DECL_REGS, __BPF_N, __VA_ARGS__)) \
> { \
> return ____##name(__BPF_MAP(x,__BPF_CAST,__BPF_N,__VA_ARGS__));\
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 30c6b2d3ef16..dd5fe021f44c 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -4817,7 +4817,7 @@ static const struct bpf_func_proto bpf_lwt_seg6_adjust_srh_proto = {
> };
> #endif /* CONFIG_IPV6_SEG6_BPF */
>
> -struct sock *sk_lookup(struct net *net, struct bpf_sock_tuple *tuple,
> +static struct sock *sk_lookup(struct net *net, struct bpf_sock_tuple *tuple,
> struct sk_buff *skb, u8 family, u8 proto)
> {
> int dif = skb->dev->ifindex;
> @@ -4902,13 +4902,13 @@ bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
> return (unsigned long) sk;
> }
>
> -BPF_CALL_5(bpf_sk_lookup_tcp, struct sk_buff *, skb,
> +static BPF_CALL_5(bpf_sk_lookup_tcp, struct sk_buff *, skb,
> struct bpf_sock_tuple *, tuple, u32, len, u64, netns_id, u64, flags)
> {
> return bpf_sk_lookup(skb, tuple, len, IPPROTO_TCP, netns_id, flags);
> }
BPF_CALL_x() has static already (before this patch). We should not
need change that
for all the BPF_CALL_?(). Joe's version looks better to me.
Thanks,
Song
>
> -static const struct bpf_func_proto bpf_sk_lookup_tcp_proto = {
> +static const __maybe_unused struct bpf_func_proto bpf_sk_lookup_tcp_proto = {
> .func = bpf_sk_lookup_tcp,
> .gpl_only = false,
> .pkt_access = true,
> @@ -4920,13 +4920,13 @@ static const struct bpf_func_proto bpf_sk_lookup_tcp_proto = {
> .arg5_type = ARG_ANYTHING,
> };
>
> -BPF_CALL_5(bpf_sk_lookup_udp, struct sk_buff *, skb,
> +static BPF_CALL_5(bpf_sk_lookup_udp, struct sk_buff *, skb,
> struct bpf_sock_tuple *, tuple, u32, len, u64, netns_id, u64, flags)
> {
> return bpf_sk_lookup(skb, tuple, len, IPPROTO_UDP, netns_id, flags);
> }
>
> -static const struct bpf_func_proto bpf_sk_lookup_udp_proto = {
> +static const __maybe_unused struct bpf_func_proto bpf_sk_lookup_udp_proto = {
> .func = bpf_sk_lookup_udp,
> .gpl_only = false,
> .pkt_access = true,
> @@ -4938,14 +4938,14 @@ static const struct bpf_func_proto bpf_sk_lookup_udp_proto = {
> .arg5_type = ARG_ANYTHING,
> };
>
> -BPF_CALL_1(bpf_sk_release, struct sock *, sk)
> +static BPF_CALL_1(bpf_sk_release, struct sock *, sk)
> {
> if (!sock_flag(sk, SOCK_RCU_FREE))
> sock_gen_put(sk);
> return 0;
> }
>
> -static const struct bpf_func_proto bpf_sk_release_proto = {
> +static const __maybe_unused struct bpf_func_proto bpf_sk_release_proto = {
> .func = bpf_sk_release,
> .gpl_only = false,
> .ret_type = RET_INTEGER,
> @@ -5158,12 +5158,14 @@ tc_cls_act_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> case BPF_FUNC_skb_ancestor_cgroup_id:
> return &bpf_skb_ancestor_cgroup_id_proto;
> #endif
> +#ifdef CONFIG_INET
> case BPF_FUNC_sk_lookup_tcp:
> return &bpf_sk_lookup_tcp_proto;
> case BPF_FUNC_sk_lookup_udp:
> return &bpf_sk_lookup_udp_proto;
> case BPF_FUNC_sk_release:
> return &bpf_sk_release_proto;
> +#endif
> default:
> return bpf_base_func_proto(func_id);
> }
> @@ -5264,12 +5266,14 @@ sk_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> return &bpf_sk_redirect_hash_proto;
> case BPF_FUNC_get_local_storage:
> return &bpf_get_local_storage_proto;
> +#ifdef CONFIG_INET
> case BPF_FUNC_sk_lookup_tcp:
> return &bpf_sk_lookup_tcp_proto;
> case BPF_FUNC_sk_lookup_udp:
> return &bpf_sk_lookup_udp_proto;
> case BPF_FUNC_sk_release:
> return &bpf_sk_release_proto;
> +#endif
> default:
> return bpf_base_func_proto(func_id);
> }
> --
> 2.18.0
>
On 10/05/2018 09:02 PM, Song Liu wrote:
[...]
> BPF_CALL_x() has static already (before this patch). We should not
> need change that
> for all the BPF_CALL_?(). Joe's version looks better to me.
My preference as well, thanks!
On 10/05/2018 09:07 PM, Daniel Borkmann wrote:
> On 10/05/2018 09:02 PM, Song Liu wrote:
> [...]
>> BPF_CALL_x() has static already (before this patch). We should not
>> need change that
>> for all the BPF_CALL_?(). Joe's version looks better to me.
>
> My preference as well, thanks!
ping, can someone send an updated patch?
Thanks,
Daniel
On Mon, 8 Oct 2018 at 01:41, Daniel Borkmann <[email protected]> wrote:
>
> On 10/05/2018 09:07 PM, Daniel Borkmann wrote:
> > On 10/05/2018 09:02 PM, Song Liu wrote:
> > [...]
> >> BPF_CALL_x() has static already (before this patch). We should not
> >> need change that
> >> for all the BPF_CALL_?(). Joe's version looks better to me.
> >
> > My preference as well, thanks!
>
> ping, can someone send an updated patch?
I will send a fresh version shortly.