From owner-dev-commits-src-all@freebsd.org Mon Feb 22 23:38:11 2021 Return-Path: Delivered-To: dev-commits-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 4C422552DFB; Mon, 22 Feb 2021 23:38:11 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4DkzBq1gpmz3Dg4; Mon, 22 Feb 2021 23:38:11 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 21B116BAE; Mon, 22 Feb 2021 23:38:11 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 11MNcBRw084740; Mon, 22 Feb 2021 23:38:11 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 11MNcBem084739; Mon, 22 Feb 2021 23:38:11 GMT (envelope-from git) Date: Mon, 22 Feb 2021 23:38:11 GMT Message-Id: <202102222338.11MNcBem084739@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: "Alexander V. Chernikov" Subject: git: 7563019bc693 - main - Add if_try_ref() to simplify refcount handling inside epoch. MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: melifaro X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: 7563019bc69301a382abefbac3b0fea1d876410e Auto-Submitted: auto-generated X-BeenThere: dev-commits-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for all branches of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 22 Feb 2021 23:38:11 -0000 The branch main has been updated by melifaro: URL: https://cgit.FreeBSD.org/src/commit/?id=7563019bc69301a382abefbac3b0fea1d876410e commit 7563019bc69301a382abefbac3b0fea1d876410e Author: Alexander V. Chernikov AuthorDate: 2021-02-22 21:37:55 +0000 Commit: Alexander V. Chernikov CommitDate: 2021-02-22 23:37:59 +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 --- 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 948be6876b65..9d5e9e26b4bb 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 *);