On Tue, Sep 10, 2019 at 05:40:51PM -0600, Andreas Dilger wrote:
> > diff --git a/e2fsck/encrypted_files.c b/e2fsck/encrypted_files.c
> > new file mode 100644
> > index 00000000..3dc706a7
> > --- /dev/null
> > +++ b/e2fsck/encrypted_files.c
> > @@ -0,0 +1,368 @@
> >
> > +/* A range of inodes which share the same encryption policy */
> > +struct encrypted_file_range {
> > + ext2_ino_t first_ino;
> > + ext2_ino_t last_ino;
> > + __u32 policy_id;
> > +};
>
> This seems like a clear win... As long as we have at least two inodes
> in a row with the same policy ID it will take less space than the previous
> version of the patch.
>
> > +static int handle_nomem(e2fsck_t ctx, struct problem_context *pctx)
> > +{
> > + fix_problem(ctx, PR_1_ALLOCATE_ENCRYPTED_DIRLIST, pctx);
> > + /* Should never get here */
> > + ctx->flags |= E2F_FLAG_ABORT;
> > + return 0;
> > +}
>
> It would be useful if the error message for PR_1_ALLOCATE_ENCRYPTED_DIRLIST
> printed the actual allocation size that failed, so that the user has some
> idea of how much memory would be needed. The underlying ext2fs_resize_mem()
> code doesn't print anything, just returns EXT2_ET_NO_MEMORY.
>
Yes, I can make it print the number of bytes using %N.
> > + if (info->file_ranges_count == info->file_ranges_capacity) {
> > + /* Double the capacity by default. */
> > + size_t new_capacity = info->file_ranges_capacity * 2;
> > +
> > + /* ... but go from 0 to 128 right away. */
> > + if (new_capacity < 128)
> > + new_capacity = 128;
> > +
> > + /* We won't need more than the filesystem's inode count. */
> > + if (new_capacity > ctx->fs->super->s_inodes_count)
> > + new_capacity = ctx->fs->super->s_inodes_count;
> > +
> > + /* To be safe, ensure the capacity really increases. */
> > + if (new_capacity < info->file_ranges_capacity + 1)
> > + new_capacity = info->file_ranges_capacity + 1;
>
> Not sure how this could happen (more inodes than s_inodes_count?), but
> better safe than sorry I guess?
>
Either that, or an integer overflow. It shouldn't really happen, but I think we
should have this check to be safe.
> > + if (ext2fs_resize_mem(info->file_ranges_capacity *
> > + sizeof(*range),
> > + new_capacity * sizeof(*range),
> > + &info->file_ranges) != 0)
> > + return handle_nomem(ctx, pctx);
>
> This is the only thing that gives me pause, potentially having a huge
> allocation, but I think the RLE encoding of entries and the fact we
> have overwhelmingly 64-bit CPUs means we could still run with swap
> (on an internal NVMe M.2 device) if really needed. A problem to fix
> if it ever actually rears its head, so long as there is a decent error
> message printed.
>
> > +/*
> > + * Find the ID of an inode's encryption policy, using the information saved
> > + * earlier.
> > + *
> > + * If the inode is encrypted, returns the policy ID or
> > + * UNRECOGNIZED_ENCRYPTION_POLICY. Else, returns NO_ENCRYPTION_POLICY.
> > + */
> > +__u32 find_encryption_policy(e2fsck_t ctx, ext2_ino_t ino)
> > +{
> > + const struct encrypted_file_info *info = ctx->encrypted_files;
> > + size_t l, r;
> > +
> > + if (info == NULL)
> > + return NO_ENCRYPTION_POLICY;
> > + l = 0;
> > + r = info->file_ranges_count;
> > + while (l < r) {
> > + size_t m = l + (r - l) / 2;
>
> Using the RLE encoding for the entries should also speed up searching
> here considerably. In theory, for a single-user Android filesystem
> there might only be one or two entries here. It would be interesting
> to run this on some of your filesystems to see what the average count
> of inodes per entry is.
>
On a freshly reset Android device I'm seeing 58 ranges for 4705 encrypted
inodes, so it's not quite *that* good, but it still helps a lot.
Note that there are actually 4 encryption policies on a single-user Android
device: system device-encrypted, user device-encrypted, user
credential-encrypted, and (recently added) per-boot encrypted.
- Eric
