From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9936419E992 for ; Thu, 20 Aug 2026 18:09:17 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.129.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787249359; cv=none; b=S18IUbt5nbGorbKiaTUDF8fgvARfFp+2hdkWEZAIyQweSHLDw7h42Az2mN8TjKArvIPQD7qUece4bWmtRJ/Zf3tyjLJjPwRKWSO+e4ncfexqzeuRqyd3oqB49eAMwXNApNTHmLSMyyCgu4NC4dSSjCzwHuZ4t2aCnxsA+K2mMA8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787249359; c=relaxed/simple; bh=9Qnpj4ijBejVpUg8k3p0sWZJjP/4k4ONXM8HSBI1bYE=; h=Message-ID:Subject:From:To:Cc:Date:In-Reply-To:References: Content-Type:MIME-Version; b=B21oMsmP6wocKh1oFabBLIigohTghei7DDE1m1tnn46CGlE6CWgP7CrYlO/2qK4NP6oAsmwTDmA0wVfufS2hFnb5C2sayquKXG7Zxc0N89ROuhZIu9/WNTI7I13iM3NoyXCP26u+NEOEP2xc6QdxmPhOtA/UJvhX2iqNQV1Np80= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=gCPMwqfv; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b=LXyZST41; arc=none smtp.client-ip=170.10.129.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="gCPMwqfv"; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b="LXyZST41" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1787249356; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=/D29wRD0UC/Hr8ZPGocWQ9UTN+csrkFSPeqNFH7Hdo4=; b=gCPMwqfvDC8btzXwHzf0zgGq5S/574m9h8w5GS/GHN7zCK+LN+Zzsn/K4jDq4BHYdh4AOg xVlVI3JR2gu2Yz8uQSSB91GRT+EZwasNKDlMdv/bJTpalAGVwFTbas3Y4H1FG+x6kVF6EY 9b9loWkrdNonUSXfVX/L/nOPjo7cPHY= Received: from mail-qv1-f70.google.com (mail-qv1-f70.google.com [209.85.219.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-413-KyV-J9ucMQC0q-0ZrEG1IA-1; Thu, 20 Aug 2026 14:09:05 -0400 X-MC-Unique: KyV-J9ucMQC0q-0ZrEG1IA-1 X-Mimecast-MFC-AGG-ID: KyV-J9ucMQC0q-0ZrEG1IA_1787249344 Received: by mail-qv1-f70.google.com with SMTP id 6a1803df08f44-9077b4c35c5so2698296d6.2 for ; Thu, 20 Aug 2026 11:09:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=google; t=1787249344; x=1787854144; darn=vger.kernel.org; h=mime-version:user-agent:content-transfer-encoding:content-type :references:in-reply-to:date:cc:to:from:subject:message-id:from:to :cc:subject:date:message-id:reply-to:content-type; bh=/D29wRD0UC/Hr8ZPGocWQ9UTN+csrkFSPeqNFH7Hdo4=; b=LXyZST41YkSaufxqO/KrRuSFitEvParmgbbzYBr8NW57T+At90DVNSEwQIZaKRd+wU mn0Wjh1Wv0DnP4yfV39TsvDXseIIkvIzS3L0IyeSjdKX/McXPY432hfNKrfzeQgYiZEw gEH8sBsf4C2wL/gO/NK5jbG9JKEVO3XXeNdwxNgWNkGRHw6/sPrIYyuvT8szPUvNtcWk ttI9fEZECQ/yo+jvtTq43ZLZzFq4U0/VT0AA6S4XHChFIupET2WNXyGzVcbVLZ2tDokW bvmr/7FjivHf3YCza/KLDSzJIGFA7sUtz6PQdKYmVLO/3w1ksnLZVVUa4XciZnmt0GEV csWQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1787249344; x=1787854144; h=mime-version:user-agent:content-transfer-encoding:content-type :references:in-reply-to:date:cc:to:from:subject:message-id:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to :content-type; bh=/D29wRD0UC/Hr8ZPGocWQ9UTN+csrkFSPeqNFH7Hdo4=; b=cU5S6QvGGI7dFt4zxSzcbKwoif8Mm8pUki5gnF7pi5vtwqpSOGXCdeC4ctB0VWJxVx cXI2hz3NcsHmtGHIe4ZDGSHNwrnMoyc014nmRqtpWaOm13l219jOJxp6BvKo1xMWamyo USVBkS7Ex/+jrl7FhJFD7KOwVq/rdkjISBktwVbjfmCCJsaWW8CX2ZDc+uqfDGzs9vvU N/UTD2i3r3NaH/DMhHYk+OlvZCVcH8nRy6QNj0wvI82piOTs4bAqIXahz1uY9A9xoqid skVHKo+gtf5671AzC4TkqiofgEW9jd7x0YzXJyyMh/P1/5CV9/GIGcvkYQGmI00TvD6t TAiw== X-Gm-Message-State: AFuF++nlomyHMt2fiM0xiyR8lcJsaK+OBWlNMnryQFD5Sw87/fZEreNg 3oYCJHabSTVlbpWk5mKopzR61xQZqiukSn9wGYYorRSk5BzyysnUy16dw/SQ/wqdDi0iqpoezsq NLMe1o1DeaU6hb03PGVKUNZJIKsGnD5y6nhuP+/nLiNskf6omGOIl9wkma+H5Y/X4nA== X-Gm-Gg: AR+sD11HI27BExGjCisgkDgZcHPeh5NOyylLLpCJReVJ2QWeMtYxTuh/VjKf0TjRkfl 09/dksXd046ZM2o8iffF+X9eQ6JtCN3mFgcFVwXxaYGYQEuiyL1S+pZ4X13RyW85dkrCRDlfYVw kp3pXl48y9IfGFY/sCPufRTnMGqlhdAQkWThdCOw0Oxa3XCvMxOOR5oFb1KWZ7COI9Kvwq9trBS AT50RtGypSoILWwx56g8NXNaeaFuiGIQJgATCI6FdfnG6cx0nX+joEG7D9qMpAe0caXdIAF4tKB 38OEf2Z40pzqQoggUW74+S9uZk9Rheu9Cv61Wv6tgmGmjwq7CdLa1oZc5adZgxC88KImAFPs X-Received: by 2002:a05:6214:acf:b0:8ff:650d:2b99 with SMTP id 6a1803df08f44-90c807cac10mr1315936d6.0.1787249340461; Thu, 20 Aug 2026 11:09:00 -0700 (PDT) X-Received: by 2002:a05:6214:acf:b0:8ff:650d:2b99 with SMTP id 6a1803df08f44-90c807cac10mr1312606d6.0.1787249337745; Thu, 20 Aug 2026 11:08:57 -0700 (PDT) Received: from [192.168.8.4] ([100.0.180.93]) by smtp.gmail.com with ESMTPSA id 6a1803df08f44-90c5edb4fa2sm43884006d6.10.2026.08.20.11.08.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 20 Aug 2026 11:08:56 -0700 (PDT) Message-ID: <7c5419077ac6b087386dcd7f28b26003237a13d8.camel@redhat.com> Subject: Re: [PATCH v2] drm/nouveau: disable the fence uevent work instead of just cancelling it From: lyude@redhat.com To: Marek Czernohous , nouveau@lists.freedesktop.org, dri-devel@lists.freedesktop.org Cc: linux-kernel@vger.kernel.org, Danilo Krummrich , David Airlie , Simona Vetter Date: Thu, 20 Aug 2026 14:08:55 -0400 In-Reply-To: <89da2f2c2cd6223e6973b1adb8aebd7a9cd9bce8.camel@redhat.com> References: <178682366001.3748010.7798811159846779765@gmail.com> <178682366002.3748010.12779628082366287968@gmail.com> <178688513268.513871.3468844663561639695@gmail.com> <89da2f2c2cd6223e6973b1adb8aebd7a9cd9bce8.camel@redhat.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.58.3 (3.58.3-1.fc43) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 (I misspoke, this patch ended up in drm-misc-next instead as fixes is currently closed) On Thu, 2026-08-20 at 13:55 -0400, lyude@redhat.com wrote: > This makes sense to me. >=20 > Reviewed-by: Lyude Paul >=20 > Will push to drm-misc-next-fixes in just a moment >=20 > On Sun, 2026-08-16 at 14:58 +0200, Marek Czernohous wrote: > > From: Marek Czernohous > >=20 > > nouveau_fence_context_del() drains the uevent work while the event > > that > > feeds it is still armed: > >=20 > > cancel_work_sync(&fctx->uevent_work); > > nouveau_fence_context_kill(fctx, 0); > > nvif_event_dtor(&fctx->event); > >=20 > > nouveau_fence_wait_uevent_handler() queues the work > > unconditionally: > >=20 > > schedule_work(&fctx->uevent_work); > > return NVIF_EVENT_KEEP; > >=20 > > so a non-stall interrupt arriving after cancel_work_sync() has > > returned > > re-arms the work that was just drained.=C2=A0 The window closes in two > > steps, > > neither of which is the drain.=C2=A0 The kill blocks the event when the > > last > > fence holding a notify_ref is signalled, which reaches > > atomic_xchg(&ntfy->allowed, 0) (nvkm/core/event.c:104), and > > nvkm_event_ntfy() skips a ntfy that is not allowed (:183).=C2=A0 That > > stops > > further handlers from starting, but not one that is already inside > > nvkm_event_ntfy(): the event is created with wait =3D false > > (nouveau_fence.c:201), so nvkm_event_ntfy_block_() leaves it on the > > list > > and never takes event->list_lock.=C2=A0 Only nvif_event_dtor() waits > > that > > one > > out: nvkm_event_ntfy_del() (:141) goes through > > nvkm_event_ntfy_remove(), > > which takes write_lock_irq() on that same list_lock (:84). > >=20 > > Either way the re-arm happens after the drain, and the caller drops > > its > > reference immediately afterwards, for example > > nv84_fence_context_del(): > >=20 > > nouveau_fence_context_del(&fctx->base); > > chan->fence =3D NULL; > > nouveau_fence_context_free(&fctx->base); > >=20 > > That is a kref_put() on fctx->fence_ref, so the context outlives > > the > > teardown only while emitted fences still hold a reference of their > > own. > > That is no safety net: whenever none do, the count reaches zero > > right > > there and nouveau_fence_context_put() kfree()s fctx while the work > > is > > still queued.=C2=A0 &fctx->uevent_work is embedded in that allocation, > > so > > the > > workqueue already dereferences freed memory when it picks the item > > up, > > and nouveau_fence_uevent_work() can then take fctx->lock on it.=C2=A0 > > With > > CONFIG_DEBUG_OBJECTS_WORK and CONFIG_DEBUG_OBJECTS_FREE, kfree() of > > a > > still-queued work item is reported as the free of an active object. > >=20 > > On live memory the re-armed work has nothing left to do: > > nouveau_fence_context_kill() empties fctx->pending and sets fctx- > > > killed > > under fctx->lock, nouveau_fence_emit() then returns -ENODEV rather > > than > > queueing anything new, and nouveau_fence_update() only reaches > > nvif_event_block() if it signalled something off that list.=C2=A0 The > > defect > > is the access to freed memory, not what the work would have found. > >=20 > > Only chips from G84 on can reach this at all: > > nouveau_fence_context_new() > > returns before nvif_event_ctor() when priv->uevent is clear, and > > nv84_fence_create() is the only place that sets it. > > nv84_fence_context_del() is the context_del for all of those, > > because > > nvc0_fence_create() and gv100_fence_create() build on > > nv84_fence_create() > > and override only context_new. > >=20 > > Use disable_work_sync() instead.=C2=A0 It drains the work exactly like > > cancel_work_sync() does, and additionally increments the work > > item's > > disable count, after which "any attempt to queue @work will fail > > and > > return %false" (kernel/workqueue.c, disable_work()).=C2=A0 The handler'= s > > schedule_work() then has nothing to re-arm, and the teardown order > > stays > > as it is. > >=20 > > Draining a second time after nvif_event_dtor() would close the > > window > > as > > well, and without the newer API: once nvkm_event_ntfy_remove() has > > returned, no handler can start or still be running, so nothing re- > > arms > > the work past that point.=C2=A0 disable_work_sync() is preferred here > > because > > it needs one synchronisation point instead of two, it keeps the > > work > > from being queued at all rather than cleaning up after it, and it > > is > > what drm has settled on for this (drm/xe, drm/panthor, > > drm_pagemap). > > Blocking the event rather than the work is not an option: fctx- > > >event > > is > > created with wait =3D false, so a handler already inside > > nvkm_event_ntfy() > > can still queue the work. > >=20 > > Note for backports: disable_work_sync() arrived in v6.10 with > > commit 86898fa6b8cd ("workqueue: Implement disable/enable for > > (delayed) > > work items"), while the fix being corrected here reached 6.6.18 and > > 6.7.6.=C2=A0 linux-6.6.y therefore carries this bug without the API, an= d > > this > > patch would apply there and then fail to build.=C2=A0 A 6.6.y backport > > wants > > the second drain described above instead, as its own patch. > >=20 > > Reported-by: sashiko-bot > > Closes: > > https://lore.kernel.org/nouveau/20260812231330.705425-1-mczernohous@gma= il.com/ > > Fixes: 39126abc5e20 ("nouveau: offload fence uevents work to > > workqueue") > > Cc: # 6.10.x > > Assisted-by: Claude:claude-opus-5 > > Signed-off-by: Marek Czernohous > > --- > >=20 > > Changes in v2: > > =C2=A0- Do not reorder the teardown.=C2=A0 v1 moved nvif_event_dtor() a= head > > of > > =C2=A0=C2=A0 nouveau_fence_context_kill(); with the fences still unsign= alled > > that > > =C2=A0=C2=A0 leaves nouveau_fence_enable_signaling() reachable, and > > =C2=A0=C2=A0 nvif_event_constructed() is a plain unlocked read of objec= t- > > > client, > > =C2=A0=C2=A0 so the dtor could race an nvif_event_allow() already past = that > > check. > > =C2=A0=C2=A0 Reported as [Critical] by the bot, and withdrawn: > > =C2=A0=C2=A0 > > https://lore.kernel.org/all/20260815200914.8A1131F000E9@smtp.kernel.org= / > > =C2=A0- Change cancel_work_sync() to disable_work_sync() instead, which > > leaves > > =C2=A0=C2=A0 every ordering alone. > > =C2=A0- Pin the damage down.=C2=A0 Both versions call it a use-after-fr= ee; > > this > > one > > =C2=A0=C2=A0 adds that &fctx->uevent_work is embedded in the freed > > allocation, > > and > > =C2=A0=C2=A0 that the re-armed work has nothing left to do on a live co= ntext, > > so > > =C2=A0=C2=A0 the access to freed memory is the whole of it. > > =C2=A0- Correct the backport note.=C2=A0 v1 claimed no longterm tree sa= t in > > the > > gap > > =C2=A0=C2=A0 between the bug and disable_work_sync(); 6.6.y does.=C2=A0= The stable > > tag is > > =C2=A0=C2=A0 annotated accordingly. > >=20 > > This replaces 1/3 of > > https://lore.kernel.org/all/178682366002.3748010.12779628082366287968@g= mail.com/ > > 2/3 and 3/3 of that series are unaffected and still stand. > >=20 > > =C2=A0drivers/gpu/drm/nouveau/nouveau_fence.c | 2 +- > > =C2=A01 file changed, 1 insertion(+), 1 deletion(-) > >=20 > > diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c > > b/drivers/gpu/drm/nouveau/nouveau_fence.c > > index edbe9e08ba0f..11e95c37ce50 100644 > > --- a/drivers/gpu/drm/nouveau/nouveau_fence.c > > +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c > > @@ -96,7 +96,7 @@ nouveau_fence_context_kill(struct > > nouveau_fence_chan *fctx, int error) > > =C2=A0void > > =C2=A0nouveau_fence_context_del(struct nouveau_fence_chan *fctx) > > =C2=A0{ > > - cancel_work_sync(&fctx->uevent_work); > > + disable_work_sync(&fctx->uevent_work); > > =C2=A0 nouveau_fence_context_kill(fctx, 0); > > =C2=A0 nvif_event_dtor(&fctx->event); > > =C2=A0 fctx->dead =3D 1; > >=20 > > base-commit: c21bb4193868a8de71fc4693fa741e195fdf5d86