Re: [PATCH] target: generate correct string identifiers for PR OUT transport IDs

[Dmitry Bogdanov <d.bogdanov@yadro.com>]

On Fri, Jun 27, 2025 at 04:52:29PM +0200, Maurizio Lombardi wrote:
> 
> Fix target_parse_pr_out_transport_id() to
> return a dynamically allocated string representing the
> transport ID in a standardized, human-readable format
> (e.g., naa.xxxxxxxx...) for various SCSI protocol types
> (SAS, FCP, SRP, SBP).

There is no a single standard how to represent TransportId. There are
several standards for the same port address. But TransportId itself is
the single standard to represent different port names in binary format.
So, the most correct solution would be have TransportId for ACLs and
to match them.

> 
> Previously, the function returned a pointer to the raw binary buffer.
> The caller would then compared it to a human-readable string,
> which obviously always failed.
>
> Now, the function constructs a string using kasprintf()
> based on the protocol's offset and format:
> 
> * SAS, FCP, SBP: 64-bit identifier
> * SRP: 128-bit identifier
> * iSCSI: duplicates the iqn string to match the new allocation behavior

The caller compares it with a specifically prepared string buffer by
fabric module in its own way. And they are not corresponded any
standard and, that especially important, to your code.
Mostly that string contains only hex-representation without any prefixes.
You may grep target_setup_session to check which transport
generates what a string.


There was a patch[1] from my old RFC patchset that addressed that
issue. It can not be applied to upstream due to dependencies on other
patches. But a generation of string representation of TransportID is
correct there.
You may get it to create a correct fix, or let me create new patch with
my solution that could be applied to upstream.

[1] https://patchwork.kernel.org/project/target-devel/patch/20220803162857.27770-28-d.bogdanov@yadro.com/

BR,
 Dmitry

> 
> Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
> ---
>  drivers/target/target_core_fabric_lib.c | 22 ++++++++++++++++------
>  drivers/target/target_core_pr.c         |  7 ++++++-
>  2 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/target/target_core_fabric_lib.c b/drivers/target/target_core_fabric_lib.c
> index 43f47e3aa448..a124cf982a4c 100644
> --- a/drivers/target/target_core_fabric_lib.c
> +++ b/drivers/target/target_core_fabric_lib.c
> @@ -390,7 +390,10 @@ int target_get_pr_transport_id(struct se_node_acl *nacl,
>  const char *target_parse_pr_out_transport_id(struct se_portal_group *tpg,
>                 char *buf, u32 *out_tid_len, char **port_nexus_ptr)
>  {
> -       u32 offset;
> +       u32 offset = 8;
> +       u32 len = 8;
> +       char *prefix;
> +       char hex[40];
> 
>         switch (tpg->proto_id) {
>         case SCSI_PROTOCOL_SAS:
> @@ -399,15 +402,21 @@ const char *target_parse_pr_out_transport_id(struct se_portal_group *tpg,
>                  * for initiator ports using SCSI over SAS Serial SCSI Protocol.
>                  */
>                 offset = 4;
> +               prefix = "naa";
>                 break;
> -       case SCSI_PROTOCOL_SBP:
>         case SCSI_PROTOCOL_SRP:
> +               prefix = "ib";
> +               len = 16;
> +               break;
>         case SCSI_PROTOCOL_FCP:
> -               offset = 8;
> +               prefix = "naa";
> +               break;
> +       case SCSI_PROTOCOL_SBP:
> +               prefix = "eui";
>                 break;
>         case SCSI_PROTOCOL_ISCSI:
> -               return iscsi_parse_pr_out_transport_id(tpg, buf, out_tid_len,
> -                                       port_nexus_ptr);
> +               return kstrdup(iscsi_parse_pr_out_transport_id(tpg, buf,
> +                               out_tid_len, port_nexus_ptr), GFP_KERNEL);
>         default:
>                 pr_err("Unknown proto_id: 0x%02x\n", tpg->proto_id);
>                 return NULL;
> @@ -415,5 +424,6 @@ const char *target_parse_pr_out_transport_id(struct se_portal_group *tpg,
> 
>         *port_nexus_ptr = NULL;
>         *out_tid_len = 24;
> -       return buf + offset;
> +       bin2hex(hex, buf + offset, len);
> +       return kasprintf(GFP_KERNEL, "%s.%s", prefix, hex);
>  }
> diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
> index 70905805cb17..b9b3adc1657d 100644
> --- a/drivers/target/target_core_pr.c
> +++ b/drivers/target/target_core_pr.c
> @@ -1571,6 +1571,7 @@ core_scsi3_decode_spec_i_port(
>                         dest_rtpi = tmp_lun->lun_tpg->tpg_rtpi;
> 
>                         iport_ptr = NULL;
> +                       kfree(i_str);
>                         i_str = target_parse_pr_out_transport_id(tmp_tpg,
>                                         ptr, &tid_len, &iport_ptr);
>                         if (!i_str)
> @@ -1808,6 +1809,7 @@ core_scsi3_decode_spec_i_port(
>                 core_scsi3_tpg_undepend_item(dest_tpg);
>         }
> 
> +       kfree(i_str);
>         return 0;
>  out_unmap:
>         transport_kunmap_data_sg(cmd);
> @@ -1852,6 +1854,7 @@ core_scsi3_decode_spec_i_port(
>                 core_scsi3_nodeacl_undepend_item(dest_node_acl);
>                 core_scsi3_tpg_undepend_item(dest_tpg);
>         }
> +       kfree(i_str);
>         return ret;
>  }
> 
> @@ -3153,7 +3156,7 @@ core_scsi3_emulate_pro_register_and_move(struct se_cmd *cmd, u64 res_key,
>         struct t10_pr_registration *pr_reg, *pr_res_holder, *dest_pr_reg;
>         struct t10_reservation *pr_tmpl = &dev->t10_pr;
>         unsigned char *buf;
> -       const unsigned char *initiator_str;
> +       const unsigned char *initiator_str = NULL;
>         char *iport_ptr = NULL, i_buf[PR_REG_ISID_ID_LEN] = { };
>         u32 tid_len, tmp_tid_len;
>         int new_reg = 0, type, scope, matching_iname;
> @@ -3526,6 +3529,7 @@ core_scsi3_emulate_pro_register_and_move(struct se_cmd *cmd, u64 res_key,
>         core_scsi3_update_and_write_aptpl(cmd->se_dev, aptpl);
> 
>         core_scsi3_put_pr_reg(dest_pr_reg);
> +       kfree(initiator_str);
>         return 0;
>  out:
>         if (buf)
> @@ -3538,6 +3542,7 @@ core_scsi3_emulate_pro_register_and_move(struct se_cmd *cmd, u64 res_key,
> 
>  out_put_pr_reg:
>         core_scsi3_put_pr_reg(pr_reg);
> +       kfree(initiator_str);
>         return ret;
>  }
> 
> --
> 2.47.1
> 
>