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

Issue 728970 link

Starred by 2 users

Issue metadata

Status: Archived
Owner:
Last visit > 30 days ago
Closed: Jun 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug

Blocked on:
issue 731505

Blocking:
issue 719266



Sign in to add a comment

ENOSPC on setxattr on ext4 crypto file system

Project Member Reported by uekawa@google.com, Jun 2 2017

Issue description

There's two cases of NO_SPACE observed on
60.0.3100.0


set-attribute is failing on "Downloads" directory with NO_SPACE, on Terra and Celes.

I suspect they had stateful storage usage of around 30%, it is hard to believe it's actually running out of storage during migration.

ENOSPC can probably be returned from setxattr when it runs out of inline inode space. 
I believe file system inode size is 256 bytes kevin. I have no idea about terra and celes. Default is 256 bytes on ext4 but was 128 bytes on ext3 ?


I think it tries to allocate a block when it runs out of inline inode storage to store xattr
and wondering if that might be returning ENOSPC when it shouldn't.



localhost ~ # tune2fs -l /dev/mmcblk0p1 | grep 'Inode size'
Inode size:               256


 

Comment 1 by uekawa@google.com, Jun 2 2017

From quick inspection:

Typical attributes for Android:
user.inode_cache=0sHAEKAAAAAAA=
user.inode_code_cache=0sHQEKAAAAAAA=
user.default
user.AndroidCache="1"

typical Downloads directory xattrs:
user.TrackedDirectoryName="user"
user.GCacheFiles=0sAA==


user.serial="0"


Migration related:
trusted.CrosDirCryptoMigrationMtime
trusted.CrosDirCryptoMigrationAtime

Comment 2 by uekawa@google.com, Jun 2 2017

celes and terra are both braswell systems, x86_64, kernel 3.18.


Comment 3 by uekawa@google.com, Jun 2 2017

Labels: M-60 Ext4CryptoMigration
This should be addressed by  issue 719287 

Comment 5 by uekawa@google.com, Jun 2 2017

 60.0.3100.0 had  3446 migrations and this is 2 failures (i.e. < 0.1%)

Comment 6 by uekawa@google.com, Jun 2 2017

#4 are you sure? celes is 16 GB system, having around 10GB stateful partition, 
with 35% used means it's got 6GB free.

6GB of metadata with 4k blocks means 1.5 million files.

Comment 7 by uekawa@google.com, Jun 9 2017

Blockedon: 731505

Comment 8 by uekawa@google.com, Jun 11 2017

Owner: dspaid@chromium.org
Status: Assigned (was: Untriaged)
From the two data points, that data storage has around 6GB free space it does not seem like 719287 would address it.
I suspect if I filled the xattr on ecryptfs side, ext4 crypto side would have less space for xattr that it will fail with ENOSPC.

The available space is 4k+96 bytes.

I was suspecting that safe browsing stores source URL and referrer URL on xattr, size of URL can be around 4k. 

https://cs.chromium.org/chromium/src/content/common/quarantine/quarantine_constants_linux.h?q=setxattr+package:%5Echromium$&dr=C&l=18

Comment 9 by dspaid@chromium.org, Jun 11 2017

You're right, that would explain things.  Unfortunately I'm also not sure how we could fix it...
Storing the timestamp xattrs on the source file would only be a marginal change (the source file would likely just run out of space instead of the destination) and even if we don't apply timestamp xattrs at all we may still run out of space with the ext4 encryption xattr.
Increasing inode size would solve the problem, but can't really be done safely on existing systems.
Project Member

Comment 10 by bugdroid1@chromium.org, Jun 13 2017

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

commit b5cd8f14b7d6df9fd446db36661bfefa19e4e080
Author: dspaid <dspaid@chromium.org>
Date: Tue Jun 13 02:09:29 2017

Add histograms for enospc cryptomigration errors.

During ecryptfs->ext4-crypto migration we are getting some ENOSPC errors
which are not well understood.  Add UMA metrics to attempt to get a
better understanding as to the cause of these errors.
Metrics reporting added in
https://chromium-review.googlesource.com/c/527742/

BUG= 728970 
TEST=./validate_format.py && ./pretty_print.py

Review-Url: https://codereview.chromium.org/2935633002
Cr-Commit-Position: refs/heads/master@{#478864}

[modify] https://crrev.com/b5cd8f14b7d6df9fd446db36661bfefa19e4e080/tools/metrics/histograms/histograms.xml

Comment 11 by uekawa@google.com, Jun 13 2017

It seems like expecting storing timestamps on xattrs always is a bad assumption with the information we have, and deleting user data because we failed to store timestamp seems like a bad tradeoff.

How about not failing in this case (maybe count the occurrence of this) but fail-safe with the wrong timestamp.

In this case we aren't failing to store the timestamps, we're failing to store the xattrs originally present on the file.  I think at this point the best option is to talk to the security folks about setting a upper limit on mark-of-the-web annotations and truncating when we find things longer than that.  Will reach out to them.

Comment 13 by uekawa@google.com, Jun 16 2017

Status: Started (was: Assigned)

Comment 14 by uekawa@google.com, Jun 16 2017

Labels: -Ext4CryptoMigration ArcExt4Migration
Labels: Merge-Request-60
Requesting merge of https://chromium-review.googlesource.com/c/538454/ to skip quarantine xattrs which aren't used on chromeOS anyway.
Project Member

Comment 16 by sheriffbot@chromium.org, Jun 23 2017

Labels: -Merge-Request-60 Hotlist-Merge-Review Merge-Review-60
This bug requires manual review: M60 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Please add appropriate OSs.
Labels: OS-Chrome
Labels: -Merge-Review-60 Merge-Approved-60
Project Member

Comment 20 by bugdroid1@chromium.org, Jun 27 2017

Labels: merge-merged-release-R60-9592.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/6a8305d361c3f0b4fde05fd86267a7cd9fa956ac

commit 6a8305d361c3f0b4fde05fd86267a7cd9fa956ac
Author: Dan Spaid <dspaid@google.com>
Date: Tue Jun 27 01:51:58 2017

cryptohome: Do not migrate quarantine xattrs.

Chrome sets user.xdg.origin.url and user.xdg.referrer.url on any
downloaded files.  These are not actually used in ChromeOS, and may
cause the migration to fail if they are too long.  Skip them during the
migration.

TEST=cros_workon_make --board=samus --test cryptohome
BUG= chromium:728970 

Change-Id: I4dbfe857d2b0536dd36330619b04ac9ce8f702e2
Reviewed-on: https://chromium-review.googlesource.com/538454
Commit-Ready: Dan Spaid <dspaid@chromium.org>
Tested-by: Dan Spaid <dspaid@chromium.org>
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
(cherry picked from commit f4582035277268368d74ab42accb6a0888df6f65)
Reviewed-on: https://chromium-review.googlesource.com/549435
Reviewed-by: Dan Spaid <dspaid@chromium.org>
Commit-Queue: Dan Spaid <dspaid@chromium.org>

[modify] https://crrev.com/6a8305d361c3f0b4fde05fd86267a7cd9fa956ac/cryptohome/dircrypto_data_migrator/migration_helper.cc
[modify] https://crrev.com/6a8305d361c3f0b4fde05fd86267a7cd9fa956ac/cryptohome/dircrypto_data_migrator/migration_helper.h
[modify] https://crrev.com/6a8305d361c3f0b4fde05fd86267a7cd9fa956ac/cryptohome/dircrypto_data_migrator/migration_helper_unittest.cc

Labels: -Hotlist-Merge-Review -Merge-Approved-60
Status: Fixed (was: Started)

Comment 22 by uekawa@google.com, Jul 11 2017

Labels: Merge-Request-59
Labels: -Merge-Request-59 Merge-Approved-59
Project Member

Comment 24 by sheriffbot@chromium.org, Jul 14 2017

Cc: josa...@chromium.org gkihumba@google.com
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 25 by sheriffbot@chromium.org, Jul 17 2017

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Approved-59
no need to merge to M59 anymore.

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

Status: Archived (was: Fixed)

Sign in to add a comment