| Message ID | 20180531222054.32655-1-ddiss@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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> -- 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
-------------------------------------------------- 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. -- 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
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 -- 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
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. -- 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
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 -- 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
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);
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(-)