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.133.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 6A636371068 for ; Tue, 18 Aug 2026 23:58:29 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.133.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787097511; cv=none; b=JdJjxTfcnrL1A6a5R4jjw4qeyKcgnCIuaSPYZV9PvyGQNdotklFnrexDMltt4w+yN4iWfLVZpXtua1UfLMaEWrBsZDQx1Mh5ErSXP9VVwu9r5RSRofOQ33Iy6SoesD886H10Por8Ynj6zonQUN71KEXSu1nV1w1M4yXszE/urik= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787097511; c=relaxed/simple; bh=tz/yXH3YIXKsxVZ87/WPseHbbbdSG7a6l6OJ3Eub38g=; h=Message-ID:Subject:From:To:Cc:Date:In-Reply-To:References: Content-Type:MIME-Version; b=LsfpSpDH1UKj5iGfQHdNzM6sqXUvn28UnCbrVSb74/Bn41kLFWggL26mxwuAr+vjLQsXWVNBHXVB+yqy30MC0SVORI8aFkFgWuPiC3aP71au9JgiereeVvHry3PC0XqzRGHC8FZSuQ8KgHxpxuPyhDeKIbvBNVCRK+8RCSCMRi8= 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=DtE4Lams; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b=Vy0Ovab/; arc=none smtp.client-ip=170.10.133.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="DtE4Lams"; dkim=pass (2048-bit key) header.d=redhat.com header.i=@redhat.com header.b="Vy0Ovab/" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1787097508; 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=tz/yXH3YIXKsxVZ87/WPseHbbbdSG7a6l6OJ3Eub38g=; b=DtE4LamsjREtF1hwpVIfOZURPMlJPBOz6JmrUJwvH6eRpj3uhG8Yke/EEVs2BZxTqz7tQh 2XybAClOabnhQYAMn6r6O7yknMsaDcepu6n7a1o4Y8V3QxqEWNHYMCF5/2yDfA3tVl9Ohm qJf3677JPzUPJL1I3R90XXt2VVGriIY= 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-622-yzO-un7wMx-q5aJaFYzSnA-1; Tue, 18 Aug 2026 19:58:27 -0400 X-MC-Unique: yzO-un7wMx-q5aJaFYzSnA-1 X-Mimecast-MFC-AGG-ID: yzO-un7wMx-q5aJaFYzSnA_1787097507 Received: by mail-qt1-f199.google.com with SMTP id d75a77b69052e-52827090cc5so4817091cf.0 for ; Tue, 18 Aug 2026 16:58:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=google; t=1787097507; x=1787702307; 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=tz/yXH3YIXKsxVZ87/WPseHbbbdSG7a6l6OJ3Eub38g=; b=Vy0Ovab/dAmDpaMmnfs/WM4gXdGjVh0cSyN0uWkO1NsHK0hSCBKu8oex/Pg/E8RbIF Uiudm+GpI1gkar15HAQ0uctZXYs7+QnSVwPohnuB4yQDJ81xgBtUdq5EbCTTIuMzwLKx PgmC2o6251OUEgWLaMQLM/GPeYREhpamrbR1t43ZeqTCdDDAWOgIbXrzYu5/oJuUFJVe yiMRINlfdjn7oEN06IbHGOl0FYIGMLIp9bNMaVNzDz1wR7+riiyBiu9k8/HjQQ2DtiMt RcFHsZsnWD0rEACwKeLSGep0UmpQsQyF1OOtARG0Nkfet7rLp85tD+9s1dV8HPnuTrAk oUlw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1787097507; x=1787702307; 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=tz/yXH3YIXKsxVZ87/WPseHbbbdSG7a6l6OJ3Eub38g=; b=cpKTgAgkfr7eTDwOef6AxsqxzbfYlSBxZ5r+30Io8cWXCbsnT38igOuXZ2lDURNF7/ paTYNIxh0nXIdq/FXX7ElfHMgf1XgHqEkFsYJpRqPsZfNJM1Oeq/DR/xEIr81g+u2HdU BhAni3EbTbaJUmv+mPjbOlrtgkWaZo28EoR9hY2YPrVHc5UTSKl43JRvasdXGv9AlbOV gQoJTy8ZjLHdxqew/MJ5lFrx4YuH8E+OEvFEU/jbqgoqPaWU48/D4xSHeYp+I46lAY4p icwKXIJU3nPR47BwoR64qVCDWRZwoCGDccnuj9vp9+NSvj+N6QYk1IZjPdZMFttVZxiz nY0Q== X-Gm-Message-State: AOJu0YyAmAtfATfAVqaRSGfe2tQ7swht9ZqePPPNZhPWo12MJ8sYMlMm oBTRE2OHTt/0y4V4O/MHHGsKrKgmclOIf4818JoE4h5NX8BPQvIc6NJW3Od4DhI5Px+VDhJJciA 9JuezzJhgKseVApI+ve0cFCuEYrclye3UajSCzJrZMJlUl7HcaaaJ/V4pO+WceW8bsQ== X-Gm-Gg: AR+sD12127dIo3/U7A+NCTcTxgx0NQn5ZmCmnICAntV9/eW9JNx0/qHTNNutA0iTWC9 yOFKIIo8WkdUEXhstTj2+UtYd09UoYT4agYaCA6TydiQPr6dWTsoUOuDX73GX0yCY+yDsv7OZk7 EZ/z+7QT+2vdjTjhZ+zwwNNp9tG4G+zbeExp/KXfAyr6oKheioniOR9VolachwpBTCaOiTWvBXU 7N+VLCE3SwS7jUvkPeW06Z5OgnLnELCAyg+BeFSnBXp6D5/kq6cjrxHC5Jl3TerKMf5P1YcUAHA lSjFFKCRpD0gPD5j6LXPcY9FQAKwgGhLwx0O9GiHVIPhrwwKhcXT7hldvaigqBXeZMdhZeq4 X-Received: by 2002:ac8:7fcc:0:b0:51c:1ea:e290 with SMTP id d75a77b69052e-52dd4970d83mr7644901cf.17.1787097506673; Tue, 18 Aug 2026 16:58:26 -0700 (PDT) X-Received: by 2002:ac8:7fcc:0:b0:51c:1ea:e290 with SMTP id d75a77b69052e-52dd4970d83mr7644631cf.17.1787097506193; Tue, 18 Aug 2026 16:58:26 -0700 (PDT) Received: from [192.168.8.4] ([100.0.180.93]) by smtp.gmail.com with ESMTPSA id d75a77b69052e-52dd872a9e6sm382701cf.20.2026.08.18.16.58.24 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 18 Aug 2026 16:58:25 -0700 (PDT) Message-ID: <3c85782ad840395622a93e9c30f151296dea838b.camel@redhat.com> Subject: Re: [PATCH 0/3] drm/nouveau: teardown ordering fixes for events and work 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: Tue, 18 Aug 2026 19:58:24 -0400 In-Reply-To: <178682366001.3748010.7798811159846779765@gmail.com> References: <178682366001.3748010.7798811159846779765@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 Thank you for all of the submissions! JFYI - I should be able to get to reviewing this one tomorrow On Sat, 2026-08-15 at 21:54 +0200, Marek Czernohous wrote: > From: Marek Czernohous >=20 > Three teardown fixes in nouveau, all the same shape: something that > can > still run after the thing it points at has been torn down or freed. >=20 > Two of them are not my finding. The Sashiko review bot flagged them > as > pre-existing issues in its review of my nv04 FIFO series, >=20 > =C2=A0 > https://lore.kernel.org/nouveau/20260812231330.705425-1-mczernohous@gmail= .com/ >=20 > naming nouveau_fence_context_del() and nouveau_connector_destroy() > directly. It was right about both. 1/3 and 2/3 carry a Reported-by > accordingly. 3/3 is mine, found while following the irq_work of 2/3 > into its handler, which is nouveau_dp_irq(). >=20 > 1/3 nouveau_fence_context_del() cancels the uevent work first and > drops > =C2=A0=C2=A0=C2=A0 the event afterwards. In between, the event is still a= rmed and > =C2=A0=C2=A0=C2=A0 nouveau_fence_wait_uevent_handler() queues the work > unconditionally, > =C2=A0=C2=A0=C2=A0 so a non-stall interrupt in that window re-arms the wo= rk that was > =C2=A0=C2=A0=C2=A0 just cancelled. The callers free the context immediate= ly after, > =C2=A0=C2=A0=C2=A0 which leaves nouveau_fence_uevent_work() walking freed= memory. > =C2=A0=C2=A0=C2=A0 Destroy the event first, then drain. >=20 > 2/3 nouveau_connector_destroy() drops the connector's two events but > =C2=A0=C2=A0=C2=A0 never drains nv_connector->irq_work, which is what the= DP IRQ > event > =C2=A0=C2=A0=C2=A0 schedules. The work can then run against a connector t= hat is > about > =C2=A0=C2=A0=C2=A0 to be, or has already been, freed. >=20 > 3/3 nouveau_dp_irq() looks the encoder up and dereferences it in the > =C2=A0=C2=A0=C2=A0 declaration block, five lines above the NULL test that= the same > =C2=A0=C2=A0=C2=A0 function already carries. >=20 > 2/3 and 3/3 both point at the same commit. Commit 773eb04d14a1 > ("drm/nouveau/disp: expose conn event class") turned nouveau_dp_irq() > into a work callback, and that single change introduced both the > undrained work and the early dereference: the drm pointer used to be > an > argument, and recovering it from the encoder put a dereference above > the > existing test. >=20 > All three carry Fixes: and Cc: stable. 1/3 and 2/3 are use-after-free > windows, and each commit message names the trigger, the window, and > the > freed object the work then touches. 3/3 is a NULL dereference sitting > above the function's own NULL test. >=20 > I also looked one level up, since it would have been the obvious next > instance. drm->hpd_work is drained in nouveau_display_fini(), right > after the hotplug events are blocked, under > "if (!runtime && !drm->headless)". That guard does not exempt the > teardown path: nouveau_drm.c:597 calls nouveau_display_fini(dev, > false, > false) immediately before nouveau_display_destroy(), so runtime is > false there. The runtime exemption belongs to the suspend path > (nouveau_display.c:781), which frees nothing. So there is no fourth > patch here. >=20 > Testing >=20 > =C2=A0 Reference hardware: Apple Macmini3,1, MCP79 / GeForce 9400M (NVAC)= , > =C2=A0 Core 2 Duo, Wayland (labwc). Note for 1/3 that this chip takes the > =C2=A0 nv84_fence path, which is the one where the event exists at all. >=20 > =C2=A0 Build. The series is built against the stated base commit, as a > full > =C2=A0 kernel build rather than a module-only one, so modpost actually > =C2=A0 resolved the module's symbols instead of being skipped for want of > =C2=A0 Module.symvers: zero compiler warnings, zero compiler errors, > =C2=A0 nouveau.ko produced. checkpatch.pl --strict is clean on all three > =C2=A0 patches and on this cover. >=20 > =C2=A0 What the testing does not show, stated plainly: I have not managed > to > =C2=A0 hit any of these three windows deliberately on this hardware. They > are > =C2=A0 ordering bugs reasoned out from the source rather than from a > =C2=A0 reproduction, and I would rather say that than dress up a crash I > do > =C2=A0 not have. Each patch names the file and the function it argues fro= m > so > =C2=A0 the reasoning can be checked directly. >=20 > AI assistance >=20 > =C2=A0 Lyude asked on an earlier thread whether these patches were writte= n > by > =C2=A0 a human and pointed at Documentation/process/coding-assistants.rst= . > =C2=A0 The answer, repeated here for the archive: this work is AI > assisted. I > =C2=A0 use Claude (claude-opus-5) as a coding and analysis assistant. > Every > =C2=A0 patch carries an Assisted-by trailer accordingly, and no Signed- > off-by > =C2=A0 is added by the tool. >=20 > =C2=A0 Nature of the assistance, so you can calibrate your review: the > =C2=A0 assistant did most of the code archaeology and drafting. I > described > =C2=A0 symptoms, asked for the mechanism to be traced in the source rathe= r > =C2=A0 than guessed, and asked for each claim to be backed by a file and = a > =C2=A0 line. The assistant also reviewed its own drafts adversarially, > which > =C2=A0 is how two errors in 1/3 were caught before this posting: an > earlier > =C2=A0 draft claimed nouveau_fence_context_kill() does not touch the > event, > =C2=A0 which the source contradicts, and it illustrated the freeing calle= r > =C2=A0 with nv04_fence_context_del(), which is precisely the case that > cannot > =C2=A0 reach the bug, since nouveau_fence_context_new() returns before > =C2=A0 nvif_event_ctor() unless nv84_fence_create() set priv->uevent. Bot= h > =C2=A0 are corrected. I reviewed the result, I understand the code, and I > =C2=A0 take responsibility for it. >=20 > Marek Czernohous (3): > =C2=A0 drm/nouveau: destroy the fence event before cancelling its work > =C2=A0 drm/nouveau: cancel the DP IRQ work before freeing the connector > =C2=A0 drm/nouveau: don't dereference outp before checking it in > =C2=A0=C2=A0=C2=A0 nouveau_dp_irq >=20 > =C2=A0drivers/gpu/drm/nouveau/nouveau_connector.c | 1 + > =C2=A0drivers/gpu/drm/nouveau/nouveau_dp.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0 | 4 +++- > =C2=A0drivers/gpu/drm/nouveau/nouveau_fence.c=C2=A0=C2=A0=C2=A0=C2=A0 | 2= +- > =C2=A03 files changed, 5 insertions(+), 2 deletions(-) >=20 >=20 > base-commit: c21bb4193868a8de71fc4693fa741e195fdf5d86