Re: [PATCH v2] nvmet: Handle a NULL admin queue in nvmet_queue_tls_keyid()

[Guixin Liu <kanie@linux.alibaba.com>]

Hi Christoph,
  Any feedback?

Best Regards,
Guixin Liu

在 2026/10/30 10:36, Guixin Liu 写道:
>
> 在 2026/10/29 20:15, Christoph Hellwig 写道:
>> On Thu, Oct 29, 2026 at 11:43:06AM +0800, Guixin Liu wrote:
>>> The admin queue is not necessarily installed when the tls_key debugfs
>>> entry is read: it may not have been created yet, or it may have been
>>> destroyed already.  nvmet_queue_tls_keyid() then dereferences a NULL
>>> sq.
>>>
>>> Guard the helper and return 0 instead, which also covers the read
>>> racing with admin queue teardown.
>> The patch itself looks obviously fine, but do we have any synchronization
>> that ensure it does not go away while we're looking at the submission
>> queue?
> The nvmet_sq has percpu_ref, but it can keep nvmet_sq_destroy() from
> freeing sq only, it can't keep nvmet_tcp_release_queue_work() from
> setting tls_key to NULL, so we need serialize with nvmet_sq_put_tls_key():
>
> """""
> diff --git a/drivers/nvme/target/debugfs.c b/drivers/nvme/target/debugfs.c
> index e85fe1d4c9f8..cc451e25ea1d 100644
> --- a/drivers/nvme/target/debugfs.c
> +++ b/drivers/nvme/target/debugfs.c
> @@ -136,8 +136,19 @@ NVMET_DEBUGFS_ATTR(nvmet_ctrl_host_traddr);
>  static int nvmet_ctrl_tls_key_show(struct seq_file *m, void *p)
>  {
>         struct nvmet_ctrl *ctrl = m->private;
> -       key_serial_t keyid = nvmet_queue_tls_keyid(ctrl->sqs[0]);
> -
> +       struct nvmet_sq *sq = ctrl->sqs[0];
> +       key_serial_t keyid = 0;
> +
> +       /*
> +        * Pin the admin queue against teardown, and serialize with
> +        * nvmet_sq_put_tls_key() so the key cannot be dropped mid-read.
> +        */
> +       if (sq && percpu_ref_tryget_live(&sq->ref)) {
> +               mutex_lock(&ctrl->lock);
> +               keyid = nvmet_queue_tls_keyid(sq);
> +               mutex_unlock(&ctrl->lock);
> +               percpu_ref_put(&sq->ref);
> +       }
>         seq_printf(m, "%08x\n", keyid);
>         return 0;
>  }
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index e315f1c24e73..24df9e4a1e1c 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -957,13 +957,17 @@ static inline void nvmet_req_bio_put(struct nvmet_req *req, struct bio *bio)
>  #ifdef CONFIG_NVME_TARGET_TCP_TLS
>  static inline key_serial_t nvmet_queue_tls_keyid(struct nvmet_sq *sq)
>  {
> +       if (!sq)
> +               return 0;
>         return sq->tls_key ? key_serial(sq->tls_key) : 0;
>  }
>  static inline void nvmet_sq_put_tls_key(struct nvmet_sq *sq)
>  {
>         if (sq->tls_key) {
> +               mutex_lock(&sq->ctrl->lock);
>                 key_put(sq->tls_key);
>                 sq->tls_key = NULL;
> +               mutex_unlock(&sq->ctrl->lock);
>         }
>  }
> """"
>
> Best Regards,
> Guixin Liu