[PATCH 0/2] nvmet: fix Reservation Acquire preempt sent by the holder

[PATCH 0/2] nvmet: fix Reservation Acquire preempt sent by the holder

[Daejun Park]

nvmet_pr_preempt() does not follow NVMe Base Specification 2.1, section
8.1.22.7 when a Reservation Acquire command with the Preempt or Preempt
and Abort action is sent by the reservation holder and the reservation
is not of an All Registrants type.

Patch 1 fixes a use-after-free read in the path that handles a preempt
sent by the holder.  It does not depend on patch 2 and comes first so
that it can be backported alone.

Patch 2: a holder that preempts with the key of another registrant gets
a successful completion, but that registrant stays registered and the
reservation type is changed to RTYPE.  The pNFS SCSI layout server
fences clients with this command (RFC 9561 section 2.2.3), so on an
nvmet namespace nfsd logs a client as fenced while the client can still
write to the device.  The patch lists what changes for a host that
holds the reservation.

Not changed by this series: when the holder preempts itself,
registrants that share its key stay registered.  Case a) of section
8.1.22.7 unregisters them, but the paragraph about a holder preempting
itself lists three effects and does not mention other registrants, so I
left it alone.  The SCSI target removes them in
core_scsi3_emulate_pro_preempt().  I can send a patch if that is the
intended reading.

The series is based on the block-7.3 branch of the block tree at
684b413b5483, which has the current nvme fixes, and was tested there
with KASAN, PROVE_LOCKING and PROVE_RCU enabled, in QEMU guests, with
nvme-cli 2.8.  drivers/nvme/target/pr.c is the
same file there, in nvme-7.3, in nvme-7.4 and in mainline.  "before" is
that commit, "after" is that commit with both patches.  Each case was
run once unless noted.  The runs with the patches applied produced no
KASAN or lockdep report.  Mainline ce1e0223d8ad, which does not have
"nvmet: copy the hostid into the ctrl before creating PR pc_refs" yet,
gave the same results before and after.

1. nvme-loop, hosts A, B and C registered on one namespace with
   different keys, A holds the reservation.  "regs" is the number of
   registrants after the command.  EA-RO is Exclusive Access -
   Registrants Only, WE Write Exclusive, WE-RO Write Exclusive -
   Registrants Only, EA-AR Exclusive Access - All Registrants.

   reservation and command                     before          after
   EA-RO, A preempts key of B, same RTYPE      regs 3          regs 2
   EA-RO, A preempts key of B, other RTYPE     regs 3, type    regs 2, type
                                               changed         unchanged
   EA-RO, A preempts and aborts key of B       regs 3          regs 2
   WE,    A preempts key of B                  regs 3          regs 2
   WE-RO, A preempts and aborts key of B       regs 3          regs 2
   EA-RO, A preempts with PRKEY 0h,            success, type   Invalid Field
          other RTYPE                          changed         in Command
   EA-RO, A preempts with an unregistered      success         Reservation
          key, same RTYPE                                      Conflict
   EA-RO, A preempts itself with another RTYPE regs 3, type    same
                                               changed
   EA-RO, C preempts key of B                  regs 2          same
   EA-AR, A preempts key of B                  regs 2          same
   EA-AR, A preempts with PRKEY 0h             regs 1          same

   With EA-RO and WE-RO, writes from B succeed while B is registered and
   fail with Reservation Conflict once it is unregistered.

2. blktests nvme/054 passes before and after, with nvme-loop and
   nvme-tcp.  A test for the multi host cases is posted separately as
   "[PATCH blktests] nvme/071: test reservation preempt with multiple
   hosts".  It fails before and passes after (nvme-loop three times,
   nvme-tcp once, and both transports in one run).

3. pNFS SCSI layout.  An nfsd server exports XFS on an nvmet-tcp
   namespace to two clients, all nodes run the same kernel.  The server
   and both clients are registered, the server holds an EA-RO
   reservation.  Client 1 holds a layout and writes 1 MiB with O_DIRECT
   every 0.2 seconds.  Its network link to the server is then blocked
   while its link to the NVMe target stays up, and a local write to the
   file on the server recalls the layout.  The server write waits for
   the lease break time (45 seconds by default), then nfsd fences
   client 1 and logs "FENCED client[...]" in both cases.  The link is
   restored about 15 seconds after the fence, and the RPC counter is
   read when client 1 stops writing.

                                               before          after
   seconds the server write waited             49              49
   registrants after the fence                 3               2
   bytes client 1 wrote to the device in the
   10 seconds after the fence                  45088768        0
   WRITE RPCs sent by client 1 by the end      0               172

   After the fence client 1 logs "reservation conflict error, dev
   nvme0c0n1" and, once the link is back, sends its writes to the
   server.

   A run with the patches in which client 1 was cut off from both the
   server and the NVMe target gave 45 seconds, 2 registrants and 0
   bytes; the fence did not block on the unreachable client.

Daejun Park (2):
  nvmet: fix use-after-free of the old registrant in nvmet_pr_preempt()
  nvmet: fix preempt of another registrant by the reservation holder

 drivers/nvme/target/pr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


base-commit: 684b413b5483f57c890c171b9400076a0143b918
-- 
2.43.0

[PATCH 1/2] nvmet: fix use-after-free of the old registrant in nvmet_pr_preempt()

[Daejun Park]

nvmet_pr_preempt() handles a preempt sent by the reservation holder by
changing the reservation type with nvmet_pr_update_reg_attr(), unless
the reservation is of an All Registrants type.  For the holder that
helper does not modify the registrant in place: it puts a copy on the
registrant list, points pr->holder to the copy and passes the old
registrant to kfree_rcu().

If the reservation type changed, nvmet_pr_preempt() then hands
&reg->hostid of that old registrant to nvmet_pr_resv_released().  Unless
the reservation released notification is masked, that function takes
subsys->lock and compares the host id against every controller of the
subsystem.  The caller only holds pr->pr_sem and is not in an RCU
read-side critical section, so nothing keeps the old registrant alive
while it is read.

The result is a read of 16 freed bytes that decides which controllers
get the notification.  Found by code inspection.  kfree_rcu() defers
the free, so it usually happens after the read; when the pending free
is forced to run before the read, KASAN reports the host id as freed
memory.

Pass the host id of the controller that sent the command instead.  The
registrant was looked up by that host id, and the controller outlives
the request.

Fixes: 5a47c2080a73 ("nvmet: support reservation feature")
Cc: stable@vger.kernel.org
Signed-off-by: Daejun Park <daejun7.park@samsung.com>
---

Notes:
    The KASAN report came from this change on top of the base commit, which
    forces the pending free and then checks the pointer that is handed to
    nvmet_pr_resv_released():
    
     		status = nvmet_pr_update_reg_attr(pr, holder,
     				nvmet_pr_update_holder_rtype, &rtype);
    +		kvfree_rcu_barrier();
    +		kasan_check_read(&reg->hostid, sizeof(reg->hostid));
     		if (!status && original_rtype != rtype)
     			nvmet_pr_resv_released(pr, &reg->hostid);
    
    A holder that preempts itself with another reservation type then gives:
    
      BUG: KASAN: slab-use-after-free in nvmet_pr_preempt.isra.0+0x462/0xa50
      Read of size 16 at addr ff110001113bb088 by task kworker/u32:0/12
      ...
      Allocated by task 12:
       ...
       nvmet_execute_pr_register+0x1f4/0x1130
      ...
      Freed by task 864:
       ...
       __rcu_free_sheaf_prepare+0x61/0x230
       rcu_free_sheaf_nobarn+0x1d/0x50
       rcu_core+0x691/0x1e20
    
    The free in this report is the one forced by the added barrier.  The
    same two lines with &ctrl->hostid on top of this patch give no report.

 drivers/nvme/target/pr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/target/pr.c b/drivers/nvme/target/pr.c
index 09d8c63f5..bfd5447f6 100644
--- a/drivers/nvme/target/pr.c
+++ b/drivers/nvme/target/pr.c
@@ -589,7 +589,7 @@ static u16 nvmet_pr_preempt(struct nvmet_req *req,
 		status = nvmet_pr_update_reg_attr(pr, holder,
 				nvmet_pr_update_holder_rtype, &rtype);
 		if (!status && original_rtype != rtype)
-			nvmet_pr_resv_released(pr, &reg->hostid);
+			nvmet_pr_resv_released(pr, &ctrl->hostid);
 		return status;
 	}
 
-- 
2.43.0

Re: [PATCH 1/2] nvmet: fix use-after-free of the old registrant in nvmet_pr_preempt()

[hch]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 2/2] nvmet: fix preempt of another registrant by the reservation holder

[Daejun Park]

Unless the reservation is of an All Registrants type, nvmet_pr_preempt()
treats every Reservation Acquire command with the Preempt or Preempt and
Abort action that is sent by the reservation holder as the holder
preempting itself.  It updates the reservation type to RTYPE and
completes the command successfully without looking at PRKEY.

NVMe Base Specification 2.1, section 8.1.22.7 makes the action depend
on PRKEY for these reservation types, no matter which registrant sends
the command:

 - PRKEY matches the key of the reservation holder: the reservation is
   preempted.  This is how a holder preempts itself.
 - PRKEY does not match the key of the holder and is not 0h: the
   registrants whose key matches PRKEY are unregistered.
 - PRKEY does not match the key of the holder and is 0h: the command is
   aborted with Invalid Field in Command.

With the current code, a holder that preempts with the key of another
registrant leaves that registrant registered, changes the reservation
type to RTYPE and still gets a successful completion.  With hosts A, B
and C registered with the keys 0xa, 0xb and 0xc, and A holding an
Exclusive Access - Registrants Only reservation, A sends:

  # nvme resv-acquire /dev/nvme1 -n 1 --crkey=0xa --prkey=0xb \
        --rtype=4 --racqa=1
  NVME Reservation Acquire success
  # nvme resv-report /dev/nvme1 -n 1 --eds | grep '^regctl '
  regctl    : 3

and B can still write to the namespace.  Preempt and Abort (--racqa=2)
gives the same result.  When C sends the preempt instead (--crkey=0xc),
B is unregistered and its writes fail with Reservation Conflict.

This defeats fencing in the pNFS SCSI layout server.  nfsd holds the
reservation and fences a client that does not return its layout by
calling pr_preempt() with the key of that client and the abort flag,
which is the command RFC 9561 section 2.2.3 asks for.  On an nvmet
namespace the preempt succeeds, nfsd logs the client as fenced, and the
client keeps writing to the device.  In a test with an nfsd server and
two clients on an nvmet-tcp namespace, the namespace still had three
registrants after the fence and the fenced client went on writing to
it.  With this patch two registrants are left and the writes of the
fenced client fail with a reservation conflict.

Take the self preempt path only when PRKEY is the key of the holder.
Any other PRKEY sent by the holder is then handled by the code that
already serves a non-holder registrant, for Preempt and for Preempt and
Abort.  For a host that holds the reservation this changes the
following:

 - A preempt with the key of another registrant unregisters the
   registrants with that key, sends them the registration preempted
   notification and leaves the reservation type alone.
 - A preempt with PRKEY 0h fails with Invalid Field in Command.  A
   holder that changed its reservation type this way has to pass its
   own key in PRKEY, which works with and without this patch.
 - A preempt with a PRKEY that matches no registrant fails with
   Reservation Conflict.  The specification defines that status only
   for the All Registrants types.  It is what nvmet already returns to
   a non-holder registrant in this case and what the SCSI target
   returns for an unregistered key.  nfsd treats it as a successful
   fence.

Not changed: when the holder preempts itself, registrants that share
its key stay registered.

Fixes: 5a47c2080a73 ("nvmet: support reservation feature")
Cc: stable@vger.kernel.org
Signed-off-by: Daejun Park <daejun7.park@samsung.com>
---
 drivers/nvme/target/pr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/target/pr.c b/drivers/nvme/target/pr.c
index bfd5447f6..665cee8fc 100644
--- a/drivers/nvme/target/pr.c
+++ b/drivers/nvme/target/pr.c
@@ -585,7 +585,7 @@ static u16 nvmet_pr_preempt(struct nvmet_req *req,
 				&ctrl->hostid, abort);
 	}
 
-	if (holder == reg) {
+	if (holder == reg && prkey == holder->rkey) {
 		status = nvmet_pr_update_reg_attr(pr, holder,
 				nvmet_pr_update_holder_rtype, &rtype);
 		if (!status && original_rtype != rtype)
-- 
2.43.0

Re: [PATCH 2/2] nvmet: fix preempt of another registrant by the reservation holder

[hch]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 0/2] nvmet: fix Reservation Acquire preempt sent by the holder

[Keith Busch]

On Fri, Oct 02, 2026 at 03:59:31PM +0900, Daejun Park wrote:
> nvmet_pr_preempt() does not follow NVMe Base Specification 2.1, section
> 8.1.22.7 when a Reservation Acquire command with the Preempt or Preempt
> and Abort action is sent by the reservation holder and the reservation
> is not of an All Registrants type.
> 
> Patch 1 fixes a use-after-free read in the path that handles a preempt
> sent by the holder.  It does not depend on patch 2 and comes first so
> that it can be backported alone.
> 
> Patch 2: a holder that preempts with the key of another registrant gets
> a successful completion, but that registrant stays registered and the
> reservation type is changed to RTYPE.  The pNFS SCSI layout server
> fences clients with this command (RFC 9561 section 2.2.3), so on an
> nvmet namespace nfsd logs a client as fenced while the client can still
> write to the device.  The patch lists what changes for a host that
> holds the reservation.

Thanks, applied to nvme-7.4.