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;
Date:   Thu, 21 Jul 2022 21:14:18 +0200
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     "Jiazi.Li" <jqqlijiazi@gmail.com>
Cc:     "Rafael J. Wysocki" <rafael@kernel.org>,
        "Jiazi.Li" <jiazi.li@transsion.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] driver core: Do not add a device that has been removed
 in device_shutdown to devices_kset list again.
Message-ID: <Ytmlih/TgGNrRTwy@kroah.com>
References: <20220721123325.4675-1-jiazi.li@transsion.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220721123325.4675-1-jiazi.li@transsion.com>
Precedence: bulk

On Thu, Jul 21, 2022 at 08:33:25PM +0800, Jiazi.Li wrote:
> There is a race between device_shutdown and devices_kset_move_last:
> 1. device_shutdown remove dev from devices_kset, unlock list_lock
> 
> 2. supplier call device_link_add move this consummer dev to
> last of devices_kset:
> devices_kset_move_last+0x184/0x1fc
> device_reorder_to_tail+0x50/0x17c
> device_link_add+0x670/0x9c0
> phy_get+0x88/0x300
> 
> 3. device_shutdown call dev->bus->shutdown first time
> 
> 4. in next loop, device_shutdown pick this dev from devices_kset
> again, and call dev->bus->shutdown for the second time.
> 
> If the protection mechanism of dev->bus->shutdown is not perfect,
> the following crash will occur:
> [37.366651] [T1000001] Unable to handle kernel NULL pointer
> dereference at virtual address 0000000000000070
> [37.414053] [T1600001] Kernel Offset: 0x2c73800000 from 0xffffffc010000000
> [37.414908] [T1600001] PHYS_OFFSET: 0x40000000
> [37.415458] [T1600001] pstate: 60400005 (nZCv daif +PAN -UAO)
> [37.416172] [T1600001] pc : [0xffffffec83eccd40] kernfs_find_ns+0x18/0x154
> [37.417025] [T1600001] lr : [0xffffffec83ed8714] sysfs_unmerge_group+0x70/0x2e0
> [37.417931] [T1600001] sp : ffffffc01008ba90
> [37.418456] [T1600001] x29: ffffffc01008ba90 x28: ffffffec863c1000
> [37.419221] [T1600001] x27: ffffffec85234e30 x26: ffffffec86490000
> [37.419985] [T1600001] x25: ffffff80c854a4c0 x24: ffffff80c854a428
> [37.420750] [T1600001] x23: 0000000000400100 x22: ffffffec85c4c5c8
> [37.421514] [T1600001] x21: ffffffec85c4c5a0 x20: 0000000000000000
> [37.422277] [T1600001] x19: ffffff80c0178000 x18: ffffffc010083068
> [37.423043] [T1600001] x17: 0000000000000000 x16: 00000000000000d8
> [37.423807] [T1600001] x15: ffffffec842b9314 x14: ffffffec85b04050
> [37.424572] [T1600001] x13: 0000000000000000 x12: 000000000000000a
> [37.425336] [T1600001] x11: 0000000000000000 x10: ffffffec863e9000
> [37.426102] [T1600001] x9 : 0000000000000001 x8 : 0000000000000000
> [37.426869] [T1600001] x7 : 332e37332020205b x6 : ffffffec863e6e7e
> [37.427633] [T1600001] x5 : ffffffffffffffff x4 : 0000000000000000
> [37.428396] [T1600001] x3 : 000000000000003c x2 : 0000000000000000
> [37.429161] [T1600001] x1 : ffffffec85c4c5c8 x0 : 0000000000000000
> [37.612410] [T1600001] Call trace:
> [37.612831] [T1600001]  dump_backtrace.cfi_jt+0x0/0x8
> [37.613457] [T1600001]  dump_stack_lvl+0xc4/0x140
> [37.614038] [T1600001]  dump_stack+0x1c/0x2c
> [37.614588] [T1600001]  mrdump_common_die+0x3a8/0x544 [mrdump]
> [37.615326] [T1600001]  ipanic_die+0x24/0x38 [mrdump]
> [37.615951] [T1600001]  die+0x344/0x748
> [37.616425] [T1600001]  die_kernel_fault+0x84/0x94
> [37.617016] [T1600001]  __do_kernel_fault+0x230/0x27c
> [37.617642] [T1600001]  do_page_fault+0xb4/0x754
> [37.618212] [T1600001]  do_translation_fault+0x48/0x64
> [37.618846] [T1600001]  do_mem_abort+0x6c/0x164
> [37.619406] [T1600001]  el1_abort+0x44/0x68
> [37.619921] [T1600001]  el1_sync_handler+0x58/0x88
> [37.620512] [T1600001]  el1_sync+0x8c/0x140
> [37.621028] [T1600001]  kernfs_find_ns+0x18/0x154
> [37.621608] [T1600001]  sysfs_unmerge_group+0x70/0x2e0
> [37.622246] [T1600001]  device_del+0x198/0xd00
> [37.622794] [T1600001]  device_unregister+0x1c/0x3c
> [37.623409] [T1600001]  charger_device_unregister+0x40/0x54 [charger_class]
> [37.624277] [T1600001]  sgm41516d_shutdown+0x54/0x84 [sgm41516d]
> [37.625021] [T1600001]  i2c_device_shutdown+0x68/0x118
> [37.625656] [T1600001]  device_shutdown+0x234/0x614
> [37.626259] [T1600001]  kernel_restart+0x74/0x1e8
> [37.626840] [T1600001]  __arm64_sys_reboot+0x3b0/0x424
> [37.627475] [T1600001]  el0_svc_common+0xd4/0x270
> [37.628056] [T1600001]  el0_svc+0x28/0x88
> [37.628549] [T1600001]  el0_sync_handler+0x8c/0xf0
> [37.629141] [T1600001]  el0_sync+0x1b4/0x1c0
> 
> because dev->kobject.sd has been set to NULL in fisrt shutdown.

We can't take kernel changes to fix bugs in out-of-tree kernel modules.
Can you reproduce this with the in-tree drivers?  If so, which ones?

thanks,

greg k-h