Date: Tue, 7 Sep 2021 21:12:39 GMT From: "Alexander V. Chernikov" <melifaro@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org Subject: git: 04e967d7270e - stable/13 - Add if_try_ref() to simplify refcount handling inside epoch. Message-ID: <202109072112.187LCdNE023651@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch stable/13 has been updated by melifaro: URL: https://cgit.FreeBSD.org/src/commit/?id=04e967d7270eacada2075906cca9a03043a9609a commit 04e967d7270eacada2075906cca9a03043a9609a Author: Alexander V. Chernikov <melifaro@FreeBSD.org> AuthorDate: 2021-02-22 21:37:55 +0000 Commit: Alexander V. Chernikov <melifaro@FreeBSD.org> CommitDate: 2021-09-07 20:55:51 +0000 Add if_try_ref() to simplify refcount handling inside epoch. When we have an ifp pointer and the code is running inside epoch, epoch guarantees the pointer will not be freed. However, the following case can still happen: * in thread 1 we drop to refcount=0 for ifp and schedule its deletion. * in thread 2 we use this ifp and reference it * destroy callout kicks in * unhappy user reports a bug This can happen with the current implementation of ifnet_byindex_ref(), as we're not holding any locks preventing ifnet deletion by a parallel thread. To address it, add if_try_ref(), allowing to return failure when referencing ifp with refcount=0. Additionally, enforce existing if_ref() is with KASSERT to provide a cleaner error in such scenarios. Finally, fix ifnet_byindex_ref() by using if_try_ref() and returning NULL if the latter fails. MFC after: 2 weeks Differential Revision: https://reviews.freebsd.org/D28836 (cherry picked from commit 7563019bc69301a382abefbac3b0fea1d876410e) --- sys/net/if.c | 14 ++++++++++++-- sys/net/if_var.h | 1 + 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/sys/net/if.c b/sys/net/if.c index f6926c43ef96..aeb7230fcc9a 100644 --- a/sys/net/if.c +++ b/sys/net/if.c @@ -359,7 +359,8 @@ ifnet_byindex_ref(u_short idx) ifp = ifnet_byindex(idx); if (ifp == NULL || (ifp->if_flags & IFF_DYING)) return (NULL); - if_ref(ifp); + if (!if_try_ref(ifp)) + return (NULL); return (ifp); } @@ -738,9 +739,18 @@ if_free(struct ifnet *ifp) void if_ref(struct ifnet *ifp) { + u_int old; /* We don't assert the ifnet list lock here, but arguably should. */ - refcount_acquire(&ifp->if_refcount); + old = refcount_acquire(&ifp->if_refcount); + KASSERT(old > 0, ("%s: ifp %p has 0 refs", __func__, ifp)); +} + +bool +if_try_ref(struct ifnet *ifp) +{ + NET_EPOCH_ASSERT(); + return (refcount_acquire_if_not_zero(&ifp->if_refcount)); } void diff --git a/sys/net/if_var.h b/sys/net/if_var.h index 291a7781d73c..33a737880a8d 100644 --- a/sys/net/if_var.h +++ b/sys/net/if_var.h @@ -661,6 +661,7 @@ void if_link_state_change(struct ifnet *, int); int if_printf(struct ifnet *, const char *, ...) __printflike(2, 3); void if_ref(struct ifnet *); void if_rele(struct ifnet *); +bool if_try_ref(struct ifnet *); int if_setlladdr(struct ifnet *, const u_char *, int); int if_tunnel_check_nesting(struct ifnet *, struct mbuf *, uint32_t, int); void if_up(struct ifnet *);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202109072112.187LCdNE023651>