linux-kernel - Re: [PATCH] nvdimm: nvdimm_bus_register: Avoid adding device to the unregistered bus

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cfa1a053-b99b-c22e-b80c-4b92f5f4e225@fujitsu.com>
Date:   Tue, 21 Mar 2023 02:17:36 +0000
From:   "lizhijian@...itsu.com" <lizhijian@...itsu.com>
To:     Dan Williams <dan.j.williams@...el.com>,
        "vishal.l.verma@...el.com" <vishal.l.verma@...el.com>,
        "dave.jiang@...el.com" <dave.jiang@...el.com>,
        "ira.weiny@...el.com" <ira.weiny@...el.com>,
        "nvdimm@...ts.linux.dev" <nvdimm@...ts.linux.dev>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] nvdimm: nvdimm_bus_register: Avoid adding device to the
 unregistered bus



On 21/03/2023 01:30, Dan Williams wrote:
> lizhijian@...itsu.com wrote:
> [..]
>>>> Dan,
>>>>
>>>> Configure the kconfig with ACPI_NFIT [=m] && LIBNVDIMM [=y], and add extra kernel booting parameter
>>>> 'initcall_blacklist=libnvdimm_init'. Then kernel panic!
>>>
>>> That's expected though,
>>
>> Do you mean we just keep it as it is.
> 
> Yes.
> 
>>
>>
>>> you can't block libnvdimm_init and then expect
>>> modules that link to libnvdimm to work.
>> Ah, we would rather see it *unable to work* than panic, isn't it.
> 
> That part is true, but consider the implications of adding error
> handling to all code that can no longer depend on initcall ordering, not
> just libnvdimm. This would be a large paradigm shift.
> 
> Now I do think it would be a good idea to fail device_add() if the bus
> is not registered, but I *think* that happens now as a result of:
> 
> 5221b82d46f2 driver core: bus: bus_add/probe/remove_device() cleanups
> 
> ...can you double check if you have that commit in your tests?

Great, panic is gone after i upgraded to the upstream!


> Now I do think it would be a good idea to fail device_add() if the bus
> is not registered,

BTW, below line 369: device_add() didn't fail in practical.

  354         mutex_init(&nvdimm_bus->reconfig_mutex);
  355         badrange_init(&nvdimm_bus->badrange);
  356         nvdimm_bus->nd_desc = nd_desc;
  357         nvdimm_bus->dev.parent = parent;
  358         nvdimm_bus->dev.type = &nvdimm_bus_dev_type;
  359         nvdimm_bus->dev.groups = nd_desc->attr_groups;
  360         nvdimm_bus->dev.bus = &nvdimm_bus_type;
  361         nvdimm_bus->dev.of_node = nd_desc->of_node;
  362         device_initialize(&nvdimm_bus->dev);
  363         lockdep_set_class(&nvdimm_bus->dev.mutex, &nvdimm_bus_key);
  364         device_set_pm_not_required(&nvdimm_bus->dev);
  365         rc = dev_set_name(&nvdimm_bus->dev, "ndbus%d", nvdimm_bus->id);
  366         if (rc)
  367                 goto err;
  368
  369         rc = device_add(&nvdimm_bus->dev);
  370         dev_err(&nvdimm_bus->dev, "registration failed: %d\n", rc);

Thanks
Zhijian