Re: [PATCH v2 1/1] 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-hwmon: race between smart_work and async_event_work
  re-arms AER early
- [High] nvme: NULL submit_async_event() dereference via
  nvme_enable_aen()
- [Low] nvme-hwmon: unfiltered NVME_AER_SMART subtype in
  nvme_complete_async_event()
--

commit 0ce1f50fb06d20be5bc969ec67713426ed866b3c
Author: Guixin Liu <kanie@linux.alibaba.com>

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

This commit enables the temperature critical warning in the Asynchronous
Event Configuration when hwmon is present and schedules work on SMART /
Health Status AEN completions to re-read the SMART log and notify hwmon
on temp1_alarm.

> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index f4aa02869b790..9e355fb0e3992 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1792,6 +1792,17 @@ static void nvme_enable_aen(struct nvme_ctrl *ctrl)
>  	u32 result, supported_aens = ctrl->oaes & NVME_AEN_SUPPORTED;
>  	int status;
>  
> +	/*
> +	 * Unlike the notice events above, SMART / Health Status events have
> +	 * no OAES capability bit: AEC dw11 bits 7:0 are the SMART / Health
> +	 * Critical Warnings (SHCW) mask, which mirrors the critical_warning
> +	 * byte of the SMART log.  Subscribe to the temperature warning
> +	 * whenever the hwmon support is present, as that is the only
> +	 * consumer.
> +	 */
> +	if (nvme_hwmon_present(ctrl))
> +		supported_aens |= NVME_SMART_CRIT_TEMPERATURE;
> +
>  	if (!supported_aens)
>  		return;

[Severity: High]
Can this cause a NULL pointer dereference in nvme_async_event_work() on
controllers whose nvme_ctrl_ops do not implement submit_async_event()?

The apple-nvme driver does not support asynchronous events, reports
ctrl->oaes == 0, and leaves submit_async_event NULL in its nvme_ctrl_ops,
but still initializes hwmon via nvme_init_ctrl_finish().  Previously,
supported_aens remained 0 so nvme_enable_aen() returned early.  With
NVME_SMART_CRIT_TEMPERATURE set whenever ctrl->hwmon_device is present,
nvme_enable_aen() now queues ctrl->async_event_work, and
nvme_async_event_work() unconditionally calls
ctrl->ops->submit_async_event(ctrl).

> @@ -5006,8 +5017,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;

[Severity: Low]
Should this check aer_subtype (or log->critical_warning in
nvme_hwmon_smart_notify()) before notifying temp1_alarm?

If any non-temperature SMART / Health Status warning is enabled in
NVME_FEAT_ASYNC_EVENT (for example via admin passthrough),
nvme_hwmon_smart_notify() will read the SMART log with RAE cleared and
emit a false temp1_alarm notification and thermal zone update.

> diff --git a/drivers/nvme/host/hwmon.c b/drivers/nvme/host/hwmon.c
> index 406203d8b9adb..172ef63f1180c 100644
> --- a/drivers/nvme/host/hwmon.c
> +++ b/drivers/nvme/host/hwmon.c
[ ... ]
> @@ -223,6 +224,39 @@ static const struct hwmon_chip_info nvme_hwmon_chip_info = {
>  	.info	= nvme_hwmon_info,
>  };
>  
> +static void nvme_hwmon_smart_notify(struct work_struct *work)
> +{
> +	struct nvme_hwmon_data *data =
> +		container_of(work, struct nvme_hwmon_data, smart_work);
> +	struct nvme_ctrl *ctrl = data->ctrl;
> +	int ret;
> +
> +	/*
> +	 * The event stays latched on the controller until the SMART log
> +	 * is read with RAE cleared, so re-read it here before the
> +	 * completion path re-arms the Asynchronous Event Request.
> +	 */
> +	mutex_lock(&data->read_lock);
> +	ret = nvme_hwmon_get_smart_log(data);
> +	mutex_unlock(&data->read_lock);
> +	if (ret)
> +		return;
> +
> +	hwmon_notify_event(ctrl->hwmon_device, hwmon_temp,
> +			   hwmon_temp_alarm, 0);
> +}

[Severity: High]
Does queueing both data->smart_work and ctrl->async_event_work on nvme_wq
actually guarantee that the SMART log is read before the Asynchronous
Event Request is re-armed?

nvme_wq is allocated with WQ_UNBOUND and default max_active, so both work
items run concurrently:

CPU0                                  CPU1
nvme_complete_async_event()
  nvme_hwmon_smart_event(ctrl)
    queue_work(nvme_wq, &smart_work)
  queue_work(nvme_wq, &async_event_work)

nvme_hwmon_smart_notify()             nvme_async_event_work()
  nvme_hwmon_get_smart_log()            ctrl->ops->submit_async_event(ctrl)
    ... (sleeps on admin I/O)             <-- re-arms AER before RAE clear

Because submit_async_event() runs before the synchronous Get Log Page
command completes and clears the latched event, the controller can
immediately complete the new AER for the same latched SMART warning,
causing repeated AER completions and duplicate uevent and hwmon
notifications.  By comparison, NVME_AER_NOTICE_FW_ACT_STARTING sets
requeue = false in nvme_handle_aen_notice() and queues
ctrl->async_event_work from nvme_fw_act_work() only after reading the log
page.

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