Re: [PATCH] devcoredump: Fix circular locking dependency with devcd->mutex.

Hey,

Den 2025-10-24 kl. 10:12, skrev Johannes Berg:

On Wed, 2025-07-23 at 16:24 +0200, Maarten Lankhorst wrote:

...

+static void __devcd_del(struct devcd_entry *devcd) +{

• devcd->deleted = true;
• device_del(&devcd->devcd_dev);
• put_device(&devcd->devcd_dev);

+}

static void devcd_del(struct work_struct *wk) { struct devcd_entry *devcd;

• bool init_completed;

devcd = container_of(wk, struct devcd_entry, del_wk.work);

• device_del(&devcd->devcd_dev);
• put_device(&devcd->devcd_dev);

• /* devcd->mutex serializes against dev_coredumpm_timeout */
• mutex_lock(&devcd->mutex);
• init_completed = devcd->init_completed;
• mutex_unlock(&devcd->mutex);
• if (init_completed)

• __devcd_del(devcd);
  

I'm not sure I understand this completely right now. I think you pull this out of the mutex because otherwise the unlock could/would be UAF, right?

But also we have this:

...

@@ -151,11 +160,21 @@ static int devcd_free(struct device *dev, void *data) { struct devcd_entry *devcd = dev_to_devcd(dev);

• /*

• * To prevent a race with devcd_data_write(), disable work and
  

• * complete manually instead.
  

• *
  

• * We cannot rely on the return value of
  

• * disable_delayed_work_sync() here, because it might be in the
  

• * middle of a cancel_delayed_work + schedule_delayed_work pair.
  

• *
  

• * devcd->mutex here guards against multiple parallel invocations
  

• * of devcd_free().
  

• */
  

• disable_delayed_work_sync(&devcd->del_wk); mutex_lock(&devcd->mutex);

• if (!devcd->delete_work)

• devcd->delete_work = true;
  

• flush_delayed_work(&devcd->del_wk);

• if (!devcd->deleted)

• __devcd_del(devcd);
  

  mutex_unlock(&devcd->mutex);

^^^^

Which I _think_ is probably OK because devcd_free is only called with an extra reference held (for each/find device.)

But ... doesn't that then still have unbalanced calls to __devcd_del() and thus device_del()/put_device()?

CPU 0 CPU 1

dev_coredump_put() devcd_del() -> devcd_free() -> locked -> !deleted -> __devcd_del() -> __devcd_del()

no?

johannes

Yeah don't you love the races in the design? All intricate and subtle.

In this case it's handled by disable_delayed_work_sync(), which waits for devcd_del() to be completed. devcd_del is called from the workqueue, and the first step devcd_free does is calling disable_delayed_work_sync, which means devcd_del() either fully completed or was not run at all.

Best regards, ~Maarten