git: 600eade2fb4f - main - Add ifa_try_ref() to simplify ifa handling inside epoch.

[Hans Petter Selasky]

On 2/16/21 9:18 PM, Alexander V. Chernikov wrote:
> The branch main has been updated by melifaro:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=600eade2fb4faacfcd408a01140ef15f85f0c817
> 
> commit 600eade2fb4faacfcd408a01140ef15f85f0c817
> Author:     Alexander V. Chernikov <melifaro at FreeBSD.org>
> AuthorDate: 2021-02-16 20:12:58 +0000
> Commit:     Alexander V. Chernikov <melifaro at FreeBSD.org>
> CommitDate: 2021-02-16 20:14:50 +0000
> 
>      Add ifa_try_ref() to simplify ifa handling inside epoch.
>      
>      More and more code migrates from lock-based protection to the NET_EPOCH
>       umbrella. It requires some logic changes, including, notably, refcount
>       handling.
>      
>      When we have an `ifa` pointer and we're running inside epoch we're
>       guaranteed that this pointer will not be freed.
>      However, the following case can still happen:
>       * in thread 1 we drop to 0 refcount for ifa and schedule its deletion.
>       * in thread 2 we use this ifa and reference it
>       * destroy callout kicks in
>       * unhappy user reports bug
>      
>      To address it, new `ifa_try_ref()` function is added, allowing to return
>       failure when we try to reference `ifa` with 0 refcount.
>      Additionally, existing `ifa_ref()` is enforced with `KASSERT` to provide
>       cleaner error in such scenarious.
>      
>      Reviewed By: rstone, donner
>      Differential Revision: https://reviews.freebsd.org/D28639
>      MFC after:      1 week
> ---
>   sys/net/if.c     | 12 +++++++++++-
>   sys/net/if_var.h |  1 +
>   2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/sys/net/if.c b/sys/net/if.c
> index c85cfab19bf6..948be6876b65 100644
> --- a/sys/net/if.c
> +++ b/sys/net/if.c
> @@ -1857,8 +1857,18 @@ fail:
>   void
>   ifa_ref(struct ifaddr *ifa)
>   {
> +	u_int old;
>   
> -	refcount_acquire(&ifa->ifa_refcnt);
> +	old = refcount_acquire(&ifa->ifa_refcnt);
> +	KASSERT(old > 0, ("%s: ifa %p has 0 refs", __func__, ifa));
> +}
> +
> +int
> +ifa_try_ref(struct ifaddr *ifa)
> +{
> +
> +	NET_EPOCH_ASSERT();
> +	return (refcount_acquire_if_not_zero(&ifa->ifa_refcnt));
>   }
>   
>   static void
> diff --git a/sys/net/if_var.h b/sys/net/if_var.h
> index 9ecdfb684296..291a7781d73c 100644
> --- a/sys/net/if_var.h
> +++ b/sys/net/if_var.h
> @@ -577,6 +577,7 @@ struct ifaddr {
>   struct ifaddr *	ifa_alloc(size_t size, int flags);
>   void	ifa_free(struct ifaddr *ifa);
>   void	ifa_ref(struct ifaddr *ifa);
> +int	ifa_try_ref(struct ifaddr *ifa);

I think you should add __result_use_check for this prototype!?

--HPS