Re: [PATCH 2/2] nvmem: core: don't defer for a missing cell once the layout is bound

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] nvmem: core: race in of_nvmem_cell_get() with concurrent
  layout probe
- [Medium] nvmem: core: race in nvmem_layout_pending() with layout init,
  probe, and free
- [Low] nvmem: core: missing Fixes trailer for commit fc29fd821d9a
--

commit fb2c3415ead8c8d1df19a4c33f70cbcf0a16033a
Author: Manikandan Muralidharan <manikandan.m@microchip.com>

> nvmem: core: don't defer for a missing cell once the layout is bound
>
> of_nvmem_cell_get() returns -EPROBE_DEFER whenever a requested cell is
> not found and the nvmem device has a layout, assuming the layout has not
> added its cells yet. A layout may add none, e.g. when the data the cell
> describes is not programmed on the device. The layout is then bound but
> the cell is never going to show up, and the consumer is deferred forever
> instead of falling back.
>
> Only defer while the layout device is not bound yet, and return -ENOENT
> once it is, so that a consumer such as an Ethernet MAC can fall back to a
> random address.
>
> Signed-off-by: Manikandan Muralidharan <manikandan.m@microchip.com>

[Severity: Low]
This looks like a bug fix for layout device handling.  Should this
include:

  Fixes: fc29fd821d9a ("nvmem: core: Rework layouts to become regular devices")

> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index be3a051500850..f34a35a04e553 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -1403,6 +1403,31 @@ static int nvmem_layout_module_get_optional(struct nvmem_device *nvmem)
>  	return 0;
>  }
>  
> +/**
> + * nvmem_layout_pending() - Check whether a layout may still add cells
> + * @nvmem: nvmem device the layout belongs to
> + *
> + * A layout adds its cells from its probe. Once the layout device is bound,
> + * a cell that is still missing is never going to show up.
> + *
> + * Return: true if @nvmem has a layout that is not bound yet, false otherwise.
> + */
> +static bool nvmem_layout_pending(struct nvmem_device *nvmem)
> +{
> +	struct device *dev;
> +	bool bound;
> +
> +	if (!nvmem->layout)
> +		return false;
> +
> +	dev = &nvmem->layout->dev;
> +	device_lock(dev);
> +	bound = device_is_bound(dev);
> +	device_unlock(dev);
> +
> +	return !bound;
> +}

[Severity: Medium]
Can nvmem_layout_pending() lock an uninitialized or freed mutex on
nvmem->layout->dev?

In nvmem_register(), device_add(&nvmem->dev) publishes the nvmem device on
nvmem_bus_type before nvmem_populate_layout() runs, and
nvmem_layout_create_device() sets nvmem->layout before device_initialize()
initializes layout->dev.mutex:

CPU0                                CPU1
nvmem_register()
  device_add(&nvmem->dev)
  nvmem_populate_layout()
    nvmem_layout_create_device()
      nvmem->layout = layout;
                                    of_nvmem_cell_get()
                                      ...
                                      nvmem_layout_pending()
                                        device_lock(&nvmem->layout->dev)
                                          <-- uninitialized mutex
      device_initialize(dev)

Similarly, nvmem_destroy_layout() unregisters and frees nvmem->layout
before nvmem->dev is removed from nvmem_bus_type and without clearing
nvmem->layout, so a concurrent nvmem_layout_pending() can dereference and
lock a freed layout device.

> @@ -1486,7 +1511,7 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np, const char *id)
>  	of_node_put(cell_np);
>  	if (!cell_entry) {
>  		nvmem_layout_module_put(nvmem);
> -		ret = nvmem->layout ? -EPROBE_DEFER : -ENOENT;
> +		ret = nvmem_layout_pending(nvmem) ? -EPROBE_DEFER : -ENOENT;
>  		nvmem_device_put(nvmem);
>  		return ERR_PTR(ret);
>  	}

[Severity: Medium]
Can of_nvmem_cell_get() falsely return -ENOENT when racing with a layout
driver probe?

During really_probe(), device_set_driver() sets layout->dev.driver under
device_lock(&layout->dev) before the layout's probe callback adds cells to
nvmem->cells:

CPU0                                CPU1
really_probe(&layout->dev)
  device_lock(&layout->dev)
  device_set_driver(dev, drv)
                                    of_nvmem_cell_get()
                                      nvmem_layout_module_get_optional()
                                        sees dev.driver != NULL
                                      nvmem_find_cell_entry_by_node()
                                        returns NULL
                                      nvmem_layout_pending()
                                        device_lock(&layout->dev)
  call_driver_probe()
    adds cell to nvmem->cells
  driver_bound(dev)
  device_unlock(&layout->dev)
                                        device_is_bound(dev) == true
                                      returns ERR_PTR(-ENOENT)

Consumer drivers then treat a valid cell that was just added as missing.
In addition, if really_probe() fails, device_unbind_cleanup() clears
layout->dev.driver under device_lock(&layout->dev), so
nvmem_layout_module_put() can see dev.driver == NULL and leak the module
reference acquired by nvmem_layout_module_get_optional().

Checking device_is_bound() and taking the module reference under
device_lock(&nvmem->layout->dev) in nvmem_layout_module_get_optional()
before searching nvmem->cells would avoid both issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794391847.git.manikandan.m@microchip.com?part=2

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/