From owner-dev-commits-src-main@freebsd.org Sat Mar 6 10:33:30 2021 Return-Path: Delivered-To: dev-commits-src-main@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 1C2CF560DD1; Sat, 6 Mar 2021 10:33:30 +0000 (UTC) (envelope-from hps@selasky.org) Received: from mail.turbocat.net (turbocat.net [88.99.82.50]) (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 mx1.freebsd.org (Postfix) with ESMTPS id 4Dt1Cs14kbz3hGr; Sat, 6 Mar 2021 10:33:28 +0000 (UTC) (envelope-from hps@selasky.org) Received: from hps2020.home.selasky.org (unknown [178.17.145.105]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mail.turbocat.net (Postfix) with ESMTPSA id 29201260202; Sat, 6 Mar 2021 11:33:21 +0100 (CET) Subject: Re: git: 600eade2fb4f - main - Add ifa_try_ref() to simplify ifa handling inside epoch. To: "Alexander V. Chernikov" , src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org References: <202102162018.11GKIs1U039108@gitrepo.freebsd.org> From: Hans Petter Selasky Message-ID: <938ca724-5f45-1d94-c22e-df348abdd030@selasky.org> Date: Sat, 6 Mar 2021 11:33:03 +0100 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:78.0) Gecko/20100101 Thunderbird/78.5.0 MIME-Version: 1.0 In-Reply-To: <202102162018.11GKIs1U039108@gitrepo.freebsd.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: 4Dt1Cs14kbz3hGr X-Spamd-Bar: --- Authentication-Results: mx1.freebsd.org; dkim=none; dmarc=none; spf=pass (mx1.freebsd.org: domain of hps@selasky.org designates 88.99.82.50 as permitted sender) smtp.mailfrom=hps@selasky.org X-Spamd-Result: default: False [-3.25 / 15.00]; RCVD_TLS_ALL(0.00)[]; RCVD_VIA_SMTP_AUTH(0.00)[]; MID_RHS_MATCH_FROM(0.00)[]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_THREE(0.00)[4]; TO_DN_SOME(0.00)[]; TO_MATCH_ENVRCPT_ALL(0.00)[]; MIME_GOOD(-0.10)[text/plain]; R_SPF_ALLOW(-0.20)[+a:mail.turbocat.net]; ARC_NA(0.00)[]; DMARC_NA(0.00)[selasky.org]; SPAMHAUS_ZRD(0.00)[88.99.82.50:from:127.0.2.255]; NEURAL_HAM_MEDIUM(-1.00)[-1.000]; RBL_DBL_DONT_QUERY_IPS(0.00)[88.99.82.50:from]; NEURAL_HAM_LONG(-1.00)[-1.000]; NEURAL_HAM_SHORT(-0.95)[-0.948]; FROM_EQ_ENVFROM(0.00)[]; R_DKIM_NA(0.00)[]; MIME_TRACE(0.00)[0:+]; ASN(0.00)[asn:24940, ipnet:88.99.0.0/16, country:DE]; RCVD_COUNT_TWO(0.00)[2]; MAILMAN_DEST(0.00)[dev-commits-src-all,dev-commits-src-main] X-BeenThere: dev-commits-src-main@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for the main branch of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 06 Mar 2021 10:33:30 -0000 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 > AuthorDate: 2021-02-16 20:12:58 +0000 > Commit: Alexander V. Chernikov > 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