mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH] nvmet: accept ANA group ID NVMET_MAX_ANAGRPS in configfs
@ 2026-09-10 15:48 Andrew Stellman
  2026-09-11 21:40 ` Sagi Grimberg
  0 siblings, 1 reply; 3+ messages in thread
From: Andrew Stellman @ 2026-09-10 15:48 UTC (permalink / raw)
  To: Christoph Hellwig, Sagi Grimberg, Chaitanya Kulkarni
  Cc: linux-nvme, linux-kernel, Andrew Stellman

nvmet_ns_ana_grpid_store() and nvmet_ana_groups_make_group() accept an
ANA group ID in the closed range 1..NVMET_MAX_ANAGRPS, but then pass it
through array_index_nospec() with NVMET_MAX_ANAGRPS as the size.  That
helper treats the size as a half-open bound, so the maximum valid ID,
128, is rewritten to 0.

nvmet_ana_group_enabled[] is NVMET_MAX_ANAGRPS + 1 entries wide, so
index 128 is a valid slot, and the controller advertises ANAGRPMAX =
NVMET_MAX_ANAGRPS.  NVMe Base Specification 2.4, section 8.1.1
(Asymmetric Namespace Access Reporting), "ANA Groups", defines a valid
ANA Group Identifier as "a non-zero value that is less than or equal to
ANAGRPMAX".

Two visible effects.  Writing 128 to a namespace's ana_grpid succeeds
but the namespace lands in the reserved group 0, which Identify
Namespace then reports.  Creating and removing ports/N/ana_groups/128
increments slot 0 on create but decrements slot 128 on release, leaving
nvmet_ana_group_enabled[128] at 0xffffffff, so every subsequent ANA log
page reports a group 128 with no namespaces in the Inaccessible state.

Use NVMET_MAX_ANAGRPS + 1 as the array_index_nospec() bound at both
sites, matching the array's actual size.  A namespace assigned to group
128 now follows the same path as any other group: its ANA state is
Inaccessible until ports/N/ana_groups/128 is created and configured,
where before it sat in group 0 with an uninitialized state and I/O was
allowed.

Tested on 7.3.0-rc1-qpb-cc-base+ (unpatched) and 7.3.0-rc1-qpb-cc-anagrpid+
(patched) in an arm64 QEMU guest with nvmet over NVMe/TCP to
127.0.0.1.  Before: ana_grpid written as 128 reads back 0, and the ANA
log after mkdir+rmdir of ana_groups/128 lists group 128 with nnsids 0,
state inaccessible.  After: ana_grpid reads back 128 and the ANA log
lists only group 1.

The issue was found by Claude Opus 5 running Quality Playbook, an
LLM-driven code review tool:
https://github.com/andrewstellman/quality-playbook

Fixes: 20dc66f2d76b ("nvme: prevent potential spectre v1 gadget")
Assisted-by: Claude:claude-opus-5 [Quality Playbook]
Signed-off-by: Andrew Stellman <astellman@stellman-greene.com>
---
 drivers/nvme/target/configfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 413ee2d16d29..0d4c69c4697a 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -698,7 +698,7 @@ static ssize_t nvmet_ns_ana_grpid_store(struct config_item *item,
 
 	down_write(&nvmet_ana_sem);
 	oldgrpid = ns->anagrpid;
-	newgrpid = array_index_nospec(newgrpid, NVMET_MAX_ANAGRPS);
+	newgrpid = array_index_nospec(newgrpid, NVMET_MAX_ANAGRPS + 1);
 	nvmet_ana_group_enabled[newgrpid]++;
 	ns->anagrpid = newgrpid;
 	nvmet_ana_group_enabled[oldgrpid]--;
@@ -1976,7 +1976,7 @@ static struct config_group *nvmet_ana_groups_make_group(
 	grp->grpid = grpid;
 
 	down_write(&nvmet_ana_sem);
-	grpid = array_index_nospec(grpid, NVMET_MAX_ANAGRPS);
+	grpid = array_index_nospec(grpid, NVMET_MAX_ANAGRPS + 1);
 	nvmet_ana_group_enabled[grpid]++;
 	up_write(&nvmet_ana_sem);
 

base-commit: 4d7d9486c04d917265f64c55bd23b2cc4fe7749c
-- 
2.53.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] nvmet: accept ANA group ID NVMET_MAX_ANAGRPS in configfs
  2026-09-10 15:48 [PATCH] nvmet: accept ANA group ID NVMET_MAX_ANAGRPS in configfs Andrew Stellman
@ 2026-09-11 21:40 ` Sagi Grimberg
  2026-09-11 21:41   ` Sagi Grimberg
  0 siblings, 1 reply; 3+ messages in thread
From: Sagi Grimberg @ 2026-09-11 21:40 UTC (permalink / raw)
  To: Andrew Stellman, Christoph Hellwig, Chaitanya Kulkarni
  Cc: linux-nvme, linux-kernel



On 10/09/2026 18:48, Andrew Stellman wrote:
> nvmet_ns_ana_grpid_store() and nvmet_ana_groups_make_group() accept an
> ANA group ID in the closed range 1..NVMET_MAX_ANAGRPS, but then pass it
> through array_index_nospec() with NVMET_MAX_ANAGRPS as the size.  That
> helper treats the size as a half-open bound, so the maximum valid ID,
> 128, is rewritten to 0.
>
> nvmet_ana_group_enabled[] is NVMET_MAX_ANAGRPS + 1 entries wide, so
> index 128 is a valid slot, and the controller advertises ANAGRPMAX =
> NVMET_MAX_ANAGRPS.  NVMe Base Specification 2.4, section 8.1.1
> (Asymmetric Namespace Access Reporting), "ANA Groups", defines a valid
> ANA Group Identifier as "a non-zero value that is less than or equal to
> ANAGRPMAX".
>
> Two visible effects.  Writing 128 to a namespace's ana_grpid succeeds
> but the namespace lands in the reserved group 0, which Identify
> Namespace then reports.  Creating and removing ports/N/ana_groups/128
> increments slot 0 on create but decrements slot 128 on release, leaving
> nvmet_ana_group_enabled[128] at 0xffffffff, so every subsequent ANA log
> page reports a group 128 with no namespaces in the Inaccessible state.
>
> Use NVMET_MAX_ANAGRPS + 1 as the array_index_nospec() bound at both
> sites, matching the array's actual size.  A namespace assigned to group
> 128 now follows the same path as any other group: its ANA state is
> Inaccessible until ports/N/ana_groups/128 is created and configured,
> where before it sat in group 0 with an uninitialized state and I/O was
> allowed.
>
> Tested on 7.3.0-rc1-qpb-cc-base+ (unpatched) and 7.3.0-rc1-qpb-cc-anagrpid+
> (patched) in an arm64 QEMU guest with nvmet over NVMe/TCP to
> 127.0.0.1.  Before: ana_grpid written as 128 reads back 0, and the ANA
> log after mkdir+rmdir of ana_groups/128 lists group 128 with nnsids 0,
> state inaccessible.  After: ana_grpid reads back 128 and the ANA log
> lists only group 1.
>
> The issue was found by Claude Opus 5 running Quality Playbook, an
> LLM-driven code review tool:
> https://github.com/andrewstellman/quality-playbook
>
> Fixes: 20dc66f2d76b ("nvme: prevent potential spectre v1 gadget")
> Assisted-by: Claude:claude-opus-5 [Quality Playbook]
> Signed-off-by: Andrew Stellman <astellman@stellman-greene.com>
> ---
>   drivers/nvme/target/configfs.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index 413ee2d16d29..0d4c69c4697a 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -698,7 +698,7 @@ static ssize_t nvmet_ns_ana_grpid_store(struct config_item *item,
>   
>   	down_write(&nvmet_ana_sem);
>   	oldgrpid = ns->anagrpid;
> -	newgrpid = array_index_nospec(newgrpid, NVMET_MAX_ANAGRPS);
> +	newgrpid = array_index_nospec(newgrpid, NVMET_MAX_ANAGRPS + 1);

There is a condition above:

>   	nvmet_ana_group_enabled[newgrpid]++;
>   	ns->anagrpid = newgrpid;
>   	nvmet_ana_group_enabled[oldgrpid]--;
> @@ -1976,7 +1976,7 @@ static struct config_group *nvmet_ana_groups_make_group(
>   	grp->grpid = grpid;
>   
>   	down_write(&nvmet_ana_sem);
> -	grpid = array_index_nospec(grpid, NVMET_MAX_ANAGRPS);
> +	grpid = array_index_nospec(grpid, NVMET_MAX_ANAGRPS + 1);
>   	nvmet_ana_group_enabled[grpid]++;
>   	up_write(&nvmet_ana_sem);
>   
>
> base-commit: 4d7d9486c04d917265f64c55bd23b2cc4fe7749c


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] nvmet: accept ANA group ID NVMET_MAX_ANAGRPS in configfs
  2026-09-11 21:40 ` Sagi Grimberg
@ 2026-09-11 21:41   ` Sagi Grimberg
  0 siblings, 0 replies; 3+ messages in thread
From: Sagi Grimberg @ 2026-09-11 21:41 UTC (permalink / raw)
  To: Andrew Stellman, Christoph Hellwig, Chaitanya Kulkarni
  Cc: linux-nvme, linux-kernel



On 12/09/2026 0:40, Sagi Grimberg wrote:
>
>
> On 10/09/2026 18:48, Andrew Stellman wrote:
>> nvmet_ns_ana_grpid_store() and nvmet_ana_groups_make_group() accept an
>> ANA group ID in the closed range 1..NVMET_MAX_ANAGRPS, but then pass it
>> through array_index_nospec() with NVMET_MAX_ANAGRPS as the size.  That
>> helper treats the size as a half-open bound, so the maximum valid ID,
>> 128, is rewritten to 0.
>>
>> nvmet_ana_group_enabled[] is NVMET_MAX_ANAGRPS + 1 entries wide, so
>> index 128 is a valid slot, and the controller advertises ANAGRPMAX =
>> NVMET_MAX_ANAGRPS.  NVMe Base Specification 2.4, section 8.1.1
>> (Asymmetric Namespace Access Reporting), "ANA Groups", defines a valid
>> ANA Group Identifier as "a non-zero value that is less than or equal to
>> ANAGRPMAX".
>>
>> Two visible effects.  Writing 128 to a namespace's ana_grpid succeeds
>> but the namespace lands in the reserved group 0, which Identify
>> Namespace then reports.  Creating and removing ports/N/ana_groups/128
>> increments slot 0 on create but decrements slot 128 on release, leaving
>> nvmet_ana_group_enabled[128] at 0xffffffff, so every subsequent ANA log
>> page reports a group 128 with no namespaces in the Inaccessible state.
>>
>> Use NVMET_MAX_ANAGRPS + 1 as the array_index_nospec() bound at both
>> sites, matching the array's actual size.  A namespace assigned to group
>> 128 now follows the same path as any other group: its ANA state is
>> Inaccessible until ports/N/ana_groups/128 is created and configured,
>> where before it sat in group 0 with an uninitialized state and I/O was
>> allowed.
>>
>> Tested on 7.3.0-rc1-qpb-cc-base+ (unpatched) and 
>> 7.3.0-rc1-qpb-cc-anagrpid+
>> (patched) in an arm64 QEMU guest with nvmet over NVMe/TCP to
>> 127.0.0.1.  Before: ana_grpid written as 128 reads back 0, and the ANA
>> log after mkdir+rmdir of ana_groups/128 lists group 128 with nnsids 0,
>> state inaccessible.  After: ana_grpid reads back 128 and the ANA log
>> lists only group 1.
>>
>> The issue was found by Claude Opus 5 running Quality Playbook, an
>> LLM-driven code review tool:
>> https://github.com/andrewstellman/quality-playbook
>>
>> Fixes: 20dc66f2d76b ("nvme: prevent potential spectre v1 gadget")
>> Assisted-by: Claude:claude-opus-5 [Quality Playbook]
>> Signed-off-by: Andrew Stellman <astellman@stellman-greene.com>
>> ---
>>   drivers/nvme/target/configfs.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/nvme/target/configfs.c 
>> b/drivers/nvme/target/configfs.c
>> index 413ee2d16d29..0d4c69c4697a 100644
>> --- a/drivers/nvme/target/configfs.c
>> +++ b/drivers/nvme/target/configfs.c
>> @@ -698,7 +698,7 @@ static ssize_t nvmet_ns_ana_grpid_store(struct 
>> config_item *item,
>>         down_write(&nvmet_ana_sem);
>>       oldgrpid = ns->anagrpid;
>> -    newgrpid = array_index_nospec(newgrpid, NVMET_MAX_ANAGRPS);
>> +    newgrpid = array_index_nospec(newgrpid, NVMET_MAX_ANAGRPS + 1);
>
> There is a condition above:

--
         if (grpid <= 1 || grpid > NVMET_MAX_ANAGRPS)
                 goto out;
--

I assume that this patch was not tested?

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-09-11 21:41 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-10 15:48 [PATCH] nvmet: accept ANA group ID NVMET_MAX_ANAGRPS in configfs Andrew Stellman
2026-09-11 21:40 ` Sagi Grimberg
2026-09-11 21:41   ` Sagi Grimberg

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®