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 0E1C447ECFB for ; Thu, 20 Aug 2026 17:55:40 +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=1787248542; cv=none; b=INCVFqwiMRGGAcnqqmq4eL2uq7VGEV6gyJKD5czcBvjnEKasWGGPwgvezSJ5Lhqabq0e0/eH6muQrvw5RBGdT9/q0NQszyhnV7oPpdzt/BxhJ8HbHvRykq4KKlfH4SvuK0FZp3WKuYPtwOqRREB5Wrbp7OgMqCwqRGK/SYim484= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787248542; c=relaxed/simple; bh=7GoDb3u0tyV+sLb+NhUxOx4YJgSD9eAhBWDwo76yHZE=; h=Message-ID:Subject:From:To:Cc:Date:In-Reply-To:References: Content-Type:MIME-Version; b=A8gHno7DHbBmbw7syw6WMby4JSRnmYdCrNPBBkEC0MEdhFSkKabnn61BQjxASs8oeyI7+Mh056D/JbnL4wr3NVXrOy9LSe18uGLEjCwTDUFZR9nAOmlO5f2kOL6FWPUhoyV51imQ8+a0tA4qpcsTlghXdDJ/i4k6QVCn0KiyTaU= 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=VKMjpM+c; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b=BZGSpjLa; 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="VKMjpM+c"; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b="BZGSpjLa" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1787248539; 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=VTFvT4h0Q0q3RfixbzGb3rOg5NcbDH7/Ar2/bb9HET4=; b=VKMjpM+cxzuAH8JUCOO8wHdkVALEtpa5bsPhMwm52bm9zH9dAYRFsu/Yiar7sORyR0F9SZ 2nLFqvdye9SYt54FFkgjVX0/SO6Xmj3A6F1j/AtMhjyeUOTNaZ/XOhSDkB5txLF59Jcm+p E8q5Xe7GBFyM5xmNHS1xxomQqVM3RSs= Received: from mail-qt1-f199.google.com (mail-qt1-f199.google.com [209.85.160.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-401-L88LXTIvNlWfZc2_na1apQ-1; Thu, 20 Aug 2026 13:55:38 -0400 X-MC-Unique: L88LXTIvNlWfZc2_na1apQ-1 X-Mimecast-MFC-AGG-ID: L88LXTIvNlWfZc2_na1apQ_1787248538 Received: by mail-qt1-f199.google.com with SMTP id d75a77b69052e-51c1c7f135bso1403181cf.0 for ; Thu, 20 Aug 2026 10:55:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=google; t=1787248538; x=1787853338; 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=VTFvT4h0Q0q3RfixbzGb3rOg5NcbDH7/Ar2/bb9HET4=; b=BZGSpjLaC+8gtpR9yNhgp9lbdAYc3pKKi81XQmSqFVnjHOulbY0XQX9IUI+0Zxcb9H Y8KE2I8ONs7MSaPVgoZTOFVqV11Rmx6q7E5ba1iExdS5pK/WRgwK8vCJ2bch9DCScMMj SJzXuS6QQtYofVBEjsFKT7DgW918zIvLlETDGAkAu2OmLylp1+cMfo2ZCX06kJl6aDuA 2o4HXWpbDRnazRvdyHzvl6Ry8sgcR5G5TgwE1wStVB/oj12sEll/8t+7Vfqyo3/x/uqJ +yzu7z5d4cPYV5i4RkD4Ii6Dpkjk1g8XTVsAb4Zj6jF87K8dGnsCbnuDxHURy3xzASp9 Dafw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1787248538; x=1787853338; 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=VTFvT4h0Q0q3RfixbzGb3rOg5NcbDH7/Ar2/bb9HET4=; b=eeWPr8/29aBSJSXA4JDG7X5hbSmgkvF9Qq/JpejUXNEWkvH6Iw4yWFQYER8NE/wdsq NF1J1788TJQMsfVazHasP4NmscIkU86B4YI9lXboJHkJom3SWY4M5X9bxDAGWLL2+zvy kcg4Mbg9C22SzWcjjBt2u9LJe9CO6lDhxU+PAw64XfJXR+1pH1BVaPitsUN1Zr8jHe1G B4RzVQAJLQhxxgbDqOYQCN81Pb2Dw6m25dUEIYMws6DM5+fMYNh7Ffi+fIVgEd9UZ2mJ lou5bIn9eFI3+pg76K3apXO//OPuGRW5FmGW6i230vOYC9Eb+jPMDHfDBCOJ6h2LUxE7 yTqQ== X-Gm-Message-State: AOJu0YxOCZpl5XFVX3dZON+EdQn3I8Uu/8CRdkuW8YbcgmqAMqL3Earl S81qc0yeS0H/EO4n9seGdBa/zLA5Gys+Kxy4XEedCiJxmELuzq7lByfBUx8fMTj4SOUda9K8JlN xDcFPHWOvb4IyAhUn3MRdEXm8EeAgzodwsNDbc7Djj241K+kpa/ZvT0udbH3l40/DJw== X-Gm-Gg: AR+sD10M8Wt3hmeqW8JazAUs2Xi+xZZPkgRwxnvr5Wo4UP+EAXYwcW3YBa2PmIga0OG /ryCkR7yfXX+TyElQ9yu5As9+rNS96DfZ5c3e5nkBSENIyAQpyt158hAXNuHVZqVAonLK/ShQOU HivTNohjIVPB0WEN0eeJNzHVNWZAcjCUEFaDtS4o2gltGcEvoAeTBv9Dq09PcEYgkk+32CbQ+34 uYSYY6lC2mwLiizHV0cyxJgDF3p/UTpTHpLsNRDTNPPzA5KA0uxTWsBoBSG3EuYzCY4grwC6jRX Tm3ZIZR+tbYy90fctA8k6Cb359LZa//zZpO2v6l5csGwQqCzhXHHOlxkFpIjxbqRBtynGJtN X-Received: by 2002:a05:622a:5591:b0:516:ed02:c85d with SMTP id d75a77b69052e-52df5637a7dmr1713101cf.3.1787248537862; Thu, 20 Aug 2026 10:55:37 -0700 (PDT) X-Received: by 2002:a05:622a:5591:b0:516:ed02:c85d with SMTP id d75a77b69052e-52df5637a7dmr1712551cf.3.1787248537384; Thu, 20 Aug 2026 10:55:37 -0700 (PDT) Received: from [192.168.8.4] ([100.0.180.93]) by smtp.gmail.com with ESMTPSA id 6a1803df08f44-90c5f29065bsm43058966d6.24.2026.08.20.10.55.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 20 Aug 2026 10:55:36 -0700 (PDT) Message-ID: <89da2f2c2cd6223e6973b1adb8aebd7a9cd9bce8.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 13:55:35 -0400 In-Reply-To: <178688513268.513871.3468844663561639695@gmail.com> References: <178682366001.3748010.7798811159846779765@gmail.com> <178682366002.3748010.12779628082366287968@gmail.com> <178688513268.513871.3468844663561639695@gmail.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 This makes sense to me. Reviewed-by: Lyude Paul Will push to drm-misc-next-fixes in just a moment 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 Wit= h > 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, and > 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@gmail= .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() ahe= ad of > =C2=A0=C2=A0 nouveau_fence_context_kill(); with the fences still unsignal= led > 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 object- > >client, > =C2=A0=C2=A0 so the dtor could race an nvif_event_allow() already past th= at > 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-free= ; this > one > =C2=A0=C2=A0 adds that &fctx->uevent_work is embedded in the freed alloca= tion, > and > =C2=A0=C2=A0 that the re-armed work has nothing left to do on a live cont= ext, > 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 sat = in the > gap > =C2=A0=C2=A0 between the bug and disable_work_sync(); 6.6.y does.=C2=A0 T= he stable > tag is > =C2=A0=C2=A0 annotated accordingly. >=20 > This replaces 1/3 of > https://lore.kernel.org/all/178682366002.3748010.12779628082366287968@gma= il.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