Skip to content

android: never look up children while iterating /data/app - #327

Open
mountaser wants to merge 1 commit into
bmax121:mainfrom
mountaser:fix/app-scan-two-phase
Open

mountaser wants to merge 1 commit into
bmax121:mainfrom
mountaser:fix/app-scan-two-phase

Conversation

@mountaser

Copy link
Copy Markdown

Summary

Never look up children while iterate_dir() holds /data/app, on any kernel version: the /data/app APK scan is now two-phase everywhere.

Problem

On kernels >= 6.1, apk_outer_actor called filp_open("/data/app/~~<hash>/") from inside the iterate_dir() actor. iterate_dir() holds the /data/app i_rwsem shared, and a child lookup that misses the dcache takes it again in lookup_slow(). If a writer is already queued (an install doing mkdir), the second down_read() blocks behind it and the task deadlocks against itself; everything touching /data/app then piles up.

Seen on a Pixel 8 Pro (6.1.162 GKI) with a cold-dcache pm install loop:

task:PackageManager state:D
  rwsem_down_write_slowpath <- down_write <- filename_create <- do_mkdirat
task:apd state:D
  down_read <- path_openat <- filp_open <- [KP] <- f2fs_readdir <- iterate_dir <- ... <- supercall

This is the uid_listener / supercall refresh path, so it is independent of the rename hook (#326).

Changes

  • Phase 1: the iterate_dir() actor only records ~~ names (no lookup, I/O or allocation).
  • Phase 2: after iterate_dir() returns and the lock is released, open and scan each recorded directory in iteration order.
  • 06887a4 limited the two-phase scan to old kernels because the collector could miss the APK: it kept at most 256 names of < 64 bytes and silently dropped the rest. The new list is packed and growable: when it fills, the actor sets an overflow flag, the caller doubles the buffer with no lock held and rescans from the start. Past 2 MiB of names the scan fails loudly with -E2BIG instead of truncating. The same list replaces the fixed array on old kernels.
  • Flat pass, log lines and return codes are unchanged; the first matching ~~ directory in iteration order still wins.

Testing

Device Build Result
Pixel 8 Pro, 6.1.162, kCFI + SCS without this change wedged within minutes under the loop above (stacks above)
Pixel 8 Pro with this change, same conditions (2 concurrent loops, cache dropped before each install) 50/50 installs completed; 69 scans, all found the manager (apk found (scramble)), including 54 uid_listener events (the apd refresh path); 0 hangs, 0 app dir scan errors
Xiaomi Mi 10, 4.19.157 with this change, 1,785 ~~ dirs (list grew 16 -> 32 -> 64 KiB) + 20 cold-cache installs 20/20 ok, 20 refreshes rc=0, 0 scan errors

On kernels >= 6.1 the trusted-manager APK scan descended into each
/data/app/~~<hash> directory from inside the /data/app iterate_dir()
actor (apk_outer_actor -> filp_open). iterate_dir() holds the /data/app
i_rwsem shared. A child lookup that misses the dcache takes it again in
lookup_slow(). If a writer is already queued, the nested down_read()
blocks behind it and the task deadlocks against itself. Every later
access to /data/app then piles up.

Seen on a Pixel 8 Pro (6.1.162 GKI) under a cold-dcache pm install loop.
PackageManager sat in D state in down_write <- filename_create <-
do_mkdirat. apd's refresh sat in D state in down_read <- path_openat <-
filp_open <- [KP] <- f2fs_readdir <- iterate_dir. This is the supercall
refresh path, so deferring the rename-hook refresh does not cover it.

Use a two-phase scan on all kernels. Phase 1 only records "~~" names
while iterating, with no lookup, I/O or allocation in the actor. Phase 2
opens and scans the recorded dirs, in iteration order, after
iterate_dir() has returned.

06887a4 limited the two-phase scan to old kernels because the collector
could miss the APK: it kept at most 256 names of fewer than 64 bytes and
silently dropped the rest. The new name list is packed and growable.
When it fills up, the actor stops and flags an overflow. The caller
doubles the buffer with the lock released and rescans from the start.
So no name is ever dropped. Past 2 MiB of names the scan fails loudly
with -E2BIG instead of truncating. The same list replaces the fixed
array on old kernels, which had the same silent cap.

The flat pass, log lines and return codes are unchanged. The first
matching ~~ dir in iteration order still wins.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant