Re: Possible race in dev_coredumpm()-del_timer() path

[Thomas Gleixner]

On Wed, Apr 13 2022 at 12:58, Greg KH wrote:
> On Wed, Apr 13, 2022 at 03:46:39PM +0530, Mukesh Ojha wrote:
>> 	p1 					p2(X)
>> 
>>    dev_coredump() uevent sent to userspace
>>    device_add()  =========================> userspace process X reads the uevents
>>                                             writes to devcd fd which
>>                                             results into writes to
>> 
>>                                             devcd_data_write()
>> 					      mod_delayed_work()
>>                                                 try_to_grab_pending()
>> 						  del_timer()
>> 						   debug_assert_init()
>>   INIT_DELAYED_WORK 
>>   schedule_delayed_work
>> 						    debug_object_fixup()
>
> Why do you have object debugging enabled?  That's going to take a LONG
> time, and will find bugs in your code.  Perhaps like this one?

It's not finding bugs in his code. It finds bug in the upstream
dev_coredump code.

> And if you turn object debugging off, what happens?

The debugobject splat goes away, but the problem persists.

    device_add()        -> uevent

Preemption or concurrency:

    devcd_data_write()
       mod_delayed_work(..., w, 0);      <- Uninitialized.

The dev_coredump code exposes the device before it is fully initialized
and a write ending up in devcd_data_write() touches uninitialized work.

It does not help to move the initialization before device_add() as that
creates another problem:

    INIT_DELAYED_WORK(w)
    ...
    device_add()        -> uevent

Preemption or concurrency:

    devcd_data_write()
      mod_delayed_work(..., w, 0);      <- Schedules work immediately

    work_queue_runs()
      devcd_del(w)
        device_del()
        put_device()                    <- Drops the last reference

    initialization continues...
    
So, yes this needs serialization of some sort.

Same problem vs. disabled_store().

Thanks,

        tglx