[PATCH bpf-next] libbpf: synchronize access to print function pointer

[PATCH bpf-next] libbpf: synchronize access to print function pointer

[JP Kobryn]

This patch prevents races on the print function pointer, allowing the
libbpf_set_print() function to become thread safe.

Signed-off-by: JP Kobryn <inwardvessel@gmail.com>
---
 tools/lib/bpf/libbpf.c | 9 ++++++---
 tools/lib/bpf/libbpf.h | 3 +++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index f6a071db5c6e..15737d7b5a28 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -216,9 +216,10 @@ static libbpf_print_fn_t __libbpf_pr = __base_pr;
 
 libbpf_print_fn_t libbpf_set_print(libbpf_print_fn_t fn)
 {
-	libbpf_print_fn_t old_print_fn = __libbpf_pr;
+	libbpf_print_fn_t old_print_fn;
+
+	old_print_fn = __atomic_exchange_n(&__libbpf_pr, fn, __ATOMIC_RELAXED);
 
-	__libbpf_pr = fn;
 	return old_print_fn;
 }
 
@@ -227,8 +228,10 @@ void libbpf_print(enum libbpf_print_level level, const char *format, ...)
 {
 	va_list args;
 	int old_errno;
+	libbpf_print_fn_t print_fn;
 
-	if (!__libbpf_pr)
+	print_fn = __atomic_load_n(&__libbpf_pr, __ATOMIC_RELAXED);
+	if (!print_fn)
 		return;
 
 	old_errno = errno;
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 1615e55e2e79..4478809ff9ca 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -99,6 +99,9 @@ typedef int (*libbpf_print_fn_t)(enum libbpf_print_level level,
 /**
  * @brief **libbpf_set_print()** sets user-provided log callback function to
  * be used for libbpf warnings and informational messages.
+ *
+ * This function is thread safe.
+ *
  * @param fn The log print function. If NULL, libbpf won't print anything.
  * @return Pointer to old print function.
  */
-- 
2.39.2

Re: [PATCH bpf-next] libbpf: synchronize access to print function pointer

[Stanislav Fomichev]

On 03/24, JP Kobryn wrote:
> This patch prevents races on the print function pointer, allowing the
> libbpf_set_print() function to become thread safe.

Why does it have to be thread-safe? The rest of the APIs aren't, so
why can't use solve it on your side by wrapping those calls with a
mutex?

(is there some context I'm missing?)

> Signed-off-by: JP Kobryn <inwardvessel@gmail.com>
> ---
>   tools/lib/bpf/libbpf.c | 9 ++++++---
>   tools/lib/bpf/libbpf.h | 3 +++
>   2 files changed, 9 insertions(+), 3 deletions(-)

> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index f6a071db5c6e..15737d7b5a28 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -216,9 +216,10 @@ static libbpf_print_fn_t __libbpf_pr = __base_pr;

>   libbpf_print_fn_t libbpf_set_print(libbpf_print_fn_t fn)
>   {
> -	libbpf_print_fn_t old_print_fn = __libbpf_pr;
> +	libbpf_print_fn_t old_print_fn;
> +
> +	old_print_fn = __atomic_exchange_n(&__libbpf_pr, fn, __ATOMIC_RELAXED);

> -	__libbpf_pr = fn;
>   	return old_print_fn;
>   }

> @@ -227,8 +228,10 @@ void libbpf_print(enum libbpf_print_level level,  
> const char *format, ...)
>   {
>   	va_list args;
>   	int old_errno;
> +	libbpf_print_fn_t print_fn;

> -	if (!__libbpf_pr)
> +	print_fn = __atomic_load_n(&__libbpf_pr, __ATOMIC_RELAXED);
> +	if (!print_fn)
>   		return;

>   	old_errno = errno;
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 1615e55e2e79..4478809ff9ca 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -99,6 +99,9 @@ typedef int (*libbpf_print_fn_t)(enum  
> libbpf_print_level level,
>   /**
>    * @brief **libbpf_set_print()** sets user-provided log callback  
> function to
>    * be used for libbpf warnings and informational messages.
> + *
> + * This function is thread safe.
> + *
>    * @param fn The log print function. If NULL, libbpf won't print  
> anything.
>    * @return Pointer to old print function.
>    */
> --
> 2.39.2

Re: [PATCH bpf-next] libbpf: synchronize access to print function pointer

[Andrii Nakryiko]

On Fri, Mar 24, 2023 at 8:09 PM Stanislav Fomichev <sdf@google.com> wrote:
>
> On 03/24, JP Kobryn wrote:
> > This patch prevents races on the print function pointer, allowing the
> > libbpf_set_print() function to become thread safe.
>
> Why does it have to be thread-safe? The rest of the APIs aren't, so

It doesn't have to, but if it can be made thread-safe trivially, it
probably should be. Rust users are especially sensitive to this (see
discussion in [0] for example).

Generally speaking, yep, libbpf APIs are not thread-safe by default
(we don't do explicit locking anywhere inside libbpf), but there are a
bunch of APIs that are inherently thread-safe as they are stateless
(like feature probing, string conversion APIs, I suspect upon
inspection we'll see that all bpf_program__attach_*() APIs are also
probably thread-safe already). So we should start marking them as such
to avoid confusion and uncertainty for users. I'd also like to
document somewhere that two independent bpf_objects can be used safely
on two separate threads, because whatever state they are sharing (like
feature detection cache) is designed in such a way to be thread-safe
and shareable with no locking.

  [0] https://github.com/libbpf/libbpf-rs/pull/374#issuecomment-1462565778

> why can't use solve it on your side by wrapping those calls with a
> mutex?

It would be very unfortunate to wrap libbpf_set_print and *all other
libbpf API* in mutex.

>
> (is there some context I'm missing?)
>
> > Signed-off-by: JP Kobryn <inwardvessel@gmail.com>
> > ---
> >   tools/lib/bpf/libbpf.c | 9 ++++++---
> >   tools/lib/bpf/libbpf.h | 3 +++
> >   2 files changed, 9 insertions(+), 3 deletions(-)
>
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index f6a071db5c6e..15737d7b5a28 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -216,9 +216,10 @@ static libbpf_print_fn_t __libbpf_pr = __base_pr;
>
> >   libbpf_print_fn_t libbpf_set_print(libbpf_print_fn_t fn)
> >   {
> > -     libbpf_print_fn_t old_print_fn = __libbpf_pr;
> > +     libbpf_print_fn_t old_print_fn;
> > +
> > +     old_print_fn = __atomic_exchange_n(&__libbpf_pr, fn, __ATOMIC_RELAXED);
>
> > -     __libbpf_pr = fn;
> >       return old_print_fn;
> >   }
>
> > @@ -227,8 +228,10 @@ void libbpf_print(enum libbpf_print_level level,
> > const char *format, ...)
> >   {
> >       va_list args;
> >       int old_errno;
> > +     libbpf_print_fn_t print_fn;
>
> > -     if (!__libbpf_pr)
> > +     print_fn = __atomic_load_n(&__libbpf_pr, __ATOMIC_RELAXED);
> > +     if (!print_fn)
> >               return;
>
> >       old_errno = errno;
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 1615e55e2e79..4478809ff9ca 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -99,6 +99,9 @@ typedef int (*libbpf_print_fn_t)(enum
> > libbpf_print_level level,
> >   /**
> >    * @brief **libbpf_set_print()** sets user-provided log callback
> > function to
> >    * be used for libbpf warnings and informational messages.
> > + *
> > + * This function is thread safe.
> > + *
> >    * @param fn The log print function. If NULL, libbpf won't print
> > anything.
> >    * @return Pointer to old print function.
> >    */
> > --
> > 2.39.2
>

Re: [PATCH bpf-next] libbpf: synchronize access to print function pointer

[Andrii Nakryiko]

On Fri, Mar 24, 2023 at 6:09 PM JP Kobryn <inwardvessel@gmail.com> wrote:
>
> This patch prevents races on the print function pointer, allowing the
> libbpf_set_print() function to become thread safe.
>
> Signed-off-by: JP Kobryn <inwardvessel@gmail.com>
> ---

I reworked the patch subject to "libbpf: Ensure print callback usage
is thread-safe", it felt more on point. Also changed to "thread-safe",
which seems to be a proper spelling for this term.

Applied to bpf-next, thanks!

>  tools/lib/bpf/libbpf.c | 9 ++++++---
>  tools/lib/bpf/libbpf.h | 3 +++
>  2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index f6a071db5c6e..15737d7b5a28 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -216,9 +216,10 @@ static libbpf_print_fn_t __libbpf_pr = __base_pr;
>
>  libbpf_print_fn_t libbpf_set_print(libbpf_print_fn_t fn)
>  {
> -       libbpf_print_fn_t old_print_fn = __libbpf_pr;
> +       libbpf_print_fn_t old_print_fn;
> +
> +       old_print_fn = __atomic_exchange_n(&__libbpf_pr, fn, __ATOMIC_RELAXED);
>
> -       __libbpf_pr = fn;
>         return old_print_fn;
>  }
>
> @@ -227,8 +228,10 @@ void libbpf_print(enum libbpf_print_level level, const char *format, ...)
>  {
>         va_list args;
>         int old_errno;
> +       libbpf_print_fn_t print_fn;
>
> -       if (!__libbpf_pr)
> +       print_fn = __atomic_load_n(&__libbpf_pr, __ATOMIC_RELAXED);
> +       if (!print_fn)
>                 return;
>
>         old_errno = errno;
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 1615e55e2e79..4478809ff9ca 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -99,6 +99,9 @@ typedef int (*libbpf_print_fn_t)(enum libbpf_print_level level,
>  /**
>   * @brief **libbpf_set_print()** sets user-provided log callback function to
>   * be used for libbpf warnings and informational messages.
> + *
> + * This function is thread safe.
> + *

I moved this part to after @return spec, where details about functions
are normally put.

>   * @param fn The log print function. If NULL, libbpf won't print anything.
>   * @return Pointer to old print function.
>   */
> --
> 2.39.2
>

Re: [PATCH bpf-next] libbpf: synchronize access to print function pointer

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Fri, 24 Mar 2023 18:08:45 -0700 you wrote:
> This patch prevents races on the print function pointer, allowing the
> libbpf_set_print() function to become thread safe.
> 
> Signed-off-by: JP Kobryn <inwardvessel@gmail.com>
> ---
>  tools/lib/bpf/libbpf.c | 9 ++++++---
>  tools/lib/bpf/libbpf.h | 3 +++
>  2 files changed, 9 insertions(+), 3 deletions(-)

Here is the summary with links:
  - [bpf-next] libbpf: synchronize access to print function pointer
    https://git.kernel.org/bpf/bpf-next/c/f1cb927cdb62

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html