| Message ID | 20250226112035.2571-2-anuj20.g@samsung.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix integrity sysfs reporting inconsistencies | expand |
On Wed, Feb 26, 2025 at 04:50:34PM +0530, Anuj Gupta wrote: > diff --git a/block/blk-settings.c b/block/blk-settings.c > index c44dadc35e1e..8bd0d0f1479c 100644 > --- a/block/blk-settings.c > +++ b/block/blk-settings.c > @@ -861,7 +861,8 @@ bool queue_limits_stack_integrity(struct queue_limits *t, > > if (!ti->tuple_size) { > /* inherit the settings from the first underlying device */ > - if (!(ti->flags & BLK_INTEGRITY_STACKED)) { > + if (!(ti->flags & BLK_INTEGRITY_STACKED) && > + (bi->flags & BLK_INTEGRITY_DEVICE_CAPABLE)) { > ti->flags = BLK_INTEGRITY_DEVICE_CAPABLE | > (bi->flags & BLK_INTEGRITY_REF_TAG); > ti->csum_type = bi->csum_type; As mentioned last round this still does the wrong thing if the first device(s) is/are not PI-capable but the next one(s) is/are. Please look into the pseudocode I posted in reply to the previous iteration on how to fix it.
On Mon, Mar 03, 2025 at 03:12:36PM +0100, Christoph Hellwig wrote: > On Wed, Feb 26, 2025 at 04:50:34PM +0530, Anuj Gupta wrote: > > diff --git a/block/blk-settings.c b/block/blk-settings.c > > index c44dadc35e1e..8bd0d0f1479c 100644 > > --- a/block/blk-settings.c > > +++ b/block/blk-settings.c > > @@ -861,7 +861,8 @@ bool queue_limits_stack_integrity(struct queue_limits *t, > > > > if (!ti->tuple_size) { > > /* inherit the settings from the first underlying device */ > > - if (!(ti->flags & BLK_INTEGRITY_STACKED)) { > > + if (!(ti->flags & BLK_INTEGRITY_STACKED) && > > + (bi->flags & BLK_INTEGRITY_DEVICE_CAPABLE)) { > > ti->flags = BLK_INTEGRITY_DEVICE_CAPABLE | > > (bi->flags & BLK_INTEGRITY_REF_TAG); > > ti->csum_type = bi->csum_type; > > As mentioned last round this still does the wrong thing if the first > device(s) is/are not PI-capable but the next one(s) is/are. Please > look into the pseudocode I posted in reply to the previous iteration > on how to fix it. Christoph, Right, based on your comment modified this patch. Does this look ok? diff --git a/block/blk-settings.c b/block/blk-settings.c index c44dadc35e1e..d0469a812734 100644 --- a/block/blk-settings.c +++ b/block/blk-settings.c @@ -859,36 +859,28 @@ bool queue_limits_stack_integrity(struct queue_limits *t, if (!IS_ENABLED(CONFIG_BLK_DEV_INTEGRITY)) return true; - if (!ti->tuple_size) { - /* inherit the settings from the first underlying device */ - if (!(ti->flags & BLK_INTEGRITY_STACKED)) { - ti->flags = BLK_INTEGRITY_DEVICE_CAPABLE | - (bi->flags & BLK_INTEGRITY_REF_TAG); - ti->csum_type = bi->csum_type; - ti->tuple_size = bi->tuple_size; - ti->pi_offset = bi->pi_offset; - ti->interval_exp = bi->interval_exp; - ti->tag_size = bi->tag_size; - goto done; - } - if (!bi->tuple_size) - goto done; + if (ti->flags & BLK_INTEGRITY_STACKED) { + if (ti->tuple_size != bi->tuple_size) + goto incompatible; + if (ti->interval_exp != bi->interval_exp) + goto incompatible; + if (ti->tag_size != bi->tag_size) + goto incompatible; + if (ti->csum_type != bi->csum_type) + goto incompatible; + if ((ti->flags & BLK_INTEGRITY_REF_TAG) != + (bi->flags & BLK_INTEGRITY_REF_TAG)) + goto incompatible; + } else { + ti->flags = BLK_INTEGRITY_STACKED; + ti->flags |= (bi->flags & BLK_INTEGRITY_DEVICE_CAPABLE) | + (bi->flags & BLK_INTEGRITY_REF_TAG); + ti->csum_type = bi->csum_type; + ti->tuple_size = bi->tuple_size; + ti->pi_offset = bi->pi_offset; + ti->interval_exp = bi->interval_exp; + ti->tag_size = bi->tag_size; } - - if (ti->tuple_size != bi->tuple_size) - goto incompatible; - if (ti->interval_exp != bi->interval_exp) - goto incompatible; - if (ti->tag_size != bi->tag_size) - goto incompatible; - if (ti->csum_type != bi->csum_type) - goto incompatible; - if ((ti->flags & BLK_INTEGRITY_REF_TAG) != - (bi->flags & BLK_INTEGRITY_REF_TAG)) - goto incompatible; - -done: - ti->flags |= BLK_INTEGRITY_STACKED; return true; incompatible:
On Tue, Mar 04, 2025 at 02:18:33PM +0530, Anuj Gupta wrote: > Christoph, > Right, based on your comment modified this patch. > Does this look ok? Yes, this looks good.
diff --git a/block/blk-settings.c b/block/blk-settings.c index c44dadc35e1e..8bd0d0f1479c 100644 --- a/block/blk-settings.c +++ b/block/blk-settings.c @@ -861,7 +861,8 @@ bool queue_limits_stack_integrity(struct queue_limits *t, if (!ti->tuple_size) { /* inherit the settings from the first underlying device */ - if (!(ti->flags & BLK_INTEGRITY_STACKED)) { + if (!(ti->flags & BLK_INTEGRITY_STACKED) && + (bi->flags & BLK_INTEGRITY_DEVICE_CAPABLE)) { ti->flags = BLK_INTEGRITY_DEVICE_CAPABLE | (bi->flags & BLK_INTEGRITY_REF_TAG); ti->csum_type = bi->csum_type;
queue_limits_stack_integrity() incorrectly sets BLK_INTEGRITY_DEVICE_CAPABLE for a DM device even when none of its underlying devices support integrity. This happens because the flag is inherited from the first base device, even if it lacks integrity support. This patch ensures that BLK_INTEGRITY_DEVICE_CAPABLE is only inherited if the first device actually supports integrity. Reported-by: M Nikhil <nikhilm@linux.ibm.com> Link: https://lore.kernel.org/linux-block/f6130475-3ccd-45d2-abde-3ccceada0f0a@linux.ibm.com/ Fixes: c6e56cf6b2e7 ("block: move integrity information into queue_limits") Signed-off-by: Anuj Gupta <anuj20.g@samsung.com> --- block/blk-settings.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)