Received-SPF: pass (google.com: domain of linux-kernel+bounces-43536-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <33df6c78c4c47a8f57a1c2bfe835065becb5a253.camel@sipsolutions.net>
Subject: Re: [PATCH 1/2] devcoredump: Remove devcoredump device if failing
 device is gone
From: Johannes Berg <johannes@sipsolutions.net>
To: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: linux-kernel@vger.kernel.org, Jose Souza <jose.souza@intel.com>, Maarten
 Lankhorst <maarten.lankhorst@linux.intel.com>, Greg Kroah-Hartman
 <gregkh@linuxfoundation.org>,  "Rafael J . Wysocki" <rafael@kernel.org>
Date: Mon, 29 Jan 2024 22:51:39 +0100
In-Reply-To: <ZbgYyra1Ypa0nf6u@intel.com>
References: <20240126151121.1076079-1-rodrigo.vivi@intel.com>
	 <d57dc34fa8b0e25cec014b8001dcd0527d1c1013.camel@sipsolutions.net>
	 <ZbgYyra1Ypa0nf6u@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.3 (3.50.3-1.fc39) 
Precedence: bulk
MIME-Version: 1.0

On Mon, 2024-01-29 at 16:29 -0500, Rodrigo Vivi wrote:
> >=20
> > > On top of that, for PCI devices, the unbind of the device will
> > > call the pci .remove void function, that cannot fail. At that
> > > time, our device is pretty much gone, but the read and free
> > > functions are alive trough the devcoredump device and they
> >                       ^ through, I guess
> >=20
> > > can get some NULL dereferences or use after free.
> >=20
> > Not sure I understand this part, how's this related to PCI's .remove?
>=20
> Well, this is my secondary concern that the idea of the link_auto_removal
> doesn't cover.
>=20
> If the failing_device is gone, the 'data cookie' it used to register with
> dev_coredumpm(... void *data,...), is also likely gone on a clean removal=
.

That's on the user. You'll always be able to shoot yourself in the foot.

> And to be honest, we shouldn't even count that the registered *read()
> function pointer is valid anymore.

That's not true: the module cannot be removed, there's a reference to it
if you're using dev_coredumpm() correctly (which is to say: pass
THIS_MODULE to the struct module *owner argument).

> Well, we could indeed. And that would unblock our CI, but I'm afraid
> it wouldn't protect the final user from bad memory access on a direct
> $ cat /sys/class/devcoredump/devcd<n>/data
>=20
> Shouldn't we consider this critical itself to justify this entirely
> removal?

No? IMHO that's totally on the user. If you absolutely cannot make a
standalone dump 'data' pointer (why not?! you can always stick the
actual data into a vmalloc chunk and use dev_coredumpv()?) then maybe we
can offer ways of removing it when you need to? But I'd rather not, it
feels weird to have a need for it.

johannes