* [patch] New mode DM-Verity error handling [not found] <CGME20200618070250epcas1p409eb2ddd19ecc5d55c219ac3dc884f25@epcas1p4.samsung.com> @ 2020-06-18 6:56 ` JeongHyeon Lee 2020-06-18 15:44 ` Mike Snitzer 0 siblings, 1 reply; 9+ messages in thread From: JeongHyeon Lee @ 2020-06-18 6:56 UTC (permalink / raw) To: snitzer; +Cc: agk, dm-devel, corbet, linux-doc, linux-kernel [-- Attachment #1: Type: text/plain, Size: 809 bytes --] Hello, Dear devcice-mapper maintainers. I'm JeongHyeon Lee, work in Samsung. I'm chage of DM-Verity feature with Mr. sunwook eom. I have a patch or suggestion about DM-Verity error handling. Our device (smart phone) need DM-Verity feature. So I hope there is new mode DM-Verity error handling. This new mode concept is When detect corrupted block, will be go to panic. Because our team policy is found device DM-Verity error, device will go panic. And then analyze what kind of device fault (crash UFS, IO error, DRAM bit flip etc) In addition to the smart phone, I would like to have an option that users or administrators can use accordingly. There are patch contents in the attachment. I would really appreciate it if you could check it. I will look forward to hearing from yours. Thank you :) [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-dm-verity-new-error-handling-mode-for-corrupted-bloc.patch --] [-- Type: text/x-patch; name="0001-dm-verity-new-error-handling-mode-for-corrupted-bloc.patch", Size: 3309 bytes --] From 6d3e508ed6872bfdc88d6ad979ac5c0347144fbb Mon Sep 17 00:00:00 2001 From: "jhs2.lee" <jhs2.lee@samsung.com> Date: Thu, 18 Jun 2020 15:32:20 +0900 Subject: [PATCH] dm verity: new error handling mode for corrupted blocks There is no panic error handling mode when a problem occurs. So We add new error handling mode. users and administrators setup to fit your need. Signed-off-by: jhs2.lee <jhs2.lee@samsung.com> --- Documentation/admin-guide/device-mapper/verity.rst | 4 ++++ drivers/md/dm-verity-target.c | 11 +++++++++++ drivers/md/dm-verity.h | 3 ++- 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/Documentation/admin-guide/device-mapper/verity.rst b/Documentation/admin-guide/device-mapper/verity.rst index bb02caa45289..66f71f0dab1b 100644 --- a/Documentation/admin-guide/device-mapper/verity.rst +++ b/Documentation/admin-guide/device-mapper/verity.rst @@ -83,6 +83,10 @@ restart_on_corruption not compatible with ignore_corruption and requires user space support to avoid restart loops. +panic_on_corruption + Panic the device when a corrupted block is discovered. This option is + not compatible with ignore_corruption and restart_on_corruption. + ignore_zero_blocks Do not verify blocks that are expected to contain zeroes and always return zeroes instead. This may be useful if the partition contains unused blocks diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c index eec9f252e935..c89114e7886c 100644 --- a/drivers/md/dm-verity-target.c +++ b/drivers/md/dm-verity-target.c @@ -30,6 +30,7 @@ #define DM_VERITY_OPT_LOGGING "ignore_corruption" #define DM_VERITY_OPT_RESTART "restart_on_corruption" +#define DM_VERITY_OPT_PANIC "panic_on_corruption" #define DM_VERITY_OPT_IGN_ZEROES "ignore_zero_blocks" #define DM_VERITY_OPT_AT_MOST_ONCE "check_at_most_once" @@ -254,6 +255,9 @@ static int verity_handle_err(struct dm_verity *v, enum verity_block_type type, if (v->mode == DM_VERITY_MODE_RESTART) kernel_restart("dm-verity device corrupted"); + if (v->mode == DM_VERITY_MODE_PANIC) + panic("dm-verity device corrupted"); + return 1; } @@ -742,6 +746,9 @@ static void verity_status(struct dm_target *ti, status_type_t type, case DM_VERITY_MODE_RESTART: DMEMIT(DM_VERITY_OPT_RESTART); break; + case DM_VERITY_MODE_PANIC: + DMEMIT(DM_VERITY_OPT_PANIC); + break; default: BUG(); } @@ -907,6 +914,10 @@ static int verity_parse_opt_args(struct dm_arg_set *as, struct dm_verity *v, v->mode = DM_VERITY_MODE_RESTART; continue; + } else if (!strcasecmp(arg_name, DM_VERITY_OPT_PANIC)) { + v->mode = DM_VERITY_MODE_PANIC; + continue; + } else if (!strcasecmp(arg_name, DM_VERITY_OPT_IGN_ZEROES)) { r = verity_alloc_zero_digest(v); if (r) { diff --git a/drivers/md/dm-verity.h b/drivers/md/dm-verity.h index 641b9e3a399b..4e769d13473a 100644 --- a/drivers/md/dm-verity.h +++ b/drivers/md/dm-verity.h @@ -20,7 +20,8 @@ enum verity_mode { DM_VERITY_MODE_EIO, DM_VERITY_MODE_LOGGING, - DM_VERITY_MODE_RESTART + DM_VERITY_MODE_RESTART, + DM_VERITY_MODE_PANIC }; enum verity_block_type { -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: New mode DM-Verity error handling 2020-06-18 6:56 ` [patch] New mode DM-Verity error handling JeongHyeon Lee @ 2020-06-18 15:44 ` Mike Snitzer 2020-06-18 16:50 ` [dm-devel] " Sami Tolvanen 0 siblings, 1 reply; 9+ messages in thread From: Mike Snitzer @ 2020-06-18 15:44 UTC (permalink / raw) To: JeongHyeon Lee; +Cc: agk, dm-devel, corbet, linux-doc, linux-kernel On Thu, Jun 18 2020 at 2:56am -0400, JeongHyeon Lee <jhs2.lee@samsung.com> wrote: > Hello, Dear devcice-mapper maintainers. > > I'm JeongHyeon Lee, work in Samsung. I'm chage of DM-Verity feature with > Mr. sunwook eom. > I have a patch or suggestion about DM-Verity error handling. > > Our device (smart phone) need DM-Verity feature. So I hope there is new > mode DM-Verity error handling. > This new mode concept is When detect corrupted block, will be go to panic. > > Because our team policy is found device DM-Verity error, device will go > panic. > And then analyze what kind of device fault (crash UFS, IO error, DRAM > bit flip etc) > > In addition to the smart phone, I would like to have an option that > users or administrators can use accordingly. > There are patch contents in the attachment. I would really appreciate it > if you could check it. > > I will look forward to hearing from yours. > Thank you :) > I do not accept that panicing the system because of verity failure is reasonable. In fact, even rebooting (via DM_VERITY_MODE_RESTART) looks very wrong. The device should be put in a failed state and left for admin recovery. Mike ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dm-devel] New mode DM-Verity error handling 2020-06-18 15:44 ` Mike Snitzer @ 2020-06-18 16:50 ` Sami Tolvanen 2020-06-18 17:09 ` Mike Snitzer 0 siblings, 1 reply; 9+ messages in thread From: Sami Tolvanen @ 2020-06-18 16:50 UTC (permalink / raw) To: Mike Snitzer Cc: JeongHyeon Lee, dm-devel, linux-doc, linux-kernel, agk, corbet On Thu, Jun 18, 2020 at 11:44:45AM -0400, Mike Snitzer wrote: > I do not accept that panicing the system because of verity failure is > reasonable. > > In fact, even rebooting (via DM_VERITY_MODE_RESTART) looks very wrong. > > The device should be put in a failed state and left for admin recovery. That's exactly how the restart mode works on some Android devices. The bootloader sees the verification error and puts the device in recovery mode. Using the restart mode on systems without firmware support won't make sense, obviously. Sami ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: New mode DM-Verity error handling 2020-06-18 16:50 ` [dm-devel] " Sami Tolvanen @ 2020-06-18 17:09 ` Mike Snitzer 2020-06-22 0:27 ` JeongHyeon Lee 2020-06-22 7:58 ` Milan Broz 0 siblings, 2 replies; 9+ messages in thread From: Mike Snitzer @ 2020-06-18 17:09 UTC (permalink / raw) To: Sami Tolvanen Cc: JeongHyeon Lee, dm-devel, linux-doc, linux-kernel, agk, corbet On Thu, Jun 18 2020 at 12:50pm -0400, Sami Tolvanen <samitolvanen@google.com> wrote: > On Thu, Jun 18, 2020 at 11:44:45AM -0400, Mike Snitzer wrote: > > I do not accept that panicing the system because of verity failure is > > reasonable. > > > > In fact, even rebooting (via DM_VERITY_MODE_RESTART) looks very wrong. > > > > The device should be put in a failed state and left for admin recovery. > > That's exactly how the restart mode works on some Android devices. The > bootloader sees the verification error and puts the device in recovery > mode. Using the restart mode on systems without firmware support won't > make sense, obviously. OK, so I need further justification from Samsung why they are asking for this panic mode. Thanks, Mike ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: New mode DM-Verity error handling 2020-06-18 17:09 ` Mike Snitzer @ 2020-06-22 0:27 ` JeongHyeon Lee 2020-06-22 7:58 ` Milan Broz 1 sibling, 0 replies; 9+ messages in thread From: JeongHyeon Lee @ 2020-06-22 0:27 UTC (permalink / raw) To: Mike Snitzer, Sami Tolvanen Cc: dm-devel, linux-doc, linux-kernel, agk, corbet Hello Dear DM-Verity maintainers. Thank you for your reply. I agreed with you that "the device should be put in a failed state and left for admin recovery" As dear Sami told us, When Android device occurred panic, restarting and to save the logs to bootloader also recovery log. Of course Using the restart mode on systems without firmware support won't make sense. However, on Android devices, restart or panic mode makes sense. In android, the behavior is different depend on the binary type. here are 3 type like user / userdebug / eng (engineering). When kernel panic occurs, it operates as follows * kernel panic in user binary(low)-> restart mode * kernel panic in eng binary(mid) -> upload mode It's actually at the debug level. All users are set to low, but change it build option or others. but Most users do not know. You might think, "Why do you need a panic instead of reboot?" To start with, It's easy to analyze what the device has problem. If we use restart mode, it's difficult to analyze because device is rebooted without logging.(not remain log) And If use panic mode, samsung takes snapshots(save log etc) when occurred panic.(Maybe other company or Android are same). So We look for a debugging log and the analyze kind of problem in device as well as dm-verity. In the development stage, most of them are use in eng mode. when panic occurs, it goes to upload mode, so it is convenient to analyze whether it is HW problem / SW problem. In most cases it was a hardware issue. Since we are a manufacturer, the HW problem is also important. Also, users using Android devices can recognize that there is a problem with my device through a reboot. Users don't know the exact reason, but they think that rebooting is wrong. As mentioned above, in user mode, panic operates in reboot mode. The user sees that device is rebooting and thinks there is a problem. They uploads QnA to Samsung members App or visit service center for repair. Then, developers need to get the log the device used by users to check what the problem is. So We are using panic to get the log. What's more, reboot/panic may seem wrong, but from a security perspective, I think it's really important when looking at dm-verity. Of course, I think the maintainers already know it. To the important partition or Android devices system, will be protected using dm-verity. We can make the partition(want to protect) into a read-only partition, compare the digest, and check whether there are any problems. If a malicious user or hacker can damage the system or important partition may change something. At this time, we can defend against further hacking by generating a panic or restart. This will make the security feel strong. So reboot mode and panic mode will be required. We have long explained why we need it. Through this, Samsung needs a panic mode, so please read it carefully and give feedback. Thank you :D Jeonghyeon Lee On 19/06/2020 02:09, Mike Snitzer wrote: > On Thu, Jun 18 2020 at 12:50pm -0400, > Sami Tolvanen <samitolvanen@google.com> wrote: > >> On Thu, Jun 18, 2020 at 11:44:45AM -0400, Mike Snitzer wrote: >>> I do not accept that panicing the system because of verity failure is >>> reasonable. >>> >>> In fact, even rebooting (via DM_VERITY_MODE_RESTART) looks very wrong. >>> >>> The device should be put in a failed state and left for admin recovery. >> That's exactly how the restart mode works on some Android devices. The >> bootloader sees the verification error and puts the device in recovery >> mode. Using the restart mode on systems without firmware support won't >> make sense, obviously. > OK, so I need further justification from Samsung why they are asking for > this panic mode. > > Thanks, > Mike > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: New mode DM-Verity error handling 2020-06-18 17:09 ` Mike Snitzer 2020-06-22 0:27 ` JeongHyeon Lee @ 2020-06-22 7:58 ` Milan Broz 2020-06-22 23:53 ` JeongHyeon Lee 1 sibling, 1 reply; 9+ messages in thread From: Milan Broz @ 2020-06-22 7:58 UTC (permalink / raw) To: Mike Snitzer, Sami Tolvanen Cc: JeongHyeon Lee, dm-devel, linux-doc, linux-kernel, agk, corbet On 18/06/2020 19:09, Mike Snitzer wrote: > On Thu, Jun 18 2020 at 12:50pm -0400, > Sami Tolvanen <samitolvanen@google.com> wrote: > >> On Thu, Jun 18, 2020 at 11:44:45AM -0400, Mike Snitzer wrote: >>> I do not accept that panicing the system because of verity failure is >>> reasonable. >>> >>> In fact, even rebooting (via DM_VERITY_MODE_RESTART) looks very wrong. >>> >>> The device should be put in a failed state and left for admin recovery. >> >> That's exactly how the restart mode works on some Android devices. The >> bootloader sees the verification error and puts the device in recovery >> mode. Using the restart mode on systems without firmware support won't >> make sense, obviously. > > OK, so I need further justification from Samsung why they are asking for > this panic mode. I think when we have reboot already, panic is not much better :-) Just please note that dm-verity is used not only in Android world (with own tooling) but in normal Linux distributions, and I need to modify userspace (veritysetup) to support and recognize this flag. Please *always* increase minor dm-verity target version when adding a new feature - we can then provide some better hint if it is not supported. Thanks, Milan ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: New mode DM-Verity error handling 2020-06-22 7:58 ` Milan Broz @ 2020-06-22 23:53 ` JeongHyeon Lee 2020-06-23 7:28 ` Milan Broz 0 siblings, 1 reply; 9+ messages in thread From: JeongHyeon Lee @ 2020-06-22 23:53 UTC (permalink / raw) To: Milan Broz, Mike Snitzer, Sami Tolvanen Cc: dm-devel, linux-doc, linux-kernel, agk, corbet Dear Milan Broz. Thank for your reply. I didn't understand well, could you explain it in more detail? For what reason isn't panic better? Is it because there is a place to use other device-mapper? Or other things? I just wonder. I would like to hear various explanations and information. I just wanted user to use what they wanted through the options(flags). Yes, If adding a new feature, modify user-space to support. Oh, I'm sorry :( If when i suggested new patch, i will send you a patch that increased minor version. Thank you for all your detailed information. Thanks. JeongHyeon Lee On 22/06/2020 16:58, Milan Broz wrote: > On 18/06/2020 19:09, Mike Snitzer wrote: >> On Thu, Jun 18 2020 at 12:50pm -0400, >> Sami Tolvanen <samitolvanen@google.com> wrote: >> >>> On Thu, Jun 18, 2020 at 11:44:45AM -0400, Mike Snitzer wrote: >>>> I do not accept that panicing the system because of verity failure is >>>> reasonable. >>>> >>>> In fact, even rebooting (via DM_VERITY_MODE_RESTART) looks very wrong. >>>> >>>> The device should be put in a failed state and left for admin recovery. >>> That's exactly how the restart mode works on some Android devices. The >>> bootloader sees the verification error and puts the device in recovery >>> mode. Using the restart mode on systems without firmware support won't >>> make sense, obviously. >> OK, so I need further justification from Samsung why they are asking for >> this panic mode. > I think when we have reboot already, panic is not much better :-) > > Just please note that dm-verity is used not only in Android world (with own tooling) > but in normal Linux distributions, and I need to modify userspace (veritysetup) to support > and recognize this flag. > > Please *always* increase minor dm-verity target version when adding a new feature > - we can then provide some better hint if it is not supported. > > Thanks, > Milan > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: New mode DM-Verity error handling 2020-06-22 23:53 ` JeongHyeon Lee @ 2020-06-23 7:28 ` Milan Broz 2020-06-23 8:08 ` JeongHyeon Lee 0 siblings, 1 reply; 9+ messages in thread From: Milan Broz @ 2020-06-23 7:28 UTC (permalink / raw) To: JeongHyeon Lee, Mike Snitzer, Sami Tolvanen Cc: dm-devel, linux-doc, linux-kernel, agk, corbet On 23/06/2020 01:53, JeongHyeon Lee wrote: > > For what reason isn't panic better? I did not say panic is better, I said that while we have restart already in mainline dm-verity code, panic() is almost the same, so I see no problem in merging this patch. Stopping system this way could create more damage if it is not configured properly, but I think it is quite common to stop the system as fast as possible if data system integrity is violated... > If when i suggested new patch, i will send you a patch that increased > minor version. I think Mike can fold-in version increase, if the patch is accepted. But please include these version changes with every new feature. Actually I am tracking it here for dm-verity as part of veritysetup userspace documentation: https://gitlab.com/cryptsetup/cryptsetup/-/wikis/DMVerity Thanks, Milan > On 22/06/2020 16:58, Milan Broz wrote: >> On 18/06/2020 19:09, Mike Snitzer wrote: >>> On Thu, Jun 18 2020 at 12:50pm -0400, >>> Sami Tolvanen <samitolvanen@google.com> wrote: >>> >>>> On Thu, Jun 18, 2020 at 11:44:45AM -0400, Mike Snitzer wrote: >>>>> I do not accept that panicing the system because of verity failure is >>>>> reasonable. >>>>> >>>>> In fact, even rebooting (via DM_VERITY_MODE_RESTART) looks very wrong. >>>>> >>>>> The device should be put in a failed state and left for admin recovery. >>>> That's exactly how the restart mode works on some Android devices. The >>>> bootloader sees the verification error and puts the device in recovery >>>> mode. Using the restart mode on systems without firmware support won't >>>> make sense, obviously. >>> OK, so I need further justification from Samsung why they are asking for >>> this panic mode. >> I think when we have reboot already, panic is not much better :-) >> >> Just please note that dm-verity is used not only in Android world (with own tooling) >> but in normal Linux distributions, and I need to modify userspace (veritysetup) to support >> and recognize this flag. >> >> Please *always* increase minor dm-verity target version when adding a new feature >> - we can then provide some better hint if it is not supported. >> >> Thanks, >> Milan >> >> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: New mode DM-Verity error handling 2020-06-23 7:28 ` Milan Broz @ 2020-06-23 8:08 ` JeongHyeon Lee 0 siblings, 0 replies; 9+ messages in thread From: JeongHyeon Lee @ 2020-06-23 8:08 UTC (permalink / raw) To: Milan Broz, Mike Snitzer, Sami Tolvanen Cc: dm-devel, linux-doc, linux-kernel, agk, corbet Dear Milan Broz. Thank you for answer my query. I asked you again because i was confused. Yes, I also looked at the document and get a lot of information or studies related to dm-verity. https://gitlab.com/cryptsetup/cryptsetup/-/wikis/DMVerity Thank you : D JeongHyeon Lee On 23/06/2020 16:28, Milan Broz wrote: > On 23/06/2020 01:53, JeongHyeon Lee wrote: >> For what reason isn't panic better? > I did not say panic is better, I said that while we have restart already in mainline dm-verity code, > panic() is almost the same, so I see no problem in merging this patch. > > Stopping system this way could create more damage if it is not configured properly, > but I think it is quite common to stop the system as fast as possible if data system integrity > is violated... > >> If when i suggested new patch, i will send you a patch that increased >> minor version. > I think Mike can fold-in version increase, if the patch is accepted. > > But please include these version changes with every new feature. > > Actually I am tracking it here for dm-verity as part of veritysetup userspace documentation: > https://gitlab.com/cryptsetup/cryptsetup/-/wikis/DMVerity > > Thanks, > Milan > >> On 22/06/2020 16:58, Milan Broz wrote: >>> On 18/06/2020 19:09, Mike Snitzer wrote: >>>> On Thu, Jun 18 2020 at 12:50pm -0400, >>>> Sami Tolvanen <samitolvanen@google.com> wrote: >>>> >>>>> On Thu, Jun 18, 2020 at 11:44:45AM -0400, Mike Snitzer wrote: >>>>>> I do not accept that panicing the system because of verity failure is >>>>>> reasonable. >>>>>> >>>>>> In fact, even rebooting (via DM_VERITY_MODE_RESTART) looks very wrong. >>>>>> >>>>>> The device should be put in a failed state and left for admin recovery. >>>>> That's exactly how the restart mode works on some Android devices. The >>>>> bootloader sees the verification error and puts the device in recovery >>>>> mode. Using the restart mode on systems without firmware support won't >>>>> make sense, obviously. >>>> OK, so I need further justification from Samsung why they are asking for >>>> this panic mode. >>> I think when we have reboot already, panic is not much better :-) >>> >>> Just please note that dm-verity is used not only in Android world (with own tooling) >>> but in normal Linux distributions, and I need to modify userspace (veritysetup) to support >>> and recognize this flag. >>> >>> Please *always* increase minor dm-verity target version when adding a new feature >>> - we can then provide some better hint if it is not supported. >>> >>> Thanks, >>> Milan >>> >>> > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-06-23 8:14 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CGME20200618070250epcas1p409eb2ddd19ecc5d55c219ac3dc884f25@epcas1p4.samsung.com> 2020-06-18 6:56 ` [patch] New mode DM-Verity error handling JeongHyeon Lee 2020-06-18 15:44 ` Mike Snitzer 2020-06-18 16:50 ` [dm-devel] " Sami Tolvanen 2020-06-18 17:09 ` Mike Snitzer 2020-06-22 0:27 ` JeongHyeon Lee 2020-06-22 7:58 ` Milan Broz 2020-06-22 23:53 ` JeongHyeon Lee 2020-06-23 7:28 ` Milan Broz 2020-06-23 8:08 ` JeongHyeon Lee
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).