Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Greg KH <gregkh@linuxfoundation.org>,
        Mukesh Ojha <quic_mojha@quicinc.com>
Cc:     linux-kernel@vger.kernel.org, sboyd@kernel.org,
        johannes@sipsolutions.net, rafael@kernel.org
Subject: Re: Possible race in dev_coredumpm()-del_timer() path
In-Reply-To: <Ylas6F75Y7O6R87U@kroah.com>
References: <2e1f81e2-428c-f11f-ce92-eb11048cb271@quicinc.com>
 <YlZg4KkiAgODr45d@kroah.com>
 <20220413101639.GA24349@hu-mojha-hyd.qualcomm.com>
 <Ylas6F75Y7O6R87U@kroah.com>
Date:   Thu, 14 Apr 2022 12:38:13 +0200
Message-ID: <87pmlkdk6i.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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