diff mbox series

bpf: fix building without CONFIG_INET

Message ID 20181005161526.843924-1-arnd@arndb.de
State New
Headers show
Series bpf: fix building without CONFIG_INET | expand

Commit Message

Arnd Bergmann Oct. 5, 2018, 4:15 p.m. UTC 
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 <arnd@arndb.de>

---
 include/linux/filter.h |  2 +-
 net/core/filter.c      | 18 +++++++++++-------
 2 files changed, 12 insertions(+), 8 deletions(-)

-- 
2.18.0

Comments

Joe Stringer Oct. 5, 2018, 5:19 p.m. UTC | #1 
On Fri, 5 Oct 2018 at 09:16, Arnd Bergmann <arnd@arndb.de> 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 <arnd@arndb.de>

> ---


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);
       }
Song Liu Oct. 5, 2018, 7:02 p.m. UTC | #2 
On Fri, Oct 5, 2018 at 9:18 AM Arnd Bergmann <arnd@arndb.de> 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 <arnd@arndb.de>

> ---

>  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

>
Daniel Borkmann Oct. 5, 2018, 7:07 p.m. UTC | #3 
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!
Daniel Borkmann Oct. 8, 2018, 8:40 a.m. UTC | #4 
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
Joe Stringer Oct. 8, 2018, 6:24 p.m. UTC | #5 
On Mon, 8 Oct 2018 at 01:41, Daniel Borkmann <daniel@iogearbox.net> 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.
diff mbox series

Patch

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);
 	}