* Re: [PATCH] target: fix truncated PR-in ReadKeys response
2018-05-31 22:20 [PATCH] target: fix truncated PR-in ReadKeys response David Disseldorp
@ 2018-06-01 19:27 ` Mike Christie
2018-06-04 11:16 ` Maged Mokhtar
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Mike Christie @ 2018-06-01 19:27 UTC (permalink / raw)
To: target-devel
On 05/31/2018 05:20 PM, David Disseldorp wrote:
> SPC5r17 states that the contents of the ADDITIONAL LENGTH field are not
> altered based on the allocation length, so always calculate and pack the
> full key list length even if the list itself is truncated.
>
> This behaviour can be tested using the libiscsi PrinReadKeys.Truncate
> test.
>
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
> drivers/target/target_core_pr.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
> index 01ac306131c1..2e865fdaa362 100644
> --- a/drivers/target/target_core_pr.c
> +++ b/drivers/target/target_core_pr.c
> @@ -3727,11 +3727,16 @@ core_scsi3_pri_read_keys(struct se_cmd *cmd)
> * Check for overflow of 8byte PRI READ_KEYS payload and
> * next reservation key list descriptor.
> */
> - if ((add_len + 8) > (cmd->data_length - 8))
> - break;
> -
> - put_unaligned_be64(pr_reg->pr_res_key, &buf[off]);
> - off += 8;
> + if ((off + 8) <= cmd->data_length) {
> + put_unaligned_be64(pr_reg->pr_res_key, &buf[off]);
> + off += 8;
> + }
> + /*
> + * SPC5r17: 6.16.2 READ KEYS service action
> + * The ADDITIONAL LENGTH field indicates the number of bytes in
> + * the Reservation key list. The contents of the ADDITIONAL
> + * LENGTH field are not altered based on the allocation length
> + */
> add_len += 8;
> }
> spin_unlock(&dev->t10_pr.registration_lock);
>
Looks ok to me.
Reviewed-by: Mike Christie <mchristi@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] target: fix truncated PR-in ReadKeys response
2018-05-31 22:20 [PATCH] target: fix truncated PR-in ReadKeys response David Disseldorp
2018-06-01 19:27 ` Mike Christie
@ 2018-06-04 11:16 ` Maged Mokhtar
2018-06-04 11:36 ` David Disseldorp
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Maged Mokhtar @ 2018-06-04 11:16 UTC (permalink / raw)
To: target-devel
--------------------------------------------------
From: "David Disseldorp" <ddiss@suse.de>
Sent: Friday, June 01, 2018 1:20 AM
To: <target-devel@vger.kernel.org>
Cc: "ronnie sahlberg" <ronniesahlberg@gmail.com>; "David Disseldorp"
<ddiss@suse.de>
Subject: [PATCH] target: fix truncated PR-in ReadKeys response
> SPC5r17 states that the contents of the ADDITIONAL LENGTH field are not
> altered based on the allocation length, so always calculate and pack the
> full key list length even if the list itself is truncated.
>
> This behaviour can be tested using the libiscsi PrinReadKeys.Truncate
> test.
>
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
> drivers/target/target_core_pr.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/target/target_core_pr.c
> b/drivers/target/target_core_pr.c
> index 01ac306131c1..2e865fdaa362 100644
> --- a/drivers/target/target_core_pr.c
> +++ b/drivers/target/target_core_pr.c
> @@ -3727,11 +3727,16 @@ core_scsi3_pri_read_keys(struct se_cmd *cmd)
> * Check for overflow of 8byte PRI READ_KEYS payload and
> * next reservation key list descriptor.
> */
> - if ((add_len + 8) > (cmd->data_length - 8))
> - break;
> -
> - put_unaligned_be64(pr_reg->pr_res_key, &buf[off]);
> - off += 8;
> + if ((off + 8) <= cmd->data_length) {
> + put_unaligned_be64(pr_reg->pr_res_key, &buf[off]);
> + off += 8;
> + }
> + /*
> + * SPC5r17: 6.16.2 READ KEYS service action
> + * The ADDITIONAL LENGTH field indicates the number of bytes in
> + * the Reservation key list. The contents of the ADDITIONAL
> + * LENGTH field are not altered based on the allocation length
> + */
> add_len += 8;
> }
> spin_unlock(&dev->t10_pr.registration_lock);
> --
> 2.13.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe target-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
This also fixes an issue in Windows server 2016 failover cluster with
many client connections, the initial allocation length sent in cdb is
72 bytes which limits it to 8 keys, with additional length not affected
by truncation, it will retry with correct size.
Interesting I was looking at the same issue in target_core_rbd.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] target: fix truncated PR-in ReadKeys response
2018-05-31 22:20 [PATCH] target: fix truncated PR-in ReadKeys response David Disseldorp
2018-06-01 19:27 ` Mike Christie
2018-06-04 11:16 ` Maged Mokhtar
@ 2018-06-04 11:36 ` David Disseldorp
2018-06-04 19:42 ` Maged Mokhtar
2018-06-04 20:20 ` David Disseldorp
4 siblings, 0 replies; 6+ messages in thread
From: David Disseldorp @ 2018-06-04 11:36 UTC (permalink / raw)
To: target-devel
On Mon, 4 Jun 2018 14:16:45 +0300, Maged Mokhtar wrote:
> This also fixes an issue in Windows server 2016 failover cluster with
> many client connections, the initial allocation length sent in cdb is
> 72 bytes which limits it to 8 keys, with additional length not affected
> by truncation, it will retry with correct size.
Interesting coincidence :) I only ran into this while looking at the
spec for the enable_pr change. Can you confirm that this fixes Win 2016
PR behaviour with an iblock / ifile backstore? If so I'll respin the
patch with an extra note in the commit message.
Cheers, David
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] target: fix truncated PR-in ReadKeys response
2018-05-31 22:20 [PATCH] target: fix truncated PR-in ReadKeys response David Disseldorp
` (2 preceding siblings ...)
2018-06-04 11:36 ` David Disseldorp
@ 2018-06-04 19:42 ` Maged Mokhtar
2018-06-04 20:20 ` David Disseldorp
4 siblings, 0 replies; 6+ messages in thread
From: Maged Mokhtar @ 2018-06-04 19:42 UTC (permalink / raw)
To: target-devel
On 2018-06-04 13:36, David Disseldorp wrote:
> On Mon, 4 Jun 2018 14:16:45 +0300, Maged Mokhtar wrote:
>
>> This also fixes an issue in Windows server 2016 failover cluster with
>> many client connections, the initial allocation length sent in cdb is
>> 72 bytes which limits it to 8 keys, with additional length not
>> affected
>> by truncation, it will retry with correct size.
>
> Interesting coincidence :) I only ran into this while looking at the
> spec for the enable_pr change. Can you confirm that this fixes Win 2016
> PR behaviour with an iblock / ifile backstore? If so I'll respin the
> patch with an extra note in the commit message.
>
> Cheers, David
> --
Yes it fixes the "Storage Spaces Persistent Reservation" test in the
Windows 2016 Server Failover Cluster validation suites when having many
connections that result in more than 8 registrations. I tested your
patch on 4.17 with iblock.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] target: fix truncated PR-in ReadKeys response
2018-05-31 22:20 [PATCH] target: fix truncated PR-in ReadKeys response David Disseldorp
` (3 preceding siblings ...)
2018-06-04 19:42 ` Maged Mokhtar
@ 2018-06-04 20:20 ` David Disseldorp
4 siblings, 0 replies; 6+ messages in thread
From: David Disseldorp @ 2018-06-04 20:20 UTC (permalink / raw)
To: target-devel
On Mon, 04 Jun 2018 21:42:36 +0200, Maged Mokhtar wrote:
> Yes it fixes the "Storage Spaces Persistent Reservation" test in the
> Windows 2016 Server Failover Cluster validation suites when having many
> connections that result in more than 8 registrations. I tested your
> patch on 4.17 with iblock.
Thanks a lot for confirming this, Maged. Patch with new commit message
to follow...
Cheers, David
^ permalink raw reply [flat|nested] 6+ messages in thread