New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 703307 link

Starred by 6 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Feature

Blocking:
issue 761180
issue 699818
issue 712897



Sign in to add a comment

ext4 crypto requires drop_caches that makes Chrome loads very slowly after sign out.

Project Member Reported by sonnysasaka@google.com, Mar 20 2017

Issue description

UserAgent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.110 Safari/537.36
Platform: 9334.x.x

Steps to reproduce the problem:
1. Get Chrome OS with Platform 9334.13 (any Chrome version).
2. Sign in with user A. By default this user will use ext4 crypto.
3. Sign out and time the "black screen" time. Also notice that dmesg shows "cryptohome: drop_caches".
4. Change /etc/init/cryptohomed.conf to comment out "env DIRENCRYPTION_FLAGS=--direncryption. Sign in with another user B. This user will use ecryptfs encryption instead of ext4 crypto.
5. Sign out and time the "black screen" time. Also notice that dmesg does not show "drop_caches" this time. 
6. Compare the black screen time of user A and user B. User A takes much longer than user B during sign out black screen.

Optionally:
1. Add the following flags to /etc/chrome_dev.conf: --trace-startup --trace-startup-file=/tmp/trace.json --trace-startup-duration=8. We can compare the trace.json from user A and user B and it looks as though user B trace is from a binary which has not been loaded from disk before (there is a lot of waiting (not CPU) time. This suggests that Chrome binary has been purged out from page cache.
2. When estimating the black screen time we can also use:
echo `bootstat_get_last login-prompt-visible` - `bootstat_get_last chrome-exec` | bc.

What is the expected behavior?
Sign out time when using ext4 crypto shouldn't take longer than when using ecryptfs.

What went wrong?
ext4 crypto requires drop_caches after every "unmount" and this makes Chrome binary get removed from Linux kernel page cache so when Chrome restarts (as a consequence from signing out) it loads slowly as the binary needs to be reloaded from disk. This is perceived as very long black screen.

Did this work before? No 

Chrome version: Any version  Channel: n/a
OS Version: 58
Flash Version: 

Possible solution ideas:
1. Do not drop_cache? But I think this will be insecure.
2. Have a daemon just to "hold" Chrome binary in memory so it won't get kicked out from cache when drop_caches happens. But I think this will still leave some things slow because there are other files (e.g. libraries that Chrome depends) removed from page cache.
3. Selectively drop_caches only for the user directory. I don't think Linux kernel currently supports this but maybe there is plan?
4. Simulate the "drop cache" in the ext4 crypto layer so it can selectively mark those files as cache-invalid.
 
Blocking: 699818
Cc: r...@chromium.org abodenha@chromium.org gwendal@chromium.org apronin@chromium.org hashimoto@chromium.org

Comment 2 by r...@chromium.org, Mar 20 2017

Cc: snanda@chromium.org
Owner: gwendal@chromium.org
Status: Assigned (was: Unconfirmed)
Dropping caches for all the mounted file systems seems a bit sub-optimal.
Is there any way we can only specifically drop caches for the ext4 file systems?


This article (http://unix.stackexchange.com/questions/36907/drop-a-specific-file-from-the-linux-filesystem-cache) discusses some potential solutions but I am not sure if any of them apply to us.

Tentatively assigning to Gwendal. Feel free to re-assign if there is a more appropriate owner.
Cc: tytso@google.com
Components: OS>Systems
Current investigation:

There was a patch  "[PATCH] sysctl: Add a feature to drop caches selectively"
to drop cache selectively, but it was shoot down: from the thread, there was no solid user case (mount/unmount is enough) and the interface was not clean.

fadvise FADV_DONTNEED only applies to file.

There is a "Fadvise: Directory level page cache cleaning support" https://lwn.net/Articles/578207/ proposal to apply on directory as well.

Looking into the code of fadvise.c, invalidate_mapping_pages() will only free pages that are clean and unused.
That's not as secure as calling truncate_inode_pages_range().
Cc: zalcorn@chromium.org
Cc: uekawa@chromium.org mitsuji@chromium.org
Blocking: 712897

Comment 7 by ebiggers@google.com, Jun 27 2017

FYI, I'm planning to look into solving this by adding an ioctl that evicts all unused inodes which were "unlocked" with a particular master key.  That's really what Chrome OS needs here, I think, and Android has a similar need as well.  If every inode using a particular master key is evicted, then all derived keys will be zeroed out, and Linux v4.12+ will also zero out the payload of the keyring key when it's no longer referenced.  Zeroing the pagecache pages after dropping them would be good too but might not be practical.
Project Member

Comment 8 by sheriffbot@chromium.org, Jul 21 2017

Labels: Hotlist-Google
Blocking: 761180
Labels: -Pri-2 Pri-1
This is a pretty major regression in performance. Bumping up to P1 in the hopes someone will find time to get it fixed soon.
Cc: sarthakkukreti@chromium.org
Owner: sarthakkukreti@chromium.org
Added a prototype CL for clearing caches at a directory level @ https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/701920 

Basically, given a directory, walk through the directory tree (using d_walk), purge all dentries and evict all inodes. On signing out, this ioctl can be used to purge the caches for the user directory in /home/.shadow/XXXXX.
Update: After discussions with @ebiggers, we are now creating an ioctl for per-superblock clearing of caches. I have updated the CL ( https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/701920 
) accordingly.

This workaround is temporary: @ebiggers is working on a patch set to add the ioctl referenced in c#7. Once that is upstreamed, this can be reverted.
Project Member

Comment 14 by bugdroid1@chromium.org, Oct 19 2017

Labels: merge-merged-chromeos-4.4
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/96e56ffa00a57f0990f7d0dbf9161668fa11f620

commit 96e56ffa00a57f0990f7d0dbf9161668fa11f620
Author: Sarthak Kukreti <sarthakkukreti@chromium.org>
Date: Thu Oct 19 07:52:28 2017

CHROMIUM: vfs: Add support for superblock-level drop_cache

Drops caches and evicts inodes for a superblock. Requires
CAP_SYS_ADMIN. Used as a workaround, by cryptohome, for clearing
cached unencrypted data for ext4 mounts.

BUG= chromium:703307 
TEST=User data is encrypted immediately after logout

Change-Id: Ib27c7e7a2e5a186e7fe2e8c3a2864562f156d8f9
Signed-off-by: Sarthak Kukreti <sarthakkukreti@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/701920
Reviewed-by: Eric Biggers <ebiggers@google.com>

[modify] https://crrev.com/96e56ffa00a57f0990f7d0dbf9161668fa11f620/fs/compat_ioctl.c
[modify] https://crrev.com/96e56ffa00a57f0990f7d0dbf9161668fa11f620/include/linux/fs.h
[modify] https://crrev.com/96e56ffa00a57f0990f7d0dbf9161668fa11f620/fs/ioctl.c
[modify] https://crrev.com/96e56ffa00a57f0990f7d0dbf9161668fa11f620/include/uapi/linux/fs.h

Project Member

Comment 15 by bugdroid1@chromium.org, Oct 25 2017

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

commit f967f3a736f0d25659b9efa1eeda0024d18c6df2
Author: Sarthak Kukreti <sarthakkukreti@chromium.org>
Date: Wed Oct 25 05:47:59 2017

cryptohome: selective superblock-level drop caches

On logout, cryptohome currently drops cache for all file systems
to ensure cached decrypted data is cleared. This results in
eviction of binaries from the pagecache that are reloaded for
the new session. The user experiences this as a prolonged black
screen before the login screen reappears.
This change adds the capability to selectively drop caches per
mount.

BUG= chromium:703307 
TEST='FEATURES=test emerge-reef cryptohome'
User data is encrypted after sign out.
User data is not corrupted on logout.
time(Logout -> Login screen) speeds up by ~1s.
CQ-DEPEND=CL:701920

Change-Id: I751b1610c7664d199cf64c7b8e227a61a65b657b
Reviewed-on: https://chromium-review.googlesource.com/701265
Commit-Ready: Sarthak Kukreti <sarthakkukreti@chromium.org>
Tested-by: Sarthak Kukreti <sarthakkukreti@chromium.org>
Reviewed-by: Gwendal Grignou <gwendal@google.com>

[modify] https://crrev.com/f967f3a736f0d25659b9efa1eeda0024d18c6df2/cryptohome/mount_unittest.cc
[modify] https://crrev.com/f967f3a736f0d25659b9efa1eeda0024d18c6df2/cryptohome/platform.h
[modify] https://crrev.com/f967f3a736f0d25659b9efa1eeda0024d18c6df2/cryptohome/mount.cc
[modify] https://crrev.com/f967f3a736f0d25659b9efa1eeda0024d18c6df2/cryptohome/platform.cc
[modify] https://crrev.com/f967f3a736f0d25659b9efa1eeda0024d18c6df2/cryptohome/mock_platform.h

Project Member

Comment 16 by bugdroid1@chromium.org, Oct 31 2017

Labels: merge-merged-chromeos-3.18
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/f2f1993fd7d1c653946db4df98f1f4d7b1d3eabc

commit f2f1993fd7d1c653946db4df98f1f4d7b1d3eabc
Author: Sarthak Kukreti <sarthakkukreti@chromium.org>
Date: Tue Oct 31 22:15:32 2017

BACKPORT: CHROMIUM: vfs: Add support for superblock-level drop_cache

Drops caches and evicts inodes for a superblock. Requires
CAP_SYS_ADMIN. Used as a workaround, by cryptohome, for clearing
cached unencrypted data for ext4 mounts.

BUG= chromium:703307 
TEST=User data is encrypted immediately after logout

 Conflicts:
        fs/compat_ioctl.c: FITRIM not defined in 3.18

Change-Id: Ib27c7e7a2e5a186e7fe2e8c3a2864562f156d8f9
Signed-off-by: Sarthak Kukreti <sarthakkukreti@chromium.org>
(cherry picked from commit 96e56ffa00a57f0990f7d0dbf9161668fa11f620)
Reviewed-on: https://chromium-review.googlesource.com/734460
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>

[modify] https://crrev.com/f2f1993fd7d1c653946db4df98f1f4d7b1d3eabc/fs/compat_ioctl.c
[modify] https://crrev.com/f2f1993fd7d1c653946db4df98f1f4d7b1d3eabc/include/linux/fs.h
[modify] https://crrev.com/f2f1993fd7d1c653946db4df98f1f4d7b1d3eabc/fs/ioctl.c
[modify] https://crrev.com/f2f1993fd7d1c653946db4df98f1f4d7b1d3eabc/include/uapi/linux/fs.h

Project Member

Comment 17 by bugdroid1@chromium.org, Oct 31 2017

Labels: merge-merged-chromeos-3.14
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/afbd1659af186134e221274b4a439b243179d51a

commit afbd1659af186134e221274b4a439b243179d51a
Author: Sarthak Kukreti <sarthakkukreti@chromium.org>
Date: Tue Oct 31 22:15:20 2017

BACKPORT: CHROMIUM: vfs: Add support for superblock-level drop_cache

Drops caches and evicts inodes for a superblock. Requires
CAP_SYS_ADMIN. Used as a workaround, by cryptohome, for clearing
cached unencrypted data for ext4 mounts.

BUG= chromium:703307 
TEST=User data is encrypted immediately after logout

 Conflicts:
	fs/compat_ioctl.c: FITRIM not defined in 3.14.

Change-Id: Ib27c7e7a2e5a186e7fe2e8c3a2864562f156d8f9
Signed-off-by: Sarthak Kukreti <sarthakkukreti@chromium.org>
(cherry picked from commit 96e56ffa00a57f0990f7d0dbf9161668fa11f620)
Reviewed-on: https://chromium-review.googlesource.com/734461
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>

[modify] https://crrev.com/afbd1659af186134e221274b4a439b243179d51a/fs/compat_ioctl.c
[modify] https://crrev.com/afbd1659af186134e221274b4a439b243179d51a/include/linux/fs.h
[modify] https://crrev.com/afbd1659af186134e221274b4a439b243179d51a/fs/ioctl.c
[modify] https://crrev.com/afbd1659af186134e221274b4a439b243179d51a/include/uapi/linux/fs.h

Status: Fixed (was: Assigned)
Marking as fixed since all the changes have landed. 

A new patchset to enable key-based eviction (as mentioned in c#7) is under review on the upstream mailing lists [1]. This should be revisited once the patchset is upstreamed.

[1] https://www.spinics.net/lists/keyrings/msg03101.html
Project Member

Comment 19 by bugdroid1@chromium.org, Dec 19 2017

Labels: merge-merged-release-R63-10032.B-chromeos-3.14
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/7e9d26182ad6ed43a1dca58b85572d79b3e828e7

commit 7e9d26182ad6ed43a1dca58b85572d79b3e828e7
Author: Sarthak Kukreti <sarthakkukreti@chromium.org>
Date: Tue Dec 19 18:17:01 2017

BACKPORT: CHROMIUM: vfs: Add support for superblock-level drop_cache

Drops caches and evicts inodes for a superblock. Requires
CAP_SYS_ADMIN. Used as a workaround, by cryptohome, for clearing
cached unencrypted data for ext4 mounts.

BUG= chromium:703307 
TEST=User data is encrypted immediately after logout

 Conflicts:
	fs/compat_ioctl.c: FITRIM not defined in 3.14.

Change-Id: Ib27c7e7a2e5a186e7fe2e8c3a2864562f156d8f9
Signed-off-by: Sarthak Kukreti <sarthakkukreti@chromium.org>
(cherry picked from commit 96e56ffa00a57f0990f7d0dbf9161668fa11f620)
Reviewed-on: https://chromium-review.googlesource.com/734461
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
(cherry picked from commit afbd1659af186134e221274b4a439b243179d51a)
Reviewed-on: https://chromium-review.googlesource.com/834419
Commit-Queue: Gwendal Grignou <gwendal@chromium.org>
Tested-by: Gwendal Grignou <gwendal@chromium.org>

[modify] https://crrev.com/7e9d26182ad6ed43a1dca58b85572d79b3e828e7/fs/compat_ioctl.c
[modify] https://crrev.com/7e9d26182ad6ed43a1dca58b85572d79b3e828e7/include/linux/fs.h
[modify] https://crrev.com/7e9d26182ad6ed43a1dca58b85572d79b3e828e7/fs/ioctl.c
[modify] https://crrev.com/7e9d26182ad6ed43a1dca58b85572d79b3e828e7/include/uapi/linux/fs.h

Comment 20 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Comment 21 by dchan@chromium.org, Jan 23 2018

Status: Fixed (was: Archived)
Project Member

Comment 22 by bugdroid1@chromium.org, Feb 28 2018

Labels: merge-merged-chromeos-4.14
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/8c29dac0777f07d2d622c833fc89e1f06aa22a4e

commit 8c29dac0777f07d2d622c833fc89e1f06aa22a4e
Author: Sarthak Kukreti <sarthakkukreti@chromium.org>
Date: Wed Feb 28 04:17:31 2018

CHROMIUM: vfs: Add support for superblock-level drop_cache

Drops caches and evicts inodes for a superblock. Requires
CAP_SYS_ADMIN. Used as a workaround, by cryptohome, for clearing
cached unencrypted data for ext4 mounts.
(forward port from chromeos-4.4).

BUG= chromium:703307 
TEST=User data is encrypted immediately after logout

 Conflicts:
        fs/ioctl.c
        include/linux/fs.h

Change-Id: Ib27c7e7a2e5a186e7fe2e8c3a2864562f156d8f9
Signed-off-by: Sarthak Kukreti <sarthakkukreti@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/701920
Reviewed-by: Eric Biggers <ebiggers@google.com>
(cherry picked from commit 96e56ffa00a57f0990f7d0dbf9161668fa11f620)
Signed-off-by: Sarthak Kukreti <sarthakkukreti@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/927489
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>

[modify] https://crrev.com/8c29dac0777f07d2d622c833fc89e1f06aa22a4e/fs/compat_ioctl.c
[modify] https://crrev.com/8c29dac0777f07d2d622c833fc89e1f06aa22a4e/include/linux/fs.h
[modify] https://crrev.com/8c29dac0777f07d2d622c833fc89e1f06aa22a4e/fs/ioctl.c
[modify] https://crrev.com/8c29dac0777f07d2d622c833fc89e1f06aa22a4e/include/uapi/linux/fs.h

Sign in to add a comment