[PATCH 0/4] target: fix bugs in Persistent Reservations

[PATCH 0/4] target: fix bugs in Persistent Reservations

[Dmitry Bogdanov]

This patch set fixes few rare bugs and deviations from standard in
PREEMPT AND ABORT and REGISTER AND MOVE operations.

Dmitry Bogdanov (4):
  target: core: fix preempt and abort for allreg res
  target: core: fix memory leak in preempt_and_abort
  target: core: abort all preempted regs if requested
  target: core: new key must be used for moved PR

 drivers/target/target_core_pr.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH 1/4] target: core: fix preempt and abort for allreg res

[Dmitry Bogdanov]

Match a key only if SARK is not zero according to SPC-4 and the comment
above the code:
 If an all registrants persistent reservation is present and the SERVICE
 ACTION RESERVATION KEY field is set to zero, then all registrations
 shall be removed except for that of the I_T nexus that is being used
 for the PERSISTENT RESERVE OUT command;

Without this patch in case of SARK==0 no registrants will be removed.

Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
---
 drivers/target/target_core_pr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index a1d67554709f..738ee7137462 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -3022,7 +3022,7 @@ core_scsi3_pro_preempt(struct se_cmd *cmd, int type, int scope, u64 res_key,
 		if (calling_it_nexus)
 			continue;
 
-		if (pr_reg->pr_res_key != sa_res_key)
+		if ((sa_res_key) && (pr_reg->pr_res_key != sa_res_key))
 			continue;
 
 		pr_reg_nacl = pr_reg->pr_reg_nacl;
-- 
2.25.1

[PATCH 2/4] target: core: fix memory leak in preempt_and_abort

[Dmitry Bogdanov]

Always release preempt_and_abort_list to avoid memory leak of
t10_pr_registration objects in it.

Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
---
 drivers/target/target_core_pr.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 738ee7137462..24441427f7b4 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -2956,13 +2956,14 @@ core_scsi3_pro_preempt(struct se_cmd *cmd, int type, int scope, u64 res_key,
 			__core_scsi3_complete_pro_preempt(dev, pr_reg_n,
 				(preempt_type == PREEMPT_AND_ABORT) ? &preempt_and_abort_list : NULL,
 				type, scope, preempt_type);
-
-			if (preempt_type == PREEMPT_AND_ABORT)
-				core_scsi3_release_preempt_and_abort(
-					&preempt_and_abort_list, pr_reg_n);
 		}
+
 		spin_unlock(&dev->dev_reservation_lock);
 
+		if (preempt_type == PREEMPT_AND_ABORT)
+			core_scsi3_release_preempt_and_abort(
+				&preempt_and_abort_list, pr_reg_n);
+
 		if (pr_tmpl->pr_aptpl_active)
 			core_scsi3_update_and_write_aptpl(cmd->se_dev, true);
 
-- 
2.25.1

[PATCH 3/4] target: core: abort all preempted regs if requested

[Dmitry Bogdanov]

According to SPC the preempted commands shall be always aborted.

SPC-4: 5.12.11.2.6 Preempting and aborting
c) all commands from the I_T nexus(es) associated with the persistent
reservations or registrations being preempted (i.e., preempted commands)
except the PERSISTENT RESERVE OUT command itself shall be aborted as
defined in SAM-5;

Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
---
 drivers/target/target_core_pr.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 24441427f7b4..8c83c5530536 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -2960,9 +2960,23 @@ core_scsi3_pro_preempt(struct se_cmd *cmd, int type, int scope, u64 res_key,
 
 		spin_unlock(&dev->dev_reservation_lock);
 
-		if (preempt_type == PREEMPT_AND_ABORT)
+		/*
+		 * SPC-4 5.12.11.2.6 Preempting and aborting
+		 * The actions described in this subclause shall be performed
+		 * for all I_T nexuses that are registered with the non-zero
+		 * SERVICE ACTION RESERVATION KEY value, without regard for
+		 * whether the preempted I_T nexuses hold the persistent
+		 * reservation. If the SERVICE ACTION RESERVATION KEY field is
+		 * set to zero and an all registrants persistent reservation is
+		 * present, the device server shall abort all commands for all
+		 * registered I_T nexuses.
+		 */
+		if (preempt_type == PREEMPT_AND_ABORT) {
+			core_tmr_lun_reset(dev, NULL, &preempt_and_abort_list,
+					   cmd);
 			core_scsi3_release_preempt_and_abort(
 				&preempt_and_abort_list, pr_reg_n);
+		}
 
 		if (pr_tmpl->pr_aptpl_active)
 			core_scsi3_update_and_write_aptpl(cmd->se_dev, true);
-- 
2.25.1

[PATCH 4/4] target: core: new key must be used for moved PR

[Dmitry Bogdanov]

According to SPC4 5.12.8:
e) Retain the reservation key specified in the SERVICE ACTION
RESERVATION KEY field and associated information;

But currently sa_res_key is only used for the not existing I_T nexus.
The patch adds an updating of the key for the existing I_T nexus the PR
moved to.

Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
---
 drivers/target/target_core_pr.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 8c83c5530536..fc7b2e2c17a5 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -3440,8 +3440,6 @@ core_scsi3_emulate_pro_register_and_move(struct se_cmd *cmd, u64 res_key,
 	 *       transport protocols where port names are not required;
 	 * d) Register the reservation key specified in the SERVICE ACTION
 	 *    RESERVATION KEY field;
-	 * e) Retain the reservation key specified in the SERVICE ACTION
-	 *    RESERVATION KEY field and associated information;
 	 *
 	 * Also, It is not an error for a REGISTER AND MOVE service action to
 	 * register an I_T nexus that is already registered with the same
@@ -3463,6 +3461,12 @@ core_scsi3_emulate_pro_register_and_move(struct se_cmd *cmd, u64 res_key,
 		dest_pr_reg = __core_scsi3_locate_pr_reg(dev, dest_node_acl,
 						iport_ptr);
 		new_reg = 1;
+	} else {
+	/*
+	 * e) Retain the reservation key specified in the SERVICE ACTION
+	 *    RESERVATION KEY field and associated information;
+	 */
+		dest_pr_reg->pr_res_key = sa_res_key;
 	}
 	/*
 	 * f) Release the persistent reservation for the persistent reservation
-- 
2.25.1

Re: [PATCH 1/4] target: core: fix preempt and abort for allreg res

[Bart Van Assche]

On 9/7/22 06:58, Dmitry Bogdanov wrote:
> -		if (pr_reg->pr_res_key != sa_res_key)
> +		if ((sa_res_key) && (pr_reg->pr_res_key != sa_res_key))
>   			continue;

Please do not introduce superfluous parentheses. Four parentheses can be 
left out from the above code without affecting readability.

Thanks,

Bart.

Re: [PATCH 4/4] target: core: new key must be used for moved PR

[Bart Van Assche]

On 9/7/22 06:58, Dmitry Bogdanov wrote:
> +	} else {
> +	/*
> +	 * e) Retain the reservation key specified in the SERVICE ACTION
> +	 *    RESERVATION KEY field and associated information;
> +	 */
> +		dest_pr_reg->pr_res_key = sa_res_key;
>   	}

The indentation of the new comment looks wrong to me. Please fix.

Thanks,

Bart.

Re: [PATCH 1/4] target: core: fix preempt and abort for allreg res

[Dmitry Bogdanov]

On Wed, Sep 07, 2022 at 09:18:57AM -0700, Bart Van Assche wrote:
> 
> On 9/7/22 06:58, Dmitry Bogdanov wrote:
> > -             if (pr_reg->pr_res_key != sa_res_key)
> > +             if ((sa_res_key) && (pr_reg->pr_res_key != sa_res_key))
> >                       continue;
> 
> Please do not introduce superfluous parentheses. Four parentheses can be
> left out from the above code without affecting readability.
Ok, will remove.

BR,
 Dmitry

Re: [PATCH 4/4] target: core: new key must be used for moved PR

[Dmitry Bogdanov]

On Wed, Sep 07, 2022 at 09:19:46AM -0700, Bart Van Assche wrote:
> 
> On 9/7/22 06:58, Dmitry Bogdanov wrote:
> > +     } else {
> > +     /*
> > +      * e) Retain the reservation key specified in the SERVICE ACTION
> > +      *    RESERVATION KEY field and associated information;
> > +      */
> > +             dest_pr_reg->pr_res_key = sa_res_key;
> >       }
> 
> The indentation of the new comment looks wrong to me. Please fix.
Oh, yes, forgot to add a tab there.

BR,
 Dmitry