diff mbox series

bpf: fix "unresolved symbol" build error with resolve_btfids

Message ID 20200930164109.2922412-1-yhs@fb.com (mailing list archive)
State Superseded
Headers show
Series bpf: fix "unresolved symbol" build error with resolve_btfids | expand

Commit Message

Yonghong Song Sept. 30, 2020, 4:41 p.m. UTC
Michal reported a build failure likes below:
   BTFIDS  vmlinux
   FAILED unresolved symbol tcp_timewait_sock
   make[1]: *** [/.../linux-5.9-rc7/Makefile:1176: vmlinux] Error 255

This error can be triggered when config has CONFIG_NET enabled
but CONFIG_INET disabled. In this case, there is no user of
structs inet_timewait_sock and tcp_timewait_sock and hence vmlinux BTF
types are not generated for these two structures.

To fix the problem, omit the above two types for BTF_SOCK_TYPE_xxx
macro if CONFIG_INET is not defined.

Fixes: fce557bcef11 ("bpf: Make btf_sock_ids global")
Reported-by: Michal Kubecek <mkubecek@suse.cz>
Signed-off-by: Yonghong Song <yhs@fb.com>
---
 include/linux/btf_ids.h | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

Comments

Andrii Nakryiko Sept. 30, 2020, 6:40 p.m. UTC | #1
On Wed, Sep 30, 2020 at 9:41 AM Yonghong Song <yhs@fb.com> wrote:
>
> Michal reported a build failure likes below:
>    BTFIDS  vmlinux
>    FAILED unresolved symbol tcp_timewait_sock
>    make[1]: *** [/.../linux-5.9-rc7/Makefile:1176: vmlinux] Error 255
>
> This error can be triggered when config has CONFIG_NET enabled
> but CONFIG_INET disabled. In this case, there is no user of
> structs inet_timewait_sock and tcp_timewait_sock and hence vmlinux BTF
> types are not generated for these two structures.
>
> To fix the problem, omit the above two types for BTF_SOCK_TYPE_xxx
> macro if CONFIG_INET is not defined.
>
> Fixes: fce557bcef11 ("bpf: Make btf_sock_ids global")
> Reported-by: Michal Kubecek <mkubecek@suse.cz>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
>  include/linux/btf_ids.h | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
> index 4867d549e3c1..d9a1e18d0921 100644
> --- a/include/linux/btf_ids.h
> +++ b/include/linux/btf_ids.h
> @@ -102,24 +102,36 @@ asm(                                                      \
>   * skc_to_*_sock() helpers. All these sockets should have
>   * sock_common as the first argument in its memory layout.
>   */
> -#define BTF_SOCK_TYPE_xxx \
> +
> +#define __BTF_SOCK_TYPE_xxx \
>         BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET, inet_sock)                    \
>         BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_CONN, inet_connection_sock)    \
>         BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_REQ, inet_request_sock)        \
> -       BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_TW, inet_timewait_sock)        \
>         BTF_SOCK_TYPE(BTF_SOCK_TYPE_REQ, request_sock)                  \
>         BTF_SOCK_TYPE(BTF_SOCK_TYPE_SOCK, sock)                         \
>         BTF_SOCK_TYPE(BTF_SOCK_TYPE_SOCK_COMMON, sock_common)           \
>         BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP, tcp_sock)                      \
>         BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP_REQ, tcp_request_sock)          \
> -       BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP_TW, tcp_timewait_sock)          \
>         BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP6, tcp6_sock)                    \
>         BTF_SOCK_TYPE(BTF_SOCK_TYPE_UDP, udp_sock)                      \
>         BTF_SOCK_TYPE(BTF_SOCK_TYPE_UDP6, udp6_sock)
>
> +#define __BTF_SOCK_TW_TYPE_xxx \
> +       BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_TW, inet_timewait_sock)        \
> +       BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP_TW, tcp_timewait_sock)
> +
> +#ifdef CONFIG_INET
> +#define BTF_SOCK_TYPE_xxx                                              \
> +       __BTF_SOCK_TYPE_xxx                                             \
> +       __BTF_SOCK_TW_TYPE_xxx
> +#else
> +#define BTF_SOCK_TYPE_xxx      __BTF_SOCK_TYPE_xxx
> +#endif
> +
>  enum {
>  #define BTF_SOCK_TYPE(name, str) name,
> -BTF_SOCK_TYPE_xxx
> +__BTF_SOCK_TYPE_xxx
> +__BTF_SOCK_TW_TYPE_xxx

Why BTF_SOCK_TYPE_xxx doesn't still work here after the above changes?

>  #undef BTF_SOCK_TYPE
>  MAX_BTF_SOCK_TYPE,
>  };
> --
> 2.24.1
>
Yonghong Song Sept. 30, 2020, 7:24 p.m. UTC | #2
On 9/30/20 11:40 AM, Andrii Nakryiko wrote:
> On Wed, Sep 30, 2020 at 9:41 AM Yonghong Song <yhs@fb.com> wrote:
>>
>> Michal reported a build failure likes below:
>>     BTFIDS  vmlinux
>>     FAILED unresolved symbol tcp_timewait_sock
>>     make[1]: *** [/.../linux-5.9-rc7/Makefile:1176: vmlinux] Error 255
>>
>> This error can be triggered when config has CONFIG_NET enabled
>> but CONFIG_INET disabled. In this case, there is no user of
>> structs inet_timewait_sock and tcp_timewait_sock and hence vmlinux BTF
>> types are not generated for these two structures.
>>
>> To fix the problem, omit the above two types for BTF_SOCK_TYPE_xxx
>> macro if CONFIG_INET is not defined.
>>
>> Fixes: fce557bcef11 ("bpf: Make btf_sock_ids global")
>> Reported-by: Michal Kubecek <mkubecek@suse.cz>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   include/linux/btf_ids.h | 20 ++++++++++++++++----
>>   1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
>> index 4867d549e3c1..d9a1e18d0921 100644
>> --- a/include/linux/btf_ids.h
>> +++ b/include/linux/btf_ids.h
>> @@ -102,24 +102,36 @@ asm(                                                      \
>>    * skc_to_*_sock() helpers. All these sockets should have
>>    * sock_common as the first argument in its memory layout.
>>    */
>> -#define BTF_SOCK_TYPE_xxx \
>> +
>> +#define __BTF_SOCK_TYPE_xxx \
>>          BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET, inet_sock)                    \
>>          BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_CONN, inet_connection_sock)    \
>>          BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_REQ, inet_request_sock)        \
>> -       BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_TW, inet_timewait_sock)        \
>>          BTF_SOCK_TYPE(BTF_SOCK_TYPE_REQ, request_sock)                  \
>>          BTF_SOCK_TYPE(BTF_SOCK_TYPE_SOCK, sock)                         \
>>          BTF_SOCK_TYPE(BTF_SOCK_TYPE_SOCK_COMMON, sock_common)           \
>>          BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP, tcp_sock)                      \
>>          BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP_REQ, tcp_request_sock)          \
>> -       BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP_TW, tcp_timewait_sock)          \
>>          BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP6, tcp6_sock)                    \
>>          BTF_SOCK_TYPE(BTF_SOCK_TYPE_UDP, udp_sock)                      \
>>          BTF_SOCK_TYPE(BTF_SOCK_TYPE_UDP6, udp6_sock)
>>
>> +#define __BTF_SOCK_TW_TYPE_xxx \
>> +       BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_TW, inet_timewait_sock)        \
>> +       BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP_TW, tcp_timewait_sock)
>> +
>> +#ifdef CONFIG_INET
>> +#define BTF_SOCK_TYPE_xxx                                              \
>> +       __BTF_SOCK_TYPE_xxx                                             \
>> +       __BTF_SOCK_TW_TYPE_xxx
>> +#else
>> +#define BTF_SOCK_TYPE_xxx      __BTF_SOCK_TYPE_xxx
>> +#endif
>> +
>>   enum {
>>   #define BTF_SOCK_TYPE(name, str) name,
>> -BTF_SOCK_TYPE_xxx
>> +__BTF_SOCK_TYPE_xxx
>> +__BTF_SOCK_TW_TYPE_xxx
> 
> Why BTF_SOCK_TYPE_xxx doesn't still work here after the above changes?

The macro, e.g., BTF_SOCK_TYPE_TCP_TW, still needed to be defined as
it is used to get the location for btf_id.

const struct bpf_func_proto bpf_skc_to_tcp_timewait_sock_proto = {
         .func                   = bpf_skc_to_tcp_timewait_sock,
         .gpl_only               = false,
         .ret_type               = RET_PTR_TO_BTF_ID_OR_NULL,
         .arg1_type              = ARG_PTR_TO_BTF_ID_SOCK_COMMON,
         .ret_btf_id             = &btf_sock_ids[BTF_SOCK_TYPE_TCP_TW],
};

If CONFIG_INET is not defined, bpf_sock_ids[BTF_SOCK_TYPE_TCP_TW]
will be 0.

> 
>>   #undef BTF_SOCK_TYPE
>>   MAX_BTF_SOCK_TYPE,
>>   };
>> --
>> 2.24.1
>>
Andrii Nakryiko Sept. 30, 2020, 7:44 p.m. UTC | #3
On Wed, Sep 30, 2020 at 12:25 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 9/30/20 11:40 AM, Andrii Nakryiko wrote:
> > On Wed, Sep 30, 2020 at 9:41 AM Yonghong Song <yhs@fb.com> wrote:
> >>
> >> Michal reported a build failure likes below:
> >>     BTFIDS  vmlinux
> >>     FAILED unresolved symbol tcp_timewait_sock
> >>     make[1]: *** [/.../linux-5.9-rc7/Makefile:1176: vmlinux] Error 255
> >>
> >> This error can be triggered when config has CONFIG_NET enabled
> >> but CONFIG_INET disabled. In this case, there is no user of
> >> structs inet_timewait_sock and tcp_timewait_sock and hence vmlinux BTF
> >> types are not generated for these two structures.
> >>
> >> To fix the problem, omit the above two types for BTF_SOCK_TYPE_xxx
> >> macro if CONFIG_INET is not defined.
> >>
> >> Fixes: fce557bcef11 ("bpf: Make btf_sock_ids global")
> >> Reported-by: Michal Kubecek <mkubecek@suse.cz>
> >> Signed-off-by: Yonghong Song <yhs@fb.com>
> >> ---
> >>   include/linux/btf_ids.h | 20 ++++++++++++++++----
> >>   1 file changed, 16 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
> >> index 4867d549e3c1..d9a1e18d0921 100644
> >> --- a/include/linux/btf_ids.h
> >> +++ b/include/linux/btf_ids.h
> >> @@ -102,24 +102,36 @@ asm(                                                      \
> >>    * skc_to_*_sock() helpers. All these sockets should have
> >>    * sock_common as the first argument in its memory layout.
> >>    */
> >> -#define BTF_SOCK_TYPE_xxx \
> >> +
> >> +#define __BTF_SOCK_TYPE_xxx \
> >>          BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET, inet_sock)                    \
> >>          BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_CONN, inet_connection_sock)    \
> >>          BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_REQ, inet_request_sock)        \
> >> -       BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_TW, inet_timewait_sock)        \
> >>          BTF_SOCK_TYPE(BTF_SOCK_TYPE_REQ, request_sock)                  \
> >>          BTF_SOCK_TYPE(BTF_SOCK_TYPE_SOCK, sock)                         \
> >>          BTF_SOCK_TYPE(BTF_SOCK_TYPE_SOCK_COMMON, sock_common)           \
> >>          BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP, tcp_sock)                      \
> >>          BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP_REQ, tcp_request_sock)          \
> >> -       BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP_TW, tcp_timewait_sock)          \
> >>          BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP6, tcp6_sock)                    \
> >>          BTF_SOCK_TYPE(BTF_SOCK_TYPE_UDP, udp_sock)                      \
> >>          BTF_SOCK_TYPE(BTF_SOCK_TYPE_UDP6, udp6_sock)
> >>
> >> +#define __BTF_SOCK_TW_TYPE_xxx \
> >> +       BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_TW, inet_timewait_sock)        \
> >> +       BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP_TW, tcp_timewait_sock)
> >> +
> >> +#ifdef CONFIG_INET
> >> +#define BTF_SOCK_TYPE_xxx                                              \
> >> +       __BTF_SOCK_TYPE_xxx                                             \
> >> +       __BTF_SOCK_TW_TYPE_xxx
> >> +#else
> >> +#define BTF_SOCK_TYPE_xxx      __BTF_SOCK_TYPE_xxx
> >> +#endif
> >> +
> >>   enum {
> >>   #define BTF_SOCK_TYPE(name, str) name,
> >> -BTF_SOCK_TYPE_xxx
> >> +__BTF_SOCK_TYPE_xxx
> >> +__BTF_SOCK_TW_TYPE_xxx
> >
> > Why BTF_SOCK_TYPE_xxx doesn't still work here after the above changes?
>
> The macro, e.g., BTF_SOCK_TYPE_TCP_TW, still needed to be defined as
> it is used to get the location for btf_id.

Ah, right, so you want those here unconditionally, even if CONFIG_INET
is not defined. Missed that. Might be worth leaving a short comment.

Otherwise LGTM.

Acked-by: Andrii Nakryiko <andriin@fb.com>

>
> const struct bpf_func_proto bpf_skc_to_tcp_timewait_sock_proto = {
>          .func                   = bpf_skc_to_tcp_timewait_sock,
>          .gpl_only               = false,
>          .ret_type               = RET_PTR_TO_BTF_ID_OR_NULL,
>          .arg1_type              = ARG_PTR_TO_BTF_ID_SOCK_COMMON,
>          .ret_btf_id             = &btf_sock_ids[BTF_SOCK_TYPE_TCP_TW],
> };
>
> If CONFIG_INET is not defined, bpf_sock_ids[BTF_SOCK_TYPE_TCP_TW]
> will be 0.
>
> >
> >>   #undef BTF_SOCK_TYPE
> >>   MAX_BTF_SOCK_TYPE,
> >>   };
> >> --
> >> 2.24.1
> >>
Martin KaFai Lau Sept. 30, 2020, 8:58 p.m. UTC | #4
On Wed, Sep 30, 2020 at 09:41:09AM -0700, Yonghong Song wrote:
> Michal reported a build failure likes below:
>    BTFIDS  vmlinux
>    FAILED unresolved symbol tcp_timewait_sock
>    make[1]: *** [/.../linux-5.9-rc7/Makefile:1176: vmlinux] Error 255
> 
> This error can be triggered when config has CONFIG_NET enabled
> but CONFIG_INET disabled. In this case, there is no user of
> structs inet_timewait_sock and tcp_timewait_sock and hence vmlinux BTF
> types are not generated for these two structures.
> 
> To fix the problem, omit the above two types for BTF_SOCK_TYPE_xxx
> macro if CONFIG_INET is not defined.
> 
> Fixes: fce557bcef11 ("bpf: Make btf_sock_ids global")
> Reported-by: Michal Kubecek <mkubecek@suse.cz>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
>  include/linux/btf_ids.h | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
> index 4867d549e3c1..d9a1e18d0921 100644
> --- a/include/linux/btf_ids.h
> +++ b/include/linux/btf_ids.h
> @@ -102,24 +102,36 @@ asm(							\
>   * skc_to_*_sock() helpers. All these sockets should have
>   * sock_common as the first argument in its memory layout.
>   */
> -#define BTF_SOCK_TYPE_xxx \
> +
> +#define __BTF_SOCK_TYPE_xxx \
>  	BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET, inet_sock)			\
>  	BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_CONN, inet_connection_sock)	\
>  	BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_REQ, inet_request_sock)	\
> -	BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_TW, inet_timewait_sock)	\
>  	BTF_SOCK_TYPE(BTF_SOCK_TYPE_REQ, request_sock)			\
>  	BTF_SOCK_TYPE(BTF_SOCK_TYPE_SOCK, sock)				\
>  	BTF_SOCK_TYPE(BTF_SOCK_TYPE_SOCK_COMMON, sock_common)		\
>  	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP, tcp_sock)			\
>  	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP_REQ, tcp_request_sock)		\
> -	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP_TW, tcp_timewait_sock)		\
>  	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP6, tcp6_sock)			\
>  	BTF_SOCK_TYPE(BTF_SOCK_TYPE_UDP, udp_sock)			\
>  	BTF_SOCK_TYPE(BTF_SOCK_TYPE_UDP6, udp6_sock)
>  
> +#define __BTF_SOCK_TW_TYPE_xxx \
> +	BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_TW, inet_timewait_sock)	\
> +	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP_TW, tcp_timewait_sock)
> +
> +#ifdef CONFIG_INET
> +#define BTF_SOCK_TYPE_xxx						\
> +	__BTF_SOCK_TYPE_xxx						\
> +	__BTF_SOCK_TW_TYPE_xxx
> +#else
> +#define BTF_SOCK_TYPE_xxx	__BTF_SOCK_TYPE_xxx
BTF_SOCK_TYPE_xxx is used in BTF_ID_LIST_GLOBAL(btf_sock_ids) in filter.c
which does not include BTF_SOCK_TYPE_TCP_TW.
However, btf_sock_ids[BTF_SOCK_TYPE_TCP_TW] is still used
in bpf_skc_to_tcp_timewait_sock_proto.

> +#endif
> +
>  enum {
>  #define BTF_SOCK_TYPE(name, str) name,
> -BTF_SOCK_TYPE_xxx
> +__BTF_SOCK_TYPE_xxx
> +__BTF_SOCK_TW_TYPE_xxx
BTF_SOCK_TYPE_TCP_TW is at the end of this enum.

Would btf_sock_ids[BTF_SOCK_TYPE_TCP_TW] always be 0?

>  #undef BTF_SOCK_TYPE
>  MAX_BTF_SOCK_TYPE,
>  };
> -- 
> 2.24.1
>
Yonghong Song Sept. 30, 2020, 10:50 p.m. UTC | #5
On 9/30/20 1:58 PM, Martin KaFai Lau wrote:
> On Wed, Sep 30, 2020 at 09:41:09AM -0700, Yonghong Song wrote:
>> Michal reported a build failure likes below:
>>     BTFIDS  vmlinux
>>     FAILED unresolved symbol tcp_timewait_sock
>>     make[1]: *** [/.../linux-5.9-rc7/Makefile:1176: vmlinux] Error 255
>>
>> This error can be triggered when config has CONFIG_NET enabled
>> but CONFIG_INET disabled. In this case, there is no user of
>> structs inet_timewait_sock and tcp_timewait_sock and hence vmlinux BTF
>> types are not generated for these two structures.
>>
>> To fix the problem, omit the above two types for BTF_SOCK_TYPE_xxx
>> macro if CONFIG_INET is not defined.
>>
>> Fixes: fce557bcef11 ("bpf: Make btf_sock_ids global")
>> Reported-by: Michal Kubecek <mkubecek@suse.cz>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   include/linux/btf_ids.h | 20 ++++++++++++++++----
>>   1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
>> index 4867d549e3c1..d9a1e18d0921 100644
>> --- a/include/linux/btf_ids.h
>> +++ b/include/linux/btf_ids.h
>> @@ -102,24 +102,36 @@ asm(							\
>>    * skc_to_*_sock() helpers. All these sockets should have
>>    * sock_common as the first argument in its memory layout.
>>    */
>> -#define BTF_SOCK_TYPE_xxx \
>> +
>> +#define __BTF_SOCK_TYPE_xxx \
>>   	BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET, inet_sock)			\
>>   	BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_CONN, inet_connection_sock)	\
>>   	BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_REQ, inet_request_sock)	\
>> -	BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_TW, inet_timewait_sock)	\
>>   	BTF_SOCK_TYPE(BTF_SOCK_TYPE_REQ, request_sock)			\
>>   	BTF_SOCK_TYPE(BTF_SOCK_TYPE_SOCK, sock)				\
>>   	BTF_SOCK_TYPE(BTF_SOCK_TYPE_SOCK_COMMON, sock_common)		\
>>   	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP, tcp_sock)			\
>>   	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP_REQ, tcp_request_sock)		\
>> -	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP_TW, tcp_timewait_sock)		\
>>   	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP6, tcp6_sock)			\
>>   	BTF_SOCK_TYPE(BTF_SOCK_TYPE_UDP, udp_sock)			\
>>   	BTF_SOCK_TYPE(BTF_SOCK_TYPE_UDP6, udp6_sock)
>>   
>> +#define __BTF_SOCK_TW_TYPE_xxx \
>> +	BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_TW, inet_timewait_sock)	\
>> +	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP_TW, tcp_timewait_sock)
>> +
>> +#ifdef CONFIG_INET
>> +#define BTF_SOCK_TYPE_xxx						\
>> +	__BTF_SOCK_TYPE_xxx						\
>> +	__BTF_SOCK_TW_TYPE_xxx
>> +#else
>> +#define BTF_SOCK_TYPE_xxx	__BTF_SOCK_TYPE_xxx
> BTF_SOCK_TYPE_xxx is used in BTF_ID_LIST_GLOBAL(btf_sock_ids) in filter.c
> which does not include BTF_SOCK_TYPE_TCP_TW.
> However, btf_sock_ids[BTF_SOCK_TYPE_TCP_TW] is still used
> in bpf_skc_to_tcp_timewait_sock_proto.
> 
>> +#endif
>> +
>>   enum {
>>   #define BTF_SOCK_TYPE(name, str) name,
>> -BTF_SOCK_TYPE_xxx
>> +__BTF_SOCK_TYPE_xxx
>> +__BTF_SOCK_TW_TYPE_xxx
> BTF_SOCK_TYPE_TCP_TW is at the end of this enum.
> 
> Would btf_sock_ids[BTF_SOCK_TYPE_TCP_TW] always be 0?

No. If CONFIG_INET is y, the above BTF_SOCK_TYPE_xxx contains
   __BTF_SOCK_TW_TYPE_xxx
and
   btf_sock_ids[BTF_SOCK_TYPE_TCP_TW] will be calculated properly.

But if CONFIG_INET is n, then BTF_SOCK_TYPE_xxx will not contain
    __BTF_SOCK_TW_TYPE_xxx
so btf_sock_ids[BTF_SOCK_TYPE_TCP_TW] will have default value 0
as btf_sock_ids is a global.

Will send v2 to add some comments to make it easy to understand.

> 
>>   #undef BTF_SOCK_TYPE
>>   MAX_BTF_SOCK_TYPE,
>>   };
>> -- 
>> 2.24.1
>>
Martin KaFai Lau Sept. 30, 2020, 10:59 p.m. UTC | #6
On Wed, Sep 30, 2020 at 03:50:10PM -0700, Yonghong Song wrote:
> 
> 
> On 9/30/20 1:58 PM, Martin KaFai Lau wrote:
> > On Wed, Sep 30, 2020 at 09:41:09AM -0700, Yonghong Song wrote:
> > > Michal reported a build failure likes below:
> > >     BTFIDS  vmlinux
> > >     FAILED unresolved symbol tcp_timewait_sock
> > >     make[1]: *** [/.../linux-5.9-rc7/Makefile:1176: vmlinux] Error 255
> > > 
> > > This error can be triggered when config has CONFIG_NET enabled
> > > but CONFIG_INET disabled. In this case, there is no user of
> > > structs inet_timewait_sock and tcp_timewait_sock and hence vmlinux BTF
> > > types are not generated for these two structures.
> > > 
> > > To fix the problem, omit the above two types for BTF_SOCK_TYPE_xxx
> > > macro if CONFIG_INET is not defined.
> > > 
> > > Fixes: fce557bcef11 ("bpf: Make btf_sock_ids global")
> > > Reported-by: Michal Kubecek <mkubecek@suse.cz>
> > > Signed-off-by: Yonghong Song <yhs@fb.com>
> > > ---
> > >   include/linux/btf_ids.h | 20 ++++++++++++++++----
> > >   1 file changed, 16 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
> > > index 4867d549e3c1..d9a1e18d0921 100644
> > > --- a/include/linux/btf_ids.h
> > > +++ b/include/linux/btf_ids.h
> > > @@ -102,24 +102,36 @@ asm(							\
> > >    * skc_to_*_sock() helpers. All these sockets should have
> > >    * sock_common as the first argument in its memory layout.
> > >    */
> > > -#define BTF_SOCK_TYPE_xxx \
> > > +
> > > +#define __BTF_SOCK_TYPE_xxx \
> > >   	BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET, inet_sock)			\
> > >   	BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_CONN, inet_connection_sock)	\
> > >   	BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_REQ, inet_request_sock)	\
> > > -	BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_TW, inet_timewait_sock)	\
> > >   	BTF_SOCK_TYPE(BTF_SOCK_TYPE_REQ, request_sock)			\
> > >   	BTF_SOCK_TYPE(BTF_SOCK_TYPE_SOCK, sock)				\
> > >   	BTF_SOCK_TYPE(BTF_SOCK_TYPE_SOCK_COMMON, sock_common)		\
> > >   	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP, tcp_sock)			\
> > >   	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP_REQ, tcp_request_sock)		\
> > > -	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP_TW, tcp_timewait_sock)		\
> > >   	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP6, tcp6_sock)			\
> > >   	BTF_SOCK_TYPE(BTF_SOCK_TYPE_UDP, udp_sock)			\
> > >   	BTF_SOCK_TYPE(BTF_SOCK_TYPE_UDP6, udp6_sock)
> > > +#define __BTF_SOCK_TW_TYPE_xxx \
> > > +	BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_TW, inet_timewait_sock)	\
> > > +	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP_TW, tcp_timewait_sock)
> > > +
> > > +#ifdef CONFIG_INET
> > > +#define BTF_SOCK_TYPE_xxx						\
> > > +	__BTF_SOCK_TYPE_xxx						\
> > > +	__BTF_SOCK_TW_TYPE_xxx
> > > +#else
> > > +#define BTF_SOCK_TYPE_xxx	__BTF_SOCK_TYPE_xxx
> > BTF_SOCK_TYPE_xxx is used in BTF_ID_LIST_GLOBAL(btf_sock_ids) in filter.c
> > which does not include BTF_SOCK_TYPE_TCP_TW.
> > However, btf_sock_ids[BTF_SOCK_TYPE_TCP_TW] is still used
> > in bpf_skc_to_tcp_timewait_sock_proto.
> > 
> > > +#endif
> > > +
> > >   enum {
> > >   #define BTF_SOCK_TYPE(name, str) name,
> > > -BTF_SOCK_TYPE_xxx
> > > +__BTF_SOCK_TYPE_xxx
> > > +__BTF_SOCK_TW_TYPE_xxx
> > BTF_SOCK_TYPE_TCP_TW is at the end of this enum.
> > 
> > Would btf_sock_ids[BTF_SOCK_TYPE_TCP_TW] always be 0?
> 
> No. If CONFIG_INET is y, the above BTF_SOCK_TYPE_xxx contains
>   __BTF_SOCK_TW_TYPE_xxx
> and
>   btf_sock_ids[BTF_SOCK_TYPE_TCP_TW] will be calculated properly.
> 
> But if CONFIG_INET is n, then BTF_SOCK_TYPE_xxx will not contain
>    __BTF_SOCK_TW_TYPE_xxx
> so btf_sock_ids[BTF_SOCK_TYPE_TCP_TW] will have default value 0
> as btf_sock_ids is a global.
I could be missing something here.
Why btf_sock_ids[BTF_SOCK_TYPE_TCP_TW] must be 0?
How does BTF_ID_LIST_GLOBAL() know there is a
btf_sock_ids[BTF_SOCK_TYPE_TCP_TW]?
I would expect at least BTF_ID_UNUSED is required.
Otherwise, the value will be whatever follow the last
btf_sock_ids[BTF_SOCK_TYPE_UDP6].

> 
> Will send v2 to add some comments to make it easy to understand.
> 
> > 
> > >   #undef BTF_SOCK_TYPE
> > >   MAX_BTF_SOCK_TYPE,
> > >   };
> > > -- 
> > > 2.24.1
> > >
Yonghong Song Oct. 1, 2020, 1 a.m. UTC | #7
On 9/30/20 3:59 PM, Martin KaFai Lau wrote:
> On Wed, Sep 30, 2020 at 03:50:10PM -0700, Yonghong Song wrote:
>>
>>
>> On 9/30/20 1:58 PM, Martin KaFai Lau wrote:
>>> On Wed, Sep 30, 2020 at 09:41:09AM -0700, Yonghong Song wrote:
>>>> Michal reported a build failure likes below:
>>>>      BTFIDS  vmlinux
>>>>      FAILED unresolved symbol tcp_timewait_sock
>>>>      make[1]: *** [/.../linux-5.9-rc7/Makefile:1176: vmlinux] Error 255
>>>>
>>>> This error can be triggered when config has CONFIG_NET enabled
>>>> but CONFIG_INET disabled. In this case, there is no user of
>>>> structs inet_timewait_sock and tcp_timewait_sock and hence vmlinux BTF
>>>> types are not generated for these two structures.
>>>>
>>>> To fix the problem, omit the above two types for BTF_SOCK_TYPE_xxx
>>>> macro if CONFIG_INET is not defined.
>>>>
>>>> Fixes: fce557bcef11 ("bpf: Make btf_sock_ids global")
>>>> Reported-by: Michal Kubecek <mkubecek@suse.cz>
>>>> Signed-off-by: Yonghong Song <yhs@fb.com>
>>>> ---
>>>>    include/linux/btf_ids.h | 20 ++++++++++++++++----
>>>>    1 file changed, 16 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
>>>> index 4867d549e3c1..d9a1e18d0921 100644
>>>> --- a/include/linux/btf_ids.h
>>>> +++ b/include/linux/btf_ids.h
>>>> @@ -102,24 +102,36 @@ asm(							\
>>>>     * skc_to_*_sock() helpers. All these sockets should have
>>>>     * sock_common as the first argument in its memory layout.
>>>>     */
>>>> -#define BTF_SOCK_TYPE_xxx \
>>>> +
>>>> +#define __BTF_SOCK_TYPE_xxx \
>>>>    	BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET, inet_sock)			\
>>>>    	BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_CONN, inet_connection_sock)	\
>>>>    	BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_REQ, inet_request_sock)	\
>>>> -	BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_TW, inet_timewait_sock)	\
>>>>    	BTF_SOCK_TYPE(BTF_SOCK_TYPE_REQ, request_sock)			\
>>>>    	BTF_SOCK_TYPE(BTF_SOCK_TYPE_SOCK, sock)				\
>>>>    	BTF_SOCK_TYPE(BTF_SOCK_TYPE_SOCK_COMMON, sock_common)		\
>>>>    	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP, tcp_sock)			\
>>>>    	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP_REQ, tcp_request_sock)		\
>>>> -	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP_TW, tcp_timewait_sock)		\
>>>>    	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP6, tcp6_sock)			\
>>>>    	BTF_SOCK_TYPE(BTF_SOCK_TYPE_UDP, udp_sock)			\
>>>>    	BTF_SOCK_TYPE(BTF_SOCK_TYPE_UDP6, udp6_sock)
>>>> +#define __BTF_SOCK_TW_TYPE_xxx \
>>>> +	BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_TW, inet_timewait_sock)	\
>>>> +	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP_TW, tcp_timewait_sock)
>>>> +
>>>> +#ifdef CONFIG_INET
>>>> +#define BTF_SOCK_TYPE_xxx						\
>>>> +	__BTF_SOCK_TYPE_xxx						\
>>>> +	__BTF_SOCK_TW_TYPE_xxx
>>>> +#else
>>>> +#define BTF_SOCK_TYPE_xxx	__BTF_SOCK_TYPE_xxx
>>> BTF_SOCK_TYPE_xxx is used in BTF_ID_LIST_GLOBAL(btf_sock_ids) in filter.c
>>> which does not include BTF_SOCK_TYPE_TCP_TW.
>>> However, btf_sock_ids[BTF_SOCK_TYPE_TCP_TW] is still used
>>> in bpf_skc_to_tcp_timewait_sock_proto.
>>>
>>>> +#endif
>>>> +
>>>>    enum {
>>>>    #define BTF_SOCK_TYPE(name, str) name,
>>>> -BTF_SOCK_TYPE_xxx
>>>> +__BTF_SOCK_TYPE_xxx
>>>> +__BTF_SOCK_TW_TYPE_xxx
>>> BTF_SOCK_TYPE_TCP_TW is at the end of this enum.
>>>
>>> Would btf_sock_ids[BTF_SOCK_TYPE_TCP_TW] always be 0?
>>
>> No. If CONFIG_INET is y, the above BTF_SOCK_TYPE_xxx contains
>>    __BTF_SOCK_TW_TYPE_xxx
>> and
>>    btf_sock_ids[BTF_SOCK_TYPE_TCP_TW] will be calculated properly.
>>
>> But if CONFIG_INET is n, then BTF_SOCK_TYPE_xxx will not contain
>>     __BTF_SOCK_TW_TYPE_xxx
>> so btf_sock_ids[BTF_SOCK_TYPE_TCP_TW] will have default value 0
>> as btf_sock_ids is a global.
> I could be missing something here.
> Why btf_sock_ids[BTF_SOCK_TYPE_TCP_TW] must be 0?
> How does BTF_ID_LIST_GLOBAL() know there is a
> btf_sock_ids[BTF_SOCK_TYPE_TCP_TW]?
> I would expect at least BTF_ID_UNUSED is required.
> Otherwise, the value will be whatever follow the last
> btf_sock_ids[BTF_SOCK_TYPE_UDP6].

Right, I realized later on when I prepare v2. Will send patch v2 shortly.

> 
>>
>> Will send v2 to add some comments to make it easy to understand.
>>
>>>
>>>>    #undef BTF_SOCK_TYPE
>>>>    MAX_BTF_SOCK_TYPE,
>>>>    };
>>>> -- 
>>>> 2.24.1
>>>>
diff mbox series

Patch

diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
index 4867d549e3c1..d9a1e18d0921 100644
--- a/include/linux/btf_ids.h
+++ b/include/linux/btf_ids.h
@@ -102,24 +102,36 @@  asm(							\
  * skc_to_*_sock() helpers. All these sockets should have
  * sock_common as the first argument in its memory layout.
  */
-#define BTF_SOCK_TYPE_xxx \
+
+#define __BTF_SOCK_TYPE_xxx \
 	BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET, inet_sock)			\
 	BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_CONN, inet_connection_sock)	\
 	BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_REQ, inet_request_sock)	\
-	BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_TW, inet_timewait_sock)	\
 	BTF_SOCK_TYPE(BTF_SOCK_TYPE_REQ, request_sock)			\
 	BTF_SOCK_TYPE(BTF_SOCK_TYPE_SOCK, sock)				\
 	BTF_SOCK_TYPE(BTF_SOCK_TYPE_SOCK_COMMON, sock_common)		\
 	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP, tcp_sock)			\
 	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP_REQ, tcp_request_sock)		\
-	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP_TW, tcp_timewait_sock)		\
 	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP6, tcp6_sock)			\
 	BTF_SOCK_TYPE(BTF_SOCK_TYPE_UDP, udp_sock)			\
 	BTF_SOCK_TYPE(BTF_SOCK_TYPE_UDP6, udp6_sock)
 
+#define __BTF_SOCK_TW_TYPE_xxx \
+	BTF_SOCK_TYPE(BTF_SOCK_TYPE_INET_TW, inet_timewait_sock)	\
+	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP_TW, tcp_timewait_sock)
+
+#ifdef CONFIG_INET
+#define BTF_SOCK_TYPE_xxx						\
+	__BTF_SOCK_TYPE_xxx						\
+	__BTF_SOCK_TW_TYPE_xxx
+#else
+#define BTF_SOCK_TYPE_xxx	__BTF_SOCK_TYPE_xxx
+#endif
+
 enum {
 #define BTF_SOCK_TYPE(name, str) name,
-BTF_SOCK_TYPE_xxx
+__BTF_SOCK_TYPE_xxx
+__BTF_SOCK_TW_TYPE_xxx
 #undef BTF_SOCK_TYPE
 MAX_BTF_SOCK_TYPE,
 };