From 03df91230ae77f7b3307d9745d808c54563ef390 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Wed, 9 Nov 2022 20:06:23 +0000 Subject: [PATCH] LockSettingsStorage: avoid unnecessary directory syncs When a new file is created, syncing the parent directory is needed for the file to be guaranteed to be present after a crash. Unfortunately, directory syncs can be very expensive. On f2fs, each directory sync causes a filesystem checkpoint. For this reason, parent directory syncs aren't often done; AtomicFile doesn't do them. LockSettingsService currently syncs the parent directory after every file it writes. This causes some significant overhead. It's important that locksettings files be properly synced. However, most of these files are created at the same time and in the same directory: the synthetic password state directory. Specifically, this happens when the synthetic password and the initial LSKF-based protector is created, and also whenever another protector is added. Reduce the overhead by batching up these updates and just syncing the synthetic password state directory at the end of creating a protector. Test: 16.3 ms reduction in average time for 'locksettings set-pin' on a low-end device. (That was the easiest thing to test. However, probably more important than that is that this change will improve first boot performance slightly, too. That's when user 0's SP is created, and lots of other things are going on during the first boot, which might make fsyncs extra expensive at that time.) Bug: 232452368 Bug: 251131631 Bug: 251147505 Change-Id: Ia0cfc6a36eb6cfb05433bc772d4fd5843d30e6df --- .../locksettings/LockSettingsStorage.java | 31 +++++++++-- .../SyntheticPasswordManager.java | 53 ++++++++++++++++++- 2 files changed, 78 insertions(+), 6 deletions(-) diff --git a/services/core/java/com/android/server/locksettings/LockSettingsStorage.java b/services/core/java/com/android/server/locksettings/LockSettingsStorage.java index d836df56ba1ec..807ba3cf4463b 100644 --- a/services/core/java/com/android/server/locksettings/LockSettingsStorage.java +++ b/services/core/java/com/android/server/locksettings/LockSettingsStorage.java @@ -309,6 +309,10 @@ class LockSettingsStorage { } private void writeFile(File path, byte[] data) { + writeFile(path, data, /* syncParentDir= */ true); + } + + private void writeFile(File path, byte[] data, boolean syncParentDir) { synchronized (mFileWriteLock) { // Use AtomicFile to guarantee atomicity of the file write, including when an existing // file is replaced with a new one. This method is usually used to create new files, @@ -326,9 +330,11 @@ class LockSettingsStorage { file.failWrite(out); } // For performance reasons, AtomicFile only syncs the file itself, not also the parent - // directory. The latter must be done explicitly here, as some callers need a guarantee - // that the file really exists on-disk when this returns. - fsyncDirectory(path.getParentFile()); + // directory. The latter must be done explicitly when requested here, as some callers + // need a guarantee that the file really exists on-disk when this returns. + if (syncParentDir) { + fsyncDirectory(path.getParentFile()); + } mCache.putFile(path, data); } } @@ -378,10 +384,20 @@ class LockSettingsStorage { } } + /** + * Writes the synthetic password state file for the given user ID, protector ID, and state name. + * If the file already exists, then it is atomically replaced. + *

+ * This doesn't sync the parent directory, and a result the new state file may be lost if the + * system crashes. The caller must call {@link syncSyntheticPasswordState()} afterwards to sync + * the parent directory if needed, preferably after batching up other state file creations for + * the same user. We do it this way because directory syncs are expensive on some filesystems. + */ public void writeSyntheticPasswordState(int userId, long protectorId, String name, byte[] data) { ensureSyntheticPasswordDirectoryForUser(userId); - writeFile(getSyntheticPasswordStateFileForUser(userId, protectorId, name), data); + writeFile(getSyntheticPasswordStateFileForUser(userId, protectorId, name), data, + /* syncParentDir= */ false); } public byte[] readSyntheticPasswordState(int userId, long protectorId, String name) { @@ -392,6 +408,13 @@ class LockSettingsStorage { deleteFile(getSyntheticPasswordStateFileForUser(userId, protectorId, name)); } + /** + * Ensures that all synthetic password state files for the user have really been saved to disk. + */ + public void syncSyntheticPasswordState(int userId) { + fsyncDirectory(getSyntheticPasswordDirectoryForUser(userId)); + } + public Map> listSyntheticPasswordProtectorsForAllUsers(String stateName) { Map> result = new ArrayMap<>(); final UserManager um = UserManager.get(mContext); diff --git a/services/core/java/com/android/server/locksettings/SyntheticPasswordManager.java b/services/core/java/com/android/server/locksettings/SyntheticPasswordManager.java index 3fd488e6d3572..73a16fdbc7c96 100644 --- a/services/core/java/com/android/server/locksettings/SyntheticPasswordManager.java +++ b/services/core/java/com/android/server/locksettings/SyntheticPasswordManager.java @@ -619,12 +619,16 @@ public class SyntheticPasswordManager { /** * Creates a new synthetic password (SP) for the given user. - * + *

* Any existing SID for the user is cleared. - * + *

* Also saves the escrow information necessary to re-generate the synthetic password under * an escrow scheme. This information can be removed with {@link #destroyEscrowData} if * password escrow should be disabled completely on the given user. + *

+ * {@link syncState()} is not called yet; the caller should create a protector afterwards, which + * handles this. This makes it so that all the user's initial SP state files, including the + * initial LSKF-based protector, are efficiently created with only a single {@link syncState()}. */ SyntheticPassword newSyntheticPassword(int userId) { clearSidForUser(userId); @@ -668,6 +672,7 @@ public class SyntheticPasswordManager { private void saveSyntheticPasswordHandle(byte[] spHandle, int userId) { saveState(SP_HANDLE_NAME, spHandle, NULL_PROTECTOR_ID, userId); + syncState(userId); } private boolean loadEscrowData(SyntheticPassword sp, int userId) { @@ -677,6 +682,11 @@ public class SyntheticPasswordManager { return e0 != null && p1 != null; } + /** + * Saves the escrow data for the synthetic password. The caller is responsible for calling + * {@link syncState()} afterwards, once the user's other initial synthetic password state files + * have been created. + */ private void saveEscrowData(SyntheticPassword sp, int userId) { saveState(SP_E0_NAME, sp.mEncryptedEscrowSplit0, NULL_PROTECTOR_ID, userId); saveState(SP_P1_NAME, sp.mEscrowSplit1, NULL_PROTECTOR_ID, userId); @@ -708,6 +718,10 @@ public class SyntheticPasswordManager { return buffer.getInt(); } + /** + * Creates a file that stores the Weaver slot the protector is using. The caller is responsible + * for calling {@link syncState()} afterwards, once all the protector's files have been created. + */ private void saveWeaverSlot(int slot, long protectorId, int userId) { ByteBuffer buffer = ByteBuffer.allocate(Byte.BYTES + Integer.BYTES); buffer.put(WEAVER_VERSION); @@ -837,6 +851,7 @@ public class SyntheticPasswordManager { } createSyntheticPasswordBlob(protectorId, PROTECTOR_TYPE_LSKF_BASED, sp, protectorSecret, sid, userId); + syncState(userId); // ensure the new files are really saved to disk return protectorId; } @@ -996,6 +1011,7 @@ public class SyntheticPasswordManager { saveSecdiscardable(tokenHandle, tokenData.secdiscardableOnDisk, userId); createSyntheticPasswordBlob(tokenHandle, getTokenBasedProtectorType(tokenData.mType), sp, tokenData.aggregatedSecret, 0L, userId); + syncState(userId); // ensure the new files are really saved to disk tokenMap.get(userId).remove(tokenHandle); if (tokenData.mCallback != null) { tokenData.mCallback.onEscrowTokenActivated(tokenHandle, userId); @@ -1003,6 +1019,11 @@ public class SyntheticPasswordManager { return true; } + /** + * Creates a synthetic password blob, i.e. the file that stores the encrypted synthetic password + * (or encrypted escrow secret) for a protector. The caller is responsible for calling + * {@link syncState()} afterwards, once all the protector's files have been created. + */ private void createSyntheticPasswordBlob(long protectorId, byte protectorType, SyntheticPassword sp, byte[] protectorSecret, long sid, int userId) { final byte[] spSecret; @@ -1118,6 +1139,7 @@ public class SyntheticPasswordManager { // (getting rid of CREDENTIAL_TYPE_PASSWORD_OR_PIN) pwd.credentialType = credential.getType(); saveState(PASSWORD_DATA_NAME, pwd.toBytes(), protectorId, userId); + syncState(userId); synchronizeFrpPassword(pwd, 0, userId); } else { Slog.w(TAG, "Fail to re-enroll user password for user " + userId); @@ -1156,6 +1178,7 @@ public class SyntheticPasswordManager { if (result.syntheticPassword != null && !credential.isNone() && !hasPasswordMetrics(protectorId, userId)) { savePasswordMetrics(credential, result.syntheticPassword, protectorId, userId); + syncState(userId); // Not strictly needed as the upgrade can be re-done, but be safe. } return result; } @@ -1275,6 +1298,7 @@ public class SyntheticPasswordManager { + blob.mProtectorType); createSyntheticPasswordBlob(protectorId, blob.mProtectorType, result, protectorSecret, sid, userId); + syncState(userId); // Not strictly needed as the upgrade can be re-done, but be safe. } return result; } @@ -1396,12 +1420,21 @@ public class SyntheticPasswordManager { return ArrayUtils.concat(data, secdiscardable); } + /** + * Generates and writes the secdiscardable file for the given protector. The caller is + * responsible for calling {@link syncState()} afterwards, once all the protector's files have + * been created. + */ private byte[] createSecdiscardable(long protectorId, int userId) { byte[] data = secureRandom(SECDISCARDABLE_LENGTH); saveSecdiscardable(protectorId, data, userId); return data; } + /** + * Writes the secdiscardable file for the given protector. The caller is responsible for + * calling {@link syncState()} afterwards, once all the protector's files have been created. + */ private void saveSecdiscardable(long protectorId, byte[] secdiscardable, int userId) { saveState(SECDISCARDABLE_NAME, secdiscardable, protectorId, userId); } @@ -1445,6 +1478,11 @@ public class SyntheticPasswordManager { return VersionedPasswordMetrics.deserialize(decrypted).getMetrics(); } + /** + * Creates the password metrics file: the file associated with the LSKF-based protector that + * contains the encrypted metrics about the LSKF. The caller is responsible for calling + * {@link syncState()} afterwards if needed. + */ private void savePasswordMetrics(LockscreenCredential credential, SyntheticPassword sp, long protectorId, int userId) { final byte[] encrypted = SyntheticPasswordCrypto.encrypt(sp.deriveMetricsKey(), @@ -1466,10 +1504,21 @@ public class SyntheticPasswordManager { return mStorage.readSyntheticPasswordState(userId, protectorId, stateName); } + /** + * Persists the given synthetic password state for the given user ID and protector ID. + *

+ * For performance reasons, this doesn't sync the user's synthetic password state directory. As + * a result, it doesn't guarantee that the file will really be present after a crash. If that + * is needed, call {@link syncState()} afterwards, preferably after batching up related updates. + */ private void saveState(String stateName, byte[] data, long protectorId, int userId) { mStorage.writeSyntheticPasswordState(userId, protectorId, stateName, data); } + private void syncState(int userId) { + mStorage.syncSyntheticPasswordState(userId); + } + private void destroyState(String stateName, long protectorId, int userId) { mStorage.deleteSyntheticPasswordState(userId, protectorId, stateName); }