Re: [PATCH 1/2] devcoredump: Remove devcoredump device if failing device is gone

[Rodrigo Vivi]

On Mon, Jan 29, 2024 at 06:48:12PM +0100, Johannes Berg wrote:
> On Fri, 2024-01-26 at 10:11 -0500, Rodrigo Vivi wrote:
> > Make dev_coredumpm a real device managed helper, that not only
> > frees the device after a scheduled delay (DEVCD_TIMEOUT), but
> > also when the failing/crashed device is gone.
> > 
> > The module remove for the drivers using devcoredump are currently
> > broken if attempted between the crash and the DEVCD_TIMEOUT, since
> > the symbolic sysfs link won't be deleted.
> 
> Hmm, is it a problem to remove a whole dev when it still has some link
> here? 

Well, the big problem is that with link there, the base sysfs dir
is not deleted/removed. So, the next reload fails to recreate beucase
file exists.

> Maybe we could just make the link be managed/auto-removed?

this would help this angle indeed.

> 
> Probably regardless of that you should change the comment in
> devcd_dev_release() since it's no longer a concern?

indeed, I don't believe that that is a concern because this
is exactly the only place deleting the link and it can't race
with itself.

> 
> > 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
> 
> > can get some NULL dereferences or use after free.
> 
> Not sure I understand this part, how's this related to PCI's .remove?

Well, this is my secondary concern that the idea of the link_auto_removal
doesn't cover.

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.

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

I'm sorry for not being clear on this point. The other one was the
immediate one blocking our CI so I ended up writing up the commit
message with that in mind and without thinking about alternatives
like only removing the link.

> 
> > So, if the failing-device is gone let's also request for the
> > devcoredump-device removal using the same mod_delayed_work
> > as when writing anything through data. The flush cannot be
> > used since it is synchronous and the devcd would be surely
> > gone right before the mutex_unlock on the next line.
> 
> Can we just decouple it instead and remove the symlink? Which is kind of
> what the comment in devcd_dev_release() says but at the time I wasn't
> aware of all the devm mechanics etc.

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

Shouldn't we consider this critical itself to justify this entirely
removal?

> 
> I'm thinking this might be annoying in certain recovery cases, e.g.
> iwlwifi uses this but may sometimes unbind/rebind itself to recover from
> certain errors, and that'd make the FW dumps disappear.

I see... but it looks like dev_coredumpsg have a different handle of
the data cookie and read functions with the read direct from the
sgtable and might not face this bad memory access, since it allocates
the sg_dump_data which is only deleted/freed by the devcoredump removal...

But I'm concerned with the direct usage of the drivers using
dev_coredumpm() directly.

Should I then move dev_coredumpm to  _dev_coredumpm()
and then create a new dev_coredumpm that calls for
_dev_coredump and this devm_add_action(dev, devcd_remove, devcd)

?

And also an improved commit message to show the bad memory access issue?

Thank you so much for the feedback,
Rodrigo.

> 
> johannes