[ovs-dev] compiler: Fix errors in Clang 17 ubsan checks.

This patch attempts to fix a large number of ubsan error messages that
take the following form:

lib/netlink-notifier.c:237:13: runtime error: call to function
    route_table_change through pointer to incorrect function type
    'void (*)(const void *, void *)'

In Clang 17 the undefined behaviour sanatizer check for function
pointers was enabled by default, whereas it was previously disabled
while compiling C code. These warnings are a false positive in the case
of OVS, as our macros already check to make sure the function parameter
is the correct size.

So that check is disabled in the single function that is causing all of
the errors.

Signed-off-by: Mike Pattrick <mkp@redhat.com>
---
 include/openvswitch/compiler.h | 11 +++++++++++
 lib/ovs-rcu.c                  |  1 +
 2 files changed, 12 insertions(+)

The github ci failed with entries like this in the logs:

+tcp,orig=(src=10.1.1.4,dst=140.82.113.22,sport=<cleared>,dport=<cleared>),reply=(src=140.82.113.22,dst=10.1.1.4,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>)
+tcp,orig=(src=10.1.1.4,dst=168.63.129.16,sport=<cleared>,dport=<cleared>),reply=(src=168.63.129.16,dst=10.1.1.4,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>)
+tcp,orig=(src=10.1.1.4,dst=20.22.166.15,sport=<cleared>,dport=<cleared>),reply=(src=20.22.166.15,dst=10.1.1.4,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>)

Those are github IP's, don't know how github traffic got into the tests.

-M

On Fri, Jan 19, 2024 at 11:13 AM Mike Pattrick <mkp@redhat.com> wrote:
>
> This patch attempts to fix a large number of ubsan error messages that
> take the following form:
>
> lib/netlink-notifier.c:237:13: runtime error: call to function
>     route_table_change through pointer to incorrect function type
>     'void (*)(const void *, void *)'
>
> In Clang 17 the undefined behaviour sanatizer check for function
> pointers was enabled by default, whereas it was previously disabled
> while compiling C code. These warnings are a false positive in the case
> of OVS, as our macros already check to make sure the function parameter
> is the correct size.
>
> So that check is disabled in the single function that is causing all of
> the errors.
>
> Signed-off-by: Mike Pattrick <mkp@redhat.com>
> ---
>  include/openvswitch/compiler.h | 11 +++++++++++
>  lib/ovs-rcu.c                  |  1 +
>  2 files changed, 12 insertions(+)
>
> diff --git a/include/openvswitch/compiler.h b/include/openvswitch/compiler.h
> index 878c5c6a7..59a835f15 100644
> --- a/include/openvswitch/compiler.h
> +++ b/include/openvswitch/compiler.h
> @@ -69,6 +69,17 @@
>  #define OVS_UNLIKELY(CONDITION) (!!(CONDITION))
>  #endif
>
> +/* Clang 17's implementation of ubsan enables checking that function pointers
> + * match the type of the called function. This currently breaks ovs-rcu, which
> + * calls multiple different types of callbacks via a generic void *(void*)
> + * function pointer type. This macro enables disabling that check for specific
> + * functions. */
> +#if __clang__ && __has_feature(undefined_behavior_sanitizer)
> +#define OVS_FUNCTION_PTR __attribute__((no_sanitize("function")))
> +#else
> +#define OVS_FUNCTION_PTR
> +#endif
> +
>  #if __has_feature(c_thread_safety_attributes)
>  /* "clang" annotations for thread safety check.
>   *
> diff --git a/lib/ovs-rcu.c b/lib/ovs-rcu.c
> index 9e07d9bab..56608aa46 100644
> --- a/lib/ovs-rcu.c
> +++ b/lib/ovs-rcu.c
> @@ -327,6 +327,7 @@ ovsrcu_postpone__(void (*function)(void *aux), void *aux)
>  }
>
>  static bool
> +OVS_FUNCTION_PTR
>  ovsrcu_call_postponed(void)
>  {
>      struct ovsrcu_cbset *cbset;
> --
> 2.39.3
>

On Fri, Jan 19, 2024 at 02:02:04PM -0500, Mike Pattrick wrote:
> The github ci failed with entries like this in the logs:
> 
> +tcp,orig=(src=10.1.1.4,dst=140.82.113.22,sport=<cleared>,dport=<cleared>),reply=(src=140.82.113.22,dst=10.1.1.4,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>)
> +tcp,orig=(src=10.1.1.4,dst=168.63.129.16,sport=<cleared>,dport=<cleared>),reply=(src=168.63.129.16,dst=10.1.1.4,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>)
> +tcp,orig=(src=10.1.1.4,dst=20.22.166.15,sport=<cleared>,dport=<cleared>),reply=(src=20.22.166.15,dst=10.1.1.4,sport=<cleared>,dport=<cleared>),protoinfo=(state=<cleared>)
> 
> Those are github IP's, don't know how github traffic got into the tests.

Ilya says that this is because

  "GitHub runners use 10.1.0.0/16 network as their base network for eth0
   interface.  That is causing random system test failures when unexpected
   conntrack entries for this network are present, because our system
   tests are mainly using 10.1.1.0/24 subnet for their test networks.

The link below is for a proposed fix for this problem.

[1] https://patchwork.ozlabs.org/project/openvswitch/patch/20240122172307.3801928-1-i.maximets@ovn.org/

On 19 Jan 2024, at 17:12, Mike Pattrick wrote:

> This patch attempts to fix a large number of ubsan error messages that
> take the following form:
>
> lib/netlink-notifier.c:237:13: runtime error: call to function
>     route_table_change through pointer to incorrect function type
>     'void (*)(const void *, void *)'
>
> In Clang 17 the undefined behaviour sanatizer check for function
> pointers was enabled by default, whereas it was previously disabled
> while compiling C code. These warnings are a false positive in the case
> of OVS, as our macros already check to make sure the function parameter
> is the correct size.
>
> So that check is disabled in the single function that is causing all of
> the errors.
>
> Signed-off-by: Mike Pattrick <mkp@redhat.com>

The idea is good, but the macro naming and useage is a bit confusing.


> ---
>  include/openvswitch/compiler.h | 11 +++++++++++
>  lib/ovs-rcu.c                  |  1 +
>  2 files changed, 12 insertions(+)
>
> diff --git a/include/openvswitch/compiler.h b/include/openvswitch/compiler.h
> index 878c5c6a7..59a835f15 100644
> --- a/include/openvswitch/compiler.h
> +++ b/include/openvswitch/compiler.h
> @@ -69,6 +69,17 @@
>  #define OVS_UNLIKELY(CONDITION) (!!(CONDITION))
>  #endif
>
> +/* Clang 17's implementation of ubsan enables checking that function pointers
> + * match the type of the called function. This currently breaks ovs-rcu, which
> + * calls multiple different types of callbacks via a generic void *(void*)
> + * function pointer type. This macro enables disabling that check for specific
> + * functions. */
> +#if __clang__ && __has_feature(undefined_behavior_sanitizer)
> +#define OVS_FUNCTION_PTR __attribute__((no_sanitize("function")))
> +#else
> +#define OVS_FUNCTION_PTR
> +#endif

The name seems to suggest that the ovsrcu_call_postponed() function is some kind of function pointer.

Maybe we should name the macro something like OVS_NO_SANITIZE_FUNCTION. The kernel has similar functions named __no_sanitize_XXX.

> +
>  #if __has_feature(c_thread_safety_attributes)
>  /* "clang" annotations for thread safety check.
>   *
> diff --git a/lib/ovs-rcu.c b/lib/ovs-rcu.c
> index 9e07d9bab..56608aa46 100644
> --- a/lib/ovs-rcu.c
> +++ b/lib/ovs-rcu.c
> @@ -327,6 +327,7 @@ ovsrcu_postpone__(void (*function)(void *aux), void *aux)
>  }
>
>  static bool
> +OVS_FUNCTION_PTR
>  ovsrcu_call_postponed(void)
>  {
>      struct ovsrcu_cbset *cbset;
> -- 
> 2.39.3
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev