Re: [PATCH 2/2] nvme-hwmon: notify hwmon alarms on the SMART / Health Status AEN

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] nvme: nvme_enable_aen() masks NVME_AEN_CFG_SMART with
  ctrl->oaes
- [High] nvme: nvme_complete_async_event() requeues AER without clearing
  SMART log
- [Medium] nvme-hwmon: nvme_hwmon_smart_notify() bypasses
  hwmon_notify_event()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] nvme: use-after-free in nvme_handle_aen_notice() [1]
--

commit 33552d2b2131791945c267787b8393f1a3c9f467
Author: Guixin Liu <kanie@linux.alibaba.com>

nvme-hwmon: notify hwmon alarms on the SMART / Health Status AEN

This commit subscribes to the NVMe SMART / Health Status asynchronous
event and schedules a work item on completion to notify sysfs pollers on
temp1_alarm.

> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index a7a4d09644c13..ef488d3ac081f 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1784,7 +1784,7 @@ int nvme_set_queue_count(struct nvme_ctrl *ctrl, int *count)
>  EXPORT_SYMBOL_GPL(nvme_set_queue_count);
>  
>  #define NVME_AEN_SUPPORTED \
> -	(NVME_AEN_CFG_NS_ATTR | NVME_AEN_CFG_FW_ACT | \
> +	(NVME_AEN_CFG_SMART | NVME_AEN_CFG_NS_ATTR | NVME_AEN_CFG_FW_ACT | \
>  	 NVME_AEN_CFG_ANA_CHANGE | NVME_AEN_CFG_DISC_CHANGE)
>  
>  static void nvme_enable_aen(struct nvme_ctrl *ctrl)

[Severity: High]
Does adding NVME_AEN_CFG_SMART to NVME_AEN_SUPPORTED actually enable SMART
asynchronous events in nvme_enable_aen()?

nvme_enable_aen() masks NVME_AEN_SUPPORTED against ctrl->oaes:

drivers/nvme/host/core.c:nvme_enable_aen() {
	u32 result, supported_aens = ctrl->oaes & NVME_AEN_SUPPORTED;
	int status;

	if (!supported_aens)
		return;
    ...
}

ctrl->oaes comes from the Identify Controller OAES field, where bits 7:0
are reserved and zero on compliant controllers (see NVMET_AEN_CFG_OPTIONAL
vs NVMET_AEN_CFG_ALL in drivers/nvme/target/nvmet.h).  In Set Features for
NVME_FEAT_ASYNC_EVENT, bits 7:0 are the mandatory SMART warning bitmask
(NVME_SMART_CRIT_SPARE, NVME_SMART_CRIT_TEMPERATURE, etc.), so masking
with ctrl->oaes clears NVME_AEN_CFG_SMART and never enables the event.

> @@ -4993,8 +4993,12 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
>  			nvme_handle_aer_persistent_error(ctrl);
>  			return;
>  		}
> -		fallthrough;
> +		ctrl->aen_result = result;
> +		break;
>  	case NVME_AER_SMART:
> +		nvme_hwmon_smart_event(ctrl);
> +		ctrl->aen_result = result;
> +		break;
>  	case NVME_AER_CSS:
>  	case NVME_AER_VS:
>  		ctrl->aen_result = result;

[Severity: High]
Does completing NVME_AER_SMART with requeue = true re-arm the Asynchronous
Event Request before the SMART / Health Information log page is read?

Per the NVMe specification, an asynchronous event remains latched on the
controller until host software reads the corresponding log page
(NVME_LOG_SMART) with RAE cleared.  Other AEN handlers such as
nvme_fw_act_work() read their log page before queueing
ctrl->async_event_work, whereas here ctrl->async_event_work resubmits the
AER immediately while nvme_hwmon_smart_notify() only calls sysfs_notify().
Before userspace wakes up and reads temp1_alarm (or if nothing polls
temp1_alarm, or CONFIG_NVME_HWMON is disabled), the new AER completes
immediately with the same uncleared event in a tight loop.

[Severity: Critical]
This problem wasn't introduced by this patch, but can a late
NVME_AER_NOTICE_ANA completion in nvme_complete_async_event() cause a
use-after-free [1] on ctrl during controller teardown?

CPU0                              CPU1
nvme_remove_ctrl()
  ...
  nvme_stop_ctrl()
    nvme_mpath_stop()
      cancel_work_sync(&ctrl->ana_work)
                                  nvme_complete_async_event()
                                    nvme_handle_aen_notice()
                                      queue_work(nvme_wq, &ctrl->ana_work)
  nvme_uninit_ctrl()
  nvme_put_ctrl() -> kfree(ctrl)
                                  nvme_ana_work()
                                    /* dereferences freed ctrl */
                                    <-- use-after-free

nvme_handle_aen_notice() checks ctrl->ana_log_buf but does not verify that
the controller is still live before queueing ctrl->ana_work, allowing the
work item to run after the controller has been freed.

> diff --git a/drivers/nvme/host/hwmon.c b/drivers/nvme/host/hwmon.c
> index 406203d8b9adb..564d5cae8001b 100644
> --- a/drivers/nvme/host/hwmon.c
> +++ b/drivers/nvme/host/hwmon.c

[ ... ]

> @@ -223,6 +225,32 @@ static const struct hwmon_chip_info nvme_hwmon_chip_info = {
>  	.info	= nvme_hwmon_info,
>  };
>  
> +/*
> + * Called from the admin completion path on a SMART / Health Status AEN.
> + * The completion context can be a hardirq, so only queue the work here;
> + * the notify itself is done in process context.  No log refresh is
> + * needed: userspace re-reads temp1_alarm, which fetches the SMART log
> + * on demand.
> + */
> +void nvme_hwmon_smart_event(struct nvme_ctrl *ctrl)
> +{
> +	struct nvme_hwmon_data *data;
> +
> +	if (!ctrl->hwmon_device)
> +		return;
> +
> +	data = dev_get_drvdata(ctrl->hwmon_device);
> +	queue_work(nvme_wq, &data->smart_work);
> +}
> +
> +static void nvme_hwmon_smart_notify(struct work_struct *work)
> +{
> +	struct nvme_hwmon_data *data =
> +		container_of(work, struct nvme_hwmon_data, smart_work);
> +
> +	sysfs_notify(&data->ctrl->hwmon_device->kobj, NULL, "temp1_alarm");
> +}

[Severity: Medium]
Should nvme_hwmon_smart_notify() call hwmon_notify_event() instead of
calling sysfs_notify() directly?

hwmon_notify_event(data->ctrl->hwmon_device, hwmon_temp, hwmon_temp_alarm,
0) notifies the sysfs attribute, emits the standard hwmon KOBJ_CHANGE
uevent, and calls hwmon_thermal_notify() to update the thermal zone
registered via HWMON_C_REGISTER_TZ.  Also, the commit message notes that
polling temp1_alarm reports spare-block conditions, but nvme_hwmon_read()
only checks NVME_SMART_CRIT_TEMPERATURE for temp1_alarm and never checks
NVME_SMART_CRIT_SPARE.

[1] https://sashiko.dev/#/bug/linux-216101c0-b2a4-4615-97e0-5cfa127dc568

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109120207.1352165-1-kanie@linux.alibaba.com?part=2