New issue
Advanced search Search tips

Issue 853730 link

Starred by 2 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Feature


Participants' hotlists:
Cros-Hwsec-Ready


Sign in to add a comment

Add basic metrics for mount_encrypted

Project Member Reported by mnissler@chromium.org, Jun 18 2018

Issue description

We should add some basic UMA tracking for mount_encrypted to validate behavior in the field. Ideas for vitals that would make sense to track:

* System key type (TPM 2 NV space, TPM 1 encstateful, TPM 1 lockbox (v1/v2), needs finalization, factory, etc.)
* Encryption key loading status (absent, failure, decryption error, etc.)
* TPM communication errors
* NVRAM space locking status?
* Mount errors
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 26 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/37be31a252ecf2934b18aae9a1e92ce4e6e87dff

commit 37be31a252ecf2934b18aae9a1e92ce4e6e87dff
Author: Mattias Nissler <mnissler@chromium.org>
Date: Tue Jun 26 19:51:57 2018

metrics: Add support for alternate metrics files

Extend MetricsLibrary to specify an alternate file to dump metric
samples to and add a function to replay events from a given file. This
allows use of MetricsLibrary also in contexts where /var/lib/metrics
is not accessible (e.g. before mounting the stateful file system).
Expose the new features via switches in metrics_client.

BUG=chromium:853730
TEST=Existing unit tests.

Change-Id: I1868303de3f5e5f3accc200c64a177051fab5880
Reviewed-on: https://chromium-review.googlesource.com/1104459
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Luigi Semenzato <semenzato@chromium.org>
Reviewed-by: Andrey Pronin <apronin@chromium.org>

[modify] https://crrev.com/37be31a252ecf2934b18aae9a1e92ce4e6e87dff/metrics/metrics_library.cc
[modify] https://crrev.com/37be31a252ecf2934b18aae9a1e92ce4e6e87dff/metrics/serialization/serialization_utils_unittest.cc
[modify] https://crrev.com/37be31a252ecf2934b18aae9a1e92ce4e6e87dff/metrics/metrics_client.cc
[modify] https://crrev.com/37be31a252ecf2934b18aae9a1e92ce4e6e87dff/metrics/serialization/serialization_utils.h
[modify] https://crrev.com/37be31a252ecf2934b18aae9a1e92ce4e6e87dff/metrics/metrics_library.h
[modify] https://crrev.com/37be31a252ecf2934b18aae9a1e92ce4e6e87dff/metrics/serialization/serialization_utils.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Jun 26 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/d9fca6d90366b80a1db74ba382a72a173ae6cc25

commit d9fca6d90366b80a1db74ba382a72a173ae6cc25
Author: Mattias Nissler <mnissler@chromium.org>
Date: Tue Jun 26 19:51:57 2018

metrics: Make MetricSample copyable and movable.

MetricSample is a value object class that contains sample data. The
current design wraps all owned objects in a std::unique_ptr. This not
only adds boilerplate, but also complicates vectors of MetricSamples.
This change makes MetricSample copyable and movable so
std::vector<MetricSample> behaves nicer (for example thanks to the
copy constructor std::initializer_list<MetricSample> now works).

Performance-wise, MetricSample construction and management is not
expected to be hot code. Regardless of that, the change shouldn't make
much of a difference since the std::unique_ptr move operations are
merely replaced by MetricSample move operations. There are a couple
additional values to be moved, but on the positive side creating
MetricSample objects no longer requires mandatory heap allocations
(which are expected to have performance impact via cache effects). A
highly unscientific sanity check confirms that 1000 iterations of
SeralizationUtilsTest.BatchedUploadTest takes the same time as before
(likely dominated by file operations anyways).

BUG=chromium:853730
TEST=Existing unit tests.

Change-Id: I5f4d0f32b2a0ba7cddce8206e2ae1961506209d2
Reviewed-on: https://chromium-review.googlesource.com/1113547
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Luigi Semenzato <semenzato@chromium.org>
Reviewed-by: Andrey Pronin <apronin@chromium.org>

[modify] https://crrev.com/d9fca6d90366b80a1db74ba382a72a173ae6cc25/metrics/metrics_library.cc
[modify] https://crrev.com/d9fca6d90366b80a1db74ba382a72a173ae6cc25/metrics/serialization/serialization_utils_unittest.cc
[modify] https://crrev.com/d9fca6d90366b80a1db74ba382a72a173ae6cc25/metrics/uploader/upload_service.cc
[modify] https://crrev.com/d9fca6d90366b80a1db74ba382a72a173ae6cc25/metrics/serialization/metric_sample.cc
[modify] https://crrev.com/d9fca6d90366b80a1db74ba382a72a173ae6cc25/metrics/serialization/metric_sample.h
[modify] https://crrev.com/d9fca6d90366b80a1db74ba382a72a173ae6cc25/metrics/serialization/serialization_utils.h
[modify] https://crrev.com/d9fca6d90366b80a1db74ba382a72a173ae6cc25/metrics/uploader/upload_service_test.cc
[modify] https://crrev.com/d9fca6d90366b80a1db74ba382a72a173ae6cc25/metrics/serialization/serialization_utils.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Jun 28 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/56d83fcf36106930bd02f75c91e29dcd98a47d06

commit 56d83fcf36106930bd02f75c91e29dcd98a47d06
Author: Mattias Nissler <mnissler@chromium.org>
Date: Thu Jun 28 16:55:38 2018

cryptohome: Basic UMA for mount-encrypted

This adds UMA metrics that report the system key and encryption key
status.

BUG=chromium:853730
TEST=Amended unit tests.

Change-Id: I7a3110161dbff1dde3bde149c29b11c6bea55f99
Reviewed-on: https://chromium-review.googlesource.com/1105984
Commit-Ready: Mattias Nissler <mnissler@chromium.org>
Tested-by: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Andrey Pronin <apronin@chromium.org>

[add] https://crrev.com/56d83fcf36106930bd02f75c91e29dcd98a47d06/cryptohome/init/send-mount-encrypted-metrics.conf
[modify] https://crrev.com/56d83fcf36106930bd02f75c91e29dcd98a47d06/cryptohome/mount_encrypted/encryption_key_unittest.cc
[modify] https://crrev.com/56d83fcf36106930bd02f75c91e29dcd98a47d06/cryptohome/mount_encrypted/tpm2.cc
[modify] https://crrev.com/56d83fcf36106930bd02f75c91e29dcd98a47d06/cryptohome/mount_encrypted.cc
[modify] https://crrev.com/56d83fcf36106930bd02f75c91e29dcd98a47d06/cryptohome/mount_encrypted/tpm1.cc
[modify] https://crrev.com/56d83fcf36106930bd02f75c91e29dcd98a47d06/cryptohome/mount_encrypted/encryption_key.cc
[modify] https://crrev.com/56d83fcf36106930bd02f75c91e29dcd98a47d06/cryptohome/mount_encrypted/tpm.h
[modify] https://crrev.com/56d83fcf36106930bd02f75c91e29dcd98a47d06/cryptohome/mount_encrypted/encryption_key.h

Cc: apronin@chromium.org sarthakkukreti@chromium.org
Status: Available (was: Started)
Key status UMA is present now.

Mount and TPM errors are pending on further code changes to enable them.
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 3

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e698a598d2edc61696b6b96652598cc9b9d24454

commit e698a598d2edc61696b6b96652598cc9b9d24454
Author: Mattias Nissler <mnissler@chromium.org>
Date: Tue Jul 03 20:25:38 2018

Add histogram declarations for Platform.MountEncrypted.*

This adds histogram declarations and corresponding enums for the
histograms added per
https://chromium-review.googlesource.com/c/chromiumos/platform2/+/1105984

BUG=chromium:853730
TEST=None

Change-Id: I64e0f89ce05a4ccd9f11f6290f1b9bdd479c2fbe
Reviewed-on: https://chromium-review.googlesource.com/1124690
Commit-Queue: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572337}
[modify] https://crrev.com/e698a598d2edc61696b6b96652598cc9b9d24454/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/e698a598d2edc61696b6b96652598cc9b9d24454/tools/metrics/histograms/histograms.xml

Cc: mnissler@chromium.org
Owner: ----
Components: OS>Systems>Security
Labels: -Pri-3 Pri-2
Labels: Cros-Hwsec-Ready

Sign in to add a comment