[PATCHv3] nvme: module parameter to disable pi with offsets

[PATCHv3] nvme: module parameter to disable pi with offsets

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

A recent commit enables integrity checks for formats the previous kernel
versions registered with the "nop" integrity profile. This means
namespaces using that format become unreadable when upgrading the kernel
past that commit.

Introduce a module parameter to restore the "nop" integrity profile so
that storage can be readable once again. This could be a boot device, so
the setting needs to happen at module load time.

Fixes: 921e81db524d17 ("nvme: allow integrity when PI is not in first bytes")
Reported-by: David Wei <dw@davidwei.uk>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
v2->v3:

  Updated name and added a comment to explain why the parameter exists
 
 drivers/nvme/host/core.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 84cb859a911d0..4ae878b86f2ba 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -91,6 +91,17 @@ module_param(apst_secondary_latency_tol_us, ulong, 0644);
 MODULE_PARM_DESC(apst_secondary_latency_tol_us,
 	"secondary APST latency tolerance in us");
 
+/*
+ * Older kernels didn't enable protection information if it was at an offset.
+ * Newer kernels do, so it breaks reads on the upgrade if such formats were
+ * used in prior kernels since the metadata written did not contain a valid
+ * checksum.
+ */
+static bool disable_pi_offsets = false;
+module_param(disable_pi_offsets, bool, 0444);
+MODULE_PARM_DESC(disable_pi_offsets,
+	"disable protection information if it has an offset");
+
 /*
  * nvme_wq - hosts nvme related works that are not reset or delete
  * nvme_reset_wq - hosts nvme reset works
@@ -1915,6 +1926,8 @@ static void nvme_configure_metadata(struct nvme_ctrl *ctrl,
 		head->pi_type = id->dps & NVME_NS_DPS_PI_MASK;
 	if (!(id->dps & NVME_NS_DPS_PI_FIRST))
 		info->pi_offset = head->ms - head->pi_size;
+	if (info->pi_offset && disable_pi)
+		head->pi_type = 0;
 
 	if (ctrl->ops->flags & NVME_F_FABRICS) {
 		/*
-- 
2.43.5

Re: [PATCHv3] nvme: module parameter to disable pi with offsets

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCHv3] nvme: module parameter to disable pi with offsets

[Keith Busch]

On Thu, Oct 24, 2024 at 08:49:23AM -0700, Keith Busch wrote:
> +	if (info->pi_offset && disable_pi)
> +		head->pi_type = 0;

Added to nvme-6.12, with the typo fix: should be "disable_pi_offsets"

Re: [PATCHv3] nvme: module parameter to disable pi with offsets

[Kanchan Joshi]

On 10/24/2024 10:14 PM, Keith Busch wrote:
> On Thu, Oct 24, 2024 at 08:49:23AM -0700, Keith Busch wrote:
>> +	if (info->pi_offset && disable_pi)
>> +		head->pi_type = 0;
> Added to nvme-6.12, with the typo fix: should be "disable_pi_offsets"

With that change

Reviewed-by: Kanchan Joshi <joshi.k@samsung.com>

Re: [PATCHv3] nvme: module parameter to disable pi with offsets

[Chaitanya Kulkarni]

On 10/24/24 09:44, Keith Busch wrote:
> On Thu, Oct 24, 2024 at 08:49:23AM -0700, Keith Busch wrote:
>> +	if (info->pi_offset && disable_pi)
>> +		head->pi_type = 0;
> Added to nvme-6.12, with the typo fix: should be "disable_pi_offsets"

Indeed.


Looks good. Please add my tag :-

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCHv3] nvme: module parameter to disable pi with offsets

[Tokunori Ikegami]

On 2024/10/25 0:49, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
>
> +module_param(disable_pi_offsets, bool, 0444);

About the naming "disable" is negative meaning so seems better to be a 
positive parameter like as "use_pi_offsets", etc. for example. By the 
way the permission 0444 is only for reads so is it needed to change the 
value by any code change, etc.?

Re: [PATCHv3] nvme: module parameter to disable pi with offsets

[Keith Busch]

On Thu, Oct 24, 2024 at 08:49:23AM -0700, Keith Busch wrote:
> +/*
> + * Older kernels didn't enable protection information if it was at an offset.
> + * Newer kernels do, so it breaks reads on the upgrade if such formats were
> + * used in prior kernels since the metadata written did not contain a valid
> + * checksum.
> + */
> +static bool disable_pi_offsets = false;
> +module_param(disable_pi_offsets, bool, 0444);
> +MODULE_PARM_DESC(disable_pi_offsets,
> +	"disable protection information if it has an offset");
> +
>  /*
>   * nvme_wq - hosts nvme related works that are not reset or delete
>   * nvme_reset_wq - hosts nvme reset works
> @@ -1915,6 +1926,8 @@ static void nvme_configure_metadata(struct nvme_ctrl *ctrl,
>  		head->pi_type = id->dps & NVME_NS_DPS_PI_MASK;
>  	if (!(id->dps & NVME_NS_DPS_PI_FIRST))
>  		info->pi_offset = head->ms - head->pi_size;
> +	if (info->pi_offset && disable_pi)
> +		head->pi_type = 0;

I need to rework this yet again. The old code didn't look for offsets.
It looked for the "first" flag. If you have MS == PI_SIZE, "first" and
"last" mean the exact same thing: there is no offset. But the old code
didn't care, it just wanted to see the PI_FIRST flag.

The part needs to look like this:

---
@@ -1937,10 +1937,12 @@ static void nvme_configure_metadata(struct nvme_ctrl *ctrl,

        if (head->pi_size && head->ms >= head->pi_size)
                head->pi_type = id->dps & NVME_NS_DPS_PI_MASK;
-       if (!(id->dps & NVME_NS_DPS_PI_FIRST))
-               info->pi_offset = head->ms - head->pi_size;
+       if (!(id->dps & NVME_NS_DPS_PI_FIRST)) {
+               if (disable_pi_offsets)
+                       head->pi_type = 0;
+               else
+                       info->pi_offset = head->ms - head->pi_size;
+       }
--