| Message ID | 20241016112912.63542-12-anuj20.g@samsung.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,01/11] block: define set of integrity flags to be inherited by cloned bip | expand |
On Wed, Oct 16, 2024 at 04:59:12PM +0530, Anuj Gupta wrote: > Add support for sending user-meta buffer. Set tags to be checked > using flags specified by user/block-layer user and underlying DIF/DIX > configuration. The patch itself looks good: Reviewed-by: Christoph Hellwig <hch@lst.de> again this should move to before the user is added. But we still seem to lack an interface to tell the userspace application what flags are actually supported. Just failing the I/O down in the sd driver still feels suboptimal.
> +/* > + * Can't check reftag alone or apptag alone > + */ > +static bool sd_prot_flags_valid(struct scsi_cmnd *scmd) > +{ > + struct request *rq = scsi_cmd_to_rq(scmd); > + struct bio *bio = rq->bio; > + > + if (bio_integrity_flagged(bio, BIP_CHECK_REFTAG) && > + !bio_integrity_flagged(bio, BIP_CHECK_APPTAG)) > + return false; > + if (!bio_integrity_flagged(bio, BIP_CHECK_REFTAG) && > + bio_integrity_flagged(bio, BIP_CHECK_APPTAG)) > + return false; > + return true; > +} > + Martin, Christoph, and all, This snippet prevents a scenario where a apptag check is specified without a reftag check and vice-versa, which is not possible for scsi[1]. But for block layer generated integrity apptag check (BIP_CHECK_APPTAG) is not specified. When scsi drive is formatted with type1/2 PI, block layer would specify refcheck but not appcheck. Hence, these I/O's would fail. Do you see how we can handle this? [1] https://lore.kernel.org/linux-block/yq1ttgz5l6d.fsf@ca-mkp.ca.oracle.com/
On Thu, Oct 17, 2024 at 05:09:23PM +0530, Anuj Gupta wrote: > This snippet prevents a scenario where a apptag check is specified without > a reftag check and vice-versa, which is not possible for scsi[1]. > But for > block layer generated integrity apptag check (BIP_CHECK_APPTAG) is not > specified. When scsi drive is formatted with type1/2 PI, block layer would > specify refcheck but not appcheck. Hence, these I/O's would fail. Do you > see how we can handle this? Well, this is also related to difference in capability checking. Just curious, do you have any user of the more fine grained checking in NVMe? If not we could support the SCSI semantics only and emulate them using the fine grained NVMe semantics and have no portability problems.
On Thu, Oct 17, 2024 at 04:39:18PM +0200, Christoph Hellwig wrote: > On Thu, Oct 17, 2024 at 05:09:23PM +0530, Anuj Gupta wrote: > > This snippet prevents a scenario where a apptag check is specified without > > a reftag check and vice-versa, which is not possible for scsi[1]. > > But for > > block layer generated integrity apptag check (BIP_CHECK_APPTAG) is not > > specified. When scsi drive is formatted with type1/2 PI, block layer would > > specify refcheck but not appcheck. Hence, these I/O's would fail. Do you > > see how we can handle this? > > Well, this is also related to difference in capability checking. Right. > Just curious, do you have any user of the more fine grained checking > in NVMe? If not we could support the SCSI semantics only and emulate > them using the fine grained NVMe semantics and have no portability > problems. We can choose to support scsi semantics only and expose only the valid scsi combinations to userspace i.e. 1. no check 2. guard check only 3. ref + app check only 4. guard + ref + app check Something like this [*] on top of this series, untested though. Does this align with what you have in mind? [*] diff --git a/block/bio-integrity.c b/block/bio-integrity.c index 24fad9b6f3ec..2ca27910770b 100644 --- a/block/bio-integrity.c +++ b/block/bio-integrity.c @@ -308,12 +308,10 @@ static void bio_uio_meta_to_bip(struct bio *bio, struct uio_meta *meta) { struct bio_integrity_payload *bip = bio_integrity(bio); - if (meta->flags & BLK_INTEGRITY_CHK_GUARD) + if (meta->flags & IO_INTEGRITY_CHK_GUARD) bip->bip_flags |= BIP_CHECK_GUARD; - if (meta->flags & BLK_INTEGRITY_CHK_APPTAG) - bip->bip_flags |= BIP_CHECK_APPTAG; - if (meta->flags & BLK_INTEGRITY_CHK_REFTAG) - bip->bip_flags |= BIP_CHECK_REFTAG; + if (meta->flags & IO_INTEGRITY_CHK_REF_APP) + bip->bip_flags |= BIP_CHECK_REFTAG | BIP_CHECK_APPTAG; bip->app_tag = meta->app_tag; } @@ -329,9 +327,9 @@ int bio_integrity_map_iter(struct bio *bio, struct uio_meta *meta) return -EINVAL; /* should fit into two bytes */ - BUILD_BUG_ON(BLK_INTEGRITY_VALID_FLAGS >= (1 << 16)); + BUILD_BUG_ON(IO_INTEGRITY_VALID_FLAGS >= (1 << 16)); - if (meta->flags && (meta->flags & ~BLK_INTEGRITY_VALID_FLAGS)) + if (meta->flags && (meta->flags & ~IO_INTEGRITY_VALID_FLAGS)) return -EINVAL; /* diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h index 753971770733..714700f9826e 100644 --- a/include/uapi/linux/fs.h +++ b/include/uapi/linux/fs.h @@ -40,6 +40,15 @@ #define BLOCK_SIZE_BITS 10 #define BLOCK_SIZE (1<<BLOCK_SIZE_BITS) +/* + * flags for integrity meta + */ +#define IO_INTEGRITY_CHK_GUARD (1U << 0) /* enforce guard check */ +#define IO_INTEGRITY_CHK_REF_APP (1U << 1) /* enforce ref and app check */ + +#define IO_INTEGRITY_VALID_FLAGS (IO_INTEGRITY_CHK_GUARD | \ + IO_INTEGRITY_CHK_REF_APP) + #define SEEK_SET 0 /* seek relative to beginning of file */ #define SEEK_CUR 1 /* seek relative to current file position */ #define SEEK_END 2 /* seek relative to end of file */
On Fri, Oct 18, 2024 at 01:56:48PM +0530, Anuj Gupta wrote: > > Just curious, do you have any user of the more fine grained checking > > in NVMe? If not we could support the SCSI semantics only and emulate > > them using the fine grained NVMe semantics and have no portability > > problems. > > We can choose to support scsi semantics only and expose only the valid > scsi combinations to userspace i.e. > > 1. no check > 2. guard check only > 3. ref + app check only > 4. guard + ref + app check > > Something like this [*] on top of this series, untested though. Does > this align with what you have in mind? That is indeed what I had in mind. But I'd really like to hear from Martin on this as well.
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index ca4bc0ac76ad..87ae19c9b29c 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -802,6 +802,23 @@ static unsigned int sd_prot_flag_mask(unsigned int prot_op) return flag_mask[prot_op]; } +/* + * Can't check reftag alone or apptag alone + */ +static bool sd_prot_flags_valid(struct scsi_cmnd *scmd) +{ + struct request *rq = scsi_cmd_to_rq(scmd); + struct bio *bio = rq->bio; + + if (bio_integrity_flagged(bio, BIP_CHECK_REFTAG) && + !bio_integrity_flagged(bio, BIP_CHECK_APPTAG)) + return false; + if (!bio_integrity_flagged(bio, BIP_CHECK_REFTAG) && + bio_integrity_flagged(bio, BIP_CHECK_APPTAG)) + return false; + return true; +} + static unsigned char sd_setup_protect_cmnd(struct scsi_cmnd *scmd, unsigned int dix, unsigned int dif) { @@ -814,14 +831,16 @@ static unsigned char sd_setup_protect_cmnd(struct scsi_cmnd *scmd, if (bio_integrity_flagged(bio, BIP_IP_CHECKSUM)) scmd->prot_flags |= SCSI_PROT_IP_CHECKSUM; - if (bio_integrity_flagged(bio, BIP_CTRL_NOCHECK) == false) + if (bio_integrity_flagged(bio, BIP_CTRL_NOCHECK) == false && + (bio_integrity_flagged(bio, BIP_CHECK_GUARD))) scmd->prot_flags |= SCSI_PROT_GUARD_CHECK; } if (dif != T10_PI_TYPE3_PROTECTION) { /* DIX/DIF Type 0, 1, 2 */ scmd->prot_flags |= SCSI_PROT_REF_INCREMENT; - if (bio_integrity_flagged(bio, BIP_CTRL_NOCHECK) == false) + if ((bio_integrity_flagged(bio, BIP_CTRL_NOCHECK) == false) && + (!dix || bio_integrity_flagged(bio, BIP_CHECK_REFTAG))) scmd->prot_flags |= SCSI_PROT_REF_CHECK; } @@ -1373,6 +1392,8 @@ static blk_status_t sd_setup_read_write_cmnd(struct scsi_cmnd *cmd) dif = scsi_host_dif_capable(cmd->device->host, sdkp->protection_type); dld = sd_cdl_dld(sdkp, cmd); + if (!sd_prot_flags_valid(cmd)) + goto fail; if (dif || dix) protect = sd_setup_protect_cmnd(cmd, dix, dif); else
Add support for sending user-meta buffer. Set tags to be checked using flags specified by user/block-layer user and underlying DIF/DIX configuration. Signed-off-by: Anuj Gupta <anuj20.g@samsung.com> --- drivers/scsi/sd.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-)