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

[Maarten Lankhorst]

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