All of lore.kernel.org
 help / color / mirror / Atom feed
* RE: [PATCH] dm verity: correcting logic used with corrupted_errs counter
@ 2020-12-02 18:05 ` Ravi Kumar Siddojigari
  0 siblings, 0 replies; 4+ messages in thread
From: Ravi Kumar Siddojigari @ 2020-12-02 18:05 UTC (permalink / raw)
  To: linux-block, dm-devel

Sorry,  Resending the patch for comments with  dm-devel added .

-----Original Message-----
From: Ravi Kumar Siddojigari <rsiddoji@codeaurora.org> 
Sent: Friday, November 20, 2020 6:37 PM
To: 'linux-block@vger.kernel.org' <linux-block@vger.kernel.org>
Cc: 'dm-devel@redhat.com' <dm-devel@redhat.com>
Subject: RE: [PATCH] dm verity: correcting logic used with corrupted_errs
counter

One more question  :
	Current code has DM_VERITY_MAX_CORRUPTED_ERRS  set to 100  can we
reduce this ? or is there any  data that made us to keep this 100 ?
Regards,
Ravi

-----Original Message-----
From: Ravi Kumar Siddojigari <rsiddoji@codeaurora.org> 
Sent: Wednesday, November 18, 2020 6:17 PM
To: 'linux-block@vger.kernel.org' <linux-block@vger.kernel.org>
Subject: [PATCH] dm verity: correcting logic used with corrupted_errs
counter

In verity_handle_err we see that the "corrupted_errs"  is never going to be
more than one as the code will fall through "out" label and hit
panic/kernel_restart on the first error  which is not as expected.. 
Following patch will make sure that corrupted_errs are incremented and only
panic/kernel_restart once it reached DM_VERITY_MAX_CORRUPTED_ERRS.

Signed-off-by: Ravi Kumar Siddojigari <rsiddoji@codeaurora.org>
---
 drivers/md/dm-verity-target.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index f74982dcbea0..d86900a2a8d7 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -221,8 +221,10 @@ static int verity_handle_err(struct dm_verity *v, enum
verity_block_type type,
 	/* Corruption should be visible in device status in all modes */
 	v->hash_failed = 1;
 
-	if (v->corrupted_errs >= DM_VERITY_MAX_CORRUPTED_ERRS)
+	if (v->corrupted_errs >= DM_VERITY_MAX_CORRUPTED_ERRS) {
+		DMERR("%s: reached maximum errors", v->data_dev->name);
 		goto out;
+	}
 
 	v->corrupted_errs++;
 
@@ -240,13 +242,13 @@ static int verity_handle_err(struct dm_verity *v, enum
verity_block_type type,
 	DMERR_LIMIT("%s: %s block %llu is corrupted", v->data_dev->name,
 		    type_str, block);
 
-	if (v->corrupted_errs == DM_VERITY_MAX_CORRUPTED_ERRS)
-		DMERR("%s: reached maximum errors", v->data_dev->name);
 
 	snprintf(verity_env, DM_VERITY_ENV_LENGTH, "%s=%d,%llu",
 		DM_VERITY_ENV_VAR_NAME, type, block);
 
 	kobject_uevent_env(&disk_to_dev(dm_disk(md))->kobj, KOBJ_CHANGE,
envp);
+	/* DM_VERITY_MAX_CORRUPTED_ERRS limit not reached yet */
+		return 0;
 
 out:
 	if (v->mode == DM_VERITY_MODE_LOGGING)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a
Linux Foundation Collaborative Project



^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [dm-devel] [PATCH] dm verity: correcting logic used with corrupted_errs counter
@ 2020-12-02 18:05 ` Ravi Kumar Siddojigari
  0 siblings, 0 replies; 4+ messages in thread
From: Ravi Kumar Siddojigari @ 2020-12-02 18:05 UTC (permalink / raw)
  To: linux-block, dm-devel

Sorry,  Resending the patch for comments with  dm-devel added .

-----Original Message-----
From: Ravi Kumar Siddojigari <rsiddoji@codeaurora.org> 
Sent: Friday, November 20, 2020 6:37 PM
To: 'linux-block@vger.kernel.org' <linux-block@vger.kernel.org>
Cc: 'dm-devel@redhat.com' <dm-devel@redhat.com>
Subject: RE: [PATCH] dm verity: correcting logic used with corrupted_errs
counter

One more question  :
	Current code has DM_VERITY_MAX_CORRUPTED_ERRS  set to 100  can we
reduce this ? or is there any  data that made us to keep this 100 ?
Regards,
Ravi

-----Original Message-----
From: Ravi Kumar Siddojigari <rsiddoji@codeaurora.org> 
Sent: Wednesday, November 18, 2020 6:17 PM
To: 'linux-block@vger.kernel.org' <linux-block@vger.kernel.org>
Subject: [PATCH] dm verity: correcting logic used with corrupted_errs
counter

In verity_handle_err we see that the "corrupted_errs"  is never going to be
more than one as the code will fall through "out" label and hit
panic/kernel_restart on the first error  which is not as expected.. 
Following patch will make sure that corrupted_errs are incremented and only
panic/kernel_restart once it reached DM_VERITY_MAX_CORRUPTED_ERRS.

Signed-off-by: Ravi Kumar Siddojigari <rsiddoji@codeaurora.org>
---
 drivers/md/dm-verity-target.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index f74982dcbea0..d86900a2a8d7 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -221,8 +221,10 @@ static int verity_handle_err(struct dm_verity *v, enum
verity_block_type type,
 	/* Corruption should be visible in device status in all modes */
 	v->hash_failed = 1;
 
-	if (v->corrupted_errs >= DM_VERITY_MAX_CORRUPTED_ERRS)
+	if (v->corrupted_errs >= DM_VERITY_MAX_CORRUPTED_ERRS) {
+		DMERR("%s: reached maximum errors", v->data_dev->name);
 		goto out;
+	}
 
 	v->corrupted_errs++;
 
@@ -240,13 +242,13 @@ static int verity_handle_err(struct dm_verity *v, enum
verity_block_type type,
 	DMERR_LIMIT("%s: %s block %llu is corrupted", v->data_dev->name,
 		    type_str, block);
 
-	if (v->corrupted_errs == DM_VERITY_MAX_CORRUPTED_ERRS)
-		DMERR("%s: reached maximum errors", v->data_dev->name);
 
 	snprintf(verity_env, DM_VERITY_ENV_LENGTH, "%s=%d,%llu",
 		DM_VERITY_ENV_VAR_NAME, type, block);
 
 	kobject_uevent_env(&disk_to_dev(dm_disk(md))->kobj, KOBJ_CHANGE,
envp);
+	/* DM_VERITY_MAX_CORRUPTED_ERRS limit not reached yet */
+		return 0;
 
 out:
 	if (v->mode == DM_VERITY_MODE_LOGGING)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a
Linux Foundation Collaborative Project


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [dm-devel] [PATCH] dm verity: correcting logic used with corrupted_errs counter
  2020-12-02 18:05 ` [dm-devel] " Ravi Kumar Siddojigari
@ 2020-12-03 22:13   ` Sami Tolvanen
  -1 siblings, 0 replies; 4+ messages in thread
From: Sami Tolvanen @ 2020-12-03 22:13 UTC (permalink / raw)
  To: Ravi Kumar Siddojigari; +Cc: linux-block, device-mapper development

On Thu, Dec 3, 2020 at 8:18 AM Ravi Kumar Siddojigari
<rsiddoji@codeaurora.org> wrote:
>
> Sorry,  Resending the patch for comments with  dm-devel added .
>
> -----Original Message-----
> From: Ravi Kumar Siddojigari <rsiddoji@codeaurora.org>
> Sent: Friday, November 20, 2020 6:37 PM
> To: 'linux-block@vger.kernel.org' <linux-block@vger.kernel.org>
> Cc: 'dm-devel@redhat.com' <dm-devel@redhat.com>
> Subject: RE: [PATCH] dm verity: correcting logic used with corrupted_errs
> counter
>
> One more question  :
>         Current code has DM_VERITY_MAX_CORRUPTED_ERRS  set to 100  can we
> reduce this ? or is there any  data that made us to keep this 100 ?
> Regards,
> Ravi
>
> -----Original Message-----
> From: Ravi Kumar Siddojigari <rsiddoji@codeaurora.org>
> Sent: Wednesday, November 18, 2020 6:17 PM
> To: 'linux-block@vger.kernel.org' <linux-block@vger.kernel.org>
> Subject: [PATCH] dm verity: correcting logic used with corrupted_errs
> counter
>
> In verity_handle_err we see that the "corrupted_errs"  is never going to be
> more than one as the code will fall through "out" label and hit
> panic/kernel_restart on the first error  which is not as expected..
> Following patch will make sure that corrupted_errs are incremented and only
> panic/kernel_restart once it reached DM_VERITY_MAX_CORRUPTED_ERRS.
>
> Signed-off-by: Ravi Kumar Siddojigari <rsiddoji@codeaurora.org>
> ---
>  drivers/md/dm-verity-target.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
> index f74982dcbea0..d86900a2a8d7 100644
> --- a/drivers/md/dm-verity-target.c
> +++ b/drivers/md/dm-verity-target.c
> @@ -221,8 +221,10 @@ static int verity_handle_err(struct dm_verity *v, enum
> verity_block_type type,
>         /* Corruption should be visible in device status in all modes */
>         v->hash_failed = 1;
>
> -       if (v->corrupted_errs >= DM_VERITY_MAX_CORRUPTED_ERRS)
> +       if (v->corrupted_errs >= DM_VERITY_MAX_CORRUPTED_ERRS) {
> +               DMERR("%s: reached maximum errors", v->data_dev->name);
>                 goto out;
> +       }
>
>         v->corrupted_errs++;
>
> @@ -240,13 +242,13 @@ static int verity_handle_err(struct dm_verity *v, enum
> verity_block_type type,
>         DMERR_LIMIT("%s: %s block %llu is corrupted", v->data_dev->name,
>                     type_str, block);
>
> -       if (v->corrupted_errs == DM_VERITY_MAX_CORRUPTED_ERRS)
> -               DMERR("%s: reached maximum errors", v->data_dev->name);
>
>         snprintf(verity_env, DM_VERITY_ENV_LENGTH, "%s=%d,%llu",
>                 DM_VERITY_ENV_VAR_NAME, type, block);
>
>         kobject_uevent_env(&disk_to_dev(dm_disk(md))->kobj, KOBJ_CHANGE,
> envp);
> +       /* DM_VERITY_MAX_CORRUPTED_ERRS limit not reached yet */
> +               return 0;

No. This would allow invalid blocks to be returned to userspace when
dm-verity is NOT in logging mode, which is unacceptable.
DM_VERITY_MAX_CORRUPTED_ERRS is only used to limit the number of error
messages printed out, we cannot let the first N corrupt blocks to just
slip through.

Sami

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [dm-devel] [PATCH] dm verity: correcting logic used with corrupted_errs counter
@ 2020-12-03 22:13   ` Sami Tolvanen
  0 siblings, 0 replies; 4+ messages in thread
From: Sami Tolvanen @ 2020-12-03 22:13 UTC (permalink / raw)
  To: Ravi Kumar Siddojigari; +Cc: linux-block, device-mapper development

On Thu, Dec 3, 2020 at 8:18 AM Ravi Kumar Siddojigari
<rsiddoji@codeaurora.org> wrote:
>
> Sorry,  Resending the patch for comments with  dm-devel added .
>
> -----Original Message-----
> From: Ravi Kumar Siddojigari <rsiddoji@codeaurora.org>
> Sent: Friday, November 20, 2020 6:37 PM
> To: 'linux-block@vger.kernel.org' <linux-block@vger.kernel.org>
> Cc: 'dm-devel@redhat.com' <dm-devel@redhat.com>
> Subject: RE: [PATCH] dm verity: correcting logic used with corrupted_errs
> counter
>
> One more question  :
>         Current code has DM_VERITY_MAX_CORRUPTED_ERRS  set to 100  can we
> reduce this ? or is there any  data that made us to keep this 100 ?
> Regards,
> Ravi
>
> -----Original Message-----
> From: Ravi Kumar Siddojigari <rsiddoji@codeaurora.org>
> Sent: Wednesday, November 18, 2020 6:17 PM
> To: 'linux-block@vger.kernel.org' <linux-block@vger.kernel.org>
> Subject: [PATCH] dm verity: correcting logic used with corrupted_errs
> counter
>
> In verity_handle_err we see that the "corrupted_errs"  is never going to be
> more than one as the code will fall through "out" label and hit
> panic/kernel_restart on the first error  which is not as expected..
> Following patch will make sure that corrupted_errs are incremented and only
> panic/kernel_restart once it reached DM_VERITY_MAX_CORRUPTED_ERRS.
>
> Signed-off-by: Ravi Kumar Siddojigari <rsiddoji@codeaurora.org>
> ---
>  drivers/md/dm-verity-target.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
> index f74982dcbea0..d86900a2a8d7 100644
> --- a/drivers/md/dm-verity-target.c
> +++ b/drivers/md/dm-verity-target.c
> @@ -221,8 +221,10 @@ static int verity_handle_err(struct dm_verity *v, enum
> verity_block_type type,
>         /* Corruption should be visible in device status in all modes */
>         v->hash_failed = 1;
>
> -       if (v->corrupted_errs >= DM_VERITY_MAX_CORRUPTED_ERRS)
> +       if (v->corrupted_errs >= DM_VERITY_MAX_CORRUPTED_ERRS) {
> +               DMERR("%s: reached maximum errors", v->data_dev->name);
>                 goto out;
> +       }
>
>         v->corrupted_errs++;
>
> @@ -240,13 +242,13 @@ static int verity_handle_err(struct dm_verity *v, enum
> verity_block_type type,
>         DMERR_LIMIT("%s: %s block %llu is corrupted", v->data_dev->name,
>                     type_str, block);
>
> -       if (v->corrupted_errs == DM_VERITY_MAX_CORRUPTED_ERRS)
> -               DMERR("%s: reached maximum errors", v->data_dev->name);
>
>         snprintf(verity_env, DM_VERITY_ENV_LENGTH, "%s=%d,%llu",
>                 DM_VERITY_ENV_VAR_NAME, type, block);
>
>         kobject_uevent_env(&disk_to_dev(dm_disk(md))->kobj, KOBJ_CHANGE,
> envp);
> +       /* DM_VERITY_MAX_CORRUPTED_ERRS limit not reached yet */
> +               return 0;

No. This would allow invalid blocks to be returned to userspace when
dm-verity is NOT in logging mode, which is unacceptable.
DM_VERITY_MAX_CORRUPTED_ERRS is only used to limit the number of error
messages printed out, we cannot let the first N corrupt blocks to just
slip through.

Sami

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel


^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-12-03 22:14 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-02 18:05 [PATCH] dm verity: correcting logic used with corrupted_errs counter Ravi Kumar Siddojigari
2020-12-02 18:05 ` [dm-devel] " Ravi Kumar Siddojigari
2020-12-03 22:13 ` Sami Tolvanen
2020-12-03 22:13   ` Sami Tolvanen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.