| Message ID | 20241016112912.63542-12-anuj20.g@samsung.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Read/Write with meta/integrity | 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.
Anuj, > +/* > + * 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; > +} This breaks reading the partition table. The BIP_CHECK_* flags should really only control DIX in the SCSI case. Filling out *PROTECT is left as an exercise for the SCSI disk driver. It's the only way we can sanely deal with this. Especially given ATO, GRD_CHK, REF_CHK, and APP_CHK. It just gets too complicated. You should just drop sd_prot_flags_valid() and things work fine. And then with BIP_CHECK_* introduced we can drop BIP_CTRL_NOCHECK.
On Mon, Oct 21, 2024 at 09:58:57PM -0400, Martin K. Petersen wrote: > > Anuj, > > > +/* > > + * 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; > > +} > > This breaks reading the partition table. > > The BIP_CHECK_* flags should really only control DIX in the SCSI case. > Filling out *PROTECT is left as an exercise for the SCSI disk driver. > It's the only way we can sanely deal with this. Especially given ATO, > GRD_CHK, REF_CHK, and APP_CHK. It just gets too complicated. > > You should just drop sd_prot_flags_valid() and things work fine. And > then with BIP_CHECK_* introduced we can drop BIP_CTRL_NOCHECK. So I will keep the fine grained userspace/bip flags (which we have in this version). And drop the sd_prot_flags_valid() and BIP_CTRL_NOCHECK like below [1]. Hope that looks fine [1] diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index ca4bc0ac76ad..0913bd43f48a 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -814,14 +814,14 @@ 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_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_CHECK_REFTAG)) scmd->prot_flags |= SCSI_PROT_REF_CHECK; }
Anuj, > So I will keep the fine grained userspace/bip flags (which we have in > this version). And drop the sd_prot_flags_valid() and BIP_CTRL_NOCHECK > like below [1]. Hope that looks fine Yep, that's fine. Thanks!
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(-)