fix: make kstorage write_kstorage find+replace atomic

[AYwlilwYA]

Problem

write_kstorage() had a race between finding an existing entry and replacing
it:

rcu_read_lock();
hlist_for_each_entry_rcu(pos, bucket, hnode) {  // lookup OUTSIDE the lock
    if (pos->did == did) { old = pos; break; }
}
spin_lock(lock);
if (old) {
    hlist_replace_rcu(&old->hnode, &new->hnode);  // replace INSIDE the lock
}

Two writers racing on the same did could both find the same old node.
The first thread replaces it (old->pprev becomes LIST_POISON2); the second
thread then calls hlist_replace_rcu() on the already-replaced node and
WRITE_ONCE(*old->pprev, new) writes through the poisoned pointer
(dead000000000122), causing a kernel Oops.

Observed in the field: apd (APatch daemon) crashed with
Unable to handle kernel paging request at virtual address dead000000000122
(LIST_POISON2) during APatch re-authorization after a system_server soft
restart, exactly at hlist_replace_rcu()'s old->pprev = LIST_POISON2.

Fix

Move the lookup inside the group spinlock so find + replace/add are atomic
under the same lock. remove_kstorage() already does its find+del under the
lock and is unaffected.

Verified: kstorage.c compiles cleanly (aarch64, no new warnings).

[Admirepowered]

kstorage在什么情况下会导致这样的情况，自旋锁会导致大量写入被拦截排队。正常使用不会触发到

[AYwlilwYA]

such as kill the system_server

[AYwlilwYA]

It will cause a kernel panic

[Admirepowered]

It cannot be reproduced and affects normal reading and writing, requiring manual handling of the reading and writing order

[AYwlilwYA]

Reopening this with the answers I should have given the first time. You asked the right question
("kstorage 在什么情况下会导致这样的情况") and I answered it badly — I'm sorry. Here is the concrete answer,
plus field evidence and the reason I think the spinlock concern doesn't hold.

---

1. Under what circumstances — the concrete concurrent-writer path

su_add_allow_uid() is only a write_kstorage() on key uid
(sucompat.c:72-83), so writing the same uid twice sequentially is harmless — it just overwrites.
The bug needs two writers inside write_kstorage() for the same did at the same time.
That is reachable, because load_ap_package_config() (userd.c:1133) has no locking of any kind
in its body and is reachable from four independent entry points:

entry point	site	who calls it
report_user_event("post-fs-data")	user_event.c:20	userspace event report
SUPERCALL_AP_LOAD_PACKAGE_CONFIG (0x100d)	supercall.c:290-291	apd, directly
supercmd reload-cfg	supercmd.c:466	apd
sucompat_init()	sucompat.c:410-411	boot, writes uid 2000 and 0

and there is a third writer: SUPERCALL_SU_GRANT_UID → call_grant_uid() (supercall.c:301-302),
also driven by apd.

On top of that, the kernel itself has a cross-context entry: the LSM hooks
after_security_path_rename / after_security_inode_rename → refresh_packages_list_tmp_rename()
(userd.c:1620+) fire when /system/packages.list.tmp is renamed, i.e. in the context of whatever
task performed the rename — normally system_server writing /data/system/packages.list.
So "install / uninstall / update any package" and "apd (re-)authorizes" can genuinely overlap.

When they do, both walk the same list and call su_add_allow_uid() for the same uid —
uid 2000 is in every one of those paths.

This is not exotic on the affected device: at boot the same batch of 5 uids is added twice within
2.6 s ([4.624261] … [4.624269], then [7.256972] … [7.256985]), because more than one of those
entry points fires. Repeated near-simultaneous triggers are the norm, not an edge case.

2. About the spinlock / "大量写入被拦截排队"

I think this concern doesn't hold, for three reasons:

1. The lookup that moves inside the lock is a hash-bucket walk.
   KSTORAGE_NBUCKETS = 256 (kstorage.c:21-22); on the affected device the su list holds
   6 entries, so buckets are almost always empty and the walk is 0–2 nodes — a handful of loads.
2. The expensive parts are already outside the lock. vmalloc() and memcpy() happen before
   spin_lock() — that's precisely why the patch does the allocation first (there's a comment about it).
   The critical section does not grow to the size the concern implies.
3. remove_kstorage() already does exactly this. It takes the same kstorage_glocks[gid] and walks
   the bucket inside the lock (kstorage.c:243-249). So "lookup + mutate under the group spinlock"
   is already the established pattern in this file — write_kstorage is the outlier.

Also, writes to this group only happen on config change / authorization, not on a hot path.

3. About "cannot be reproduced"

It's a race with a very narrow window, so being hard to reproduce is expected — I don't dispute that.
But it did happen on a real device, repeatedly. The crash data:

CPU: 5 PID: 7651 Comm: apd   Tainted: G  W  O
Unable to handle kernel paging request at virtual address dead000000000122
  ESR = 0x0000000096000044
  EC = 0x25: DABT (current EL), IL = 32 bits
pc : 0xffffffe7476af624   lr : 0xffffffe7476af618
Kernel panic - not syncing: Oops: Fatal exception

• dead000000000122 is LIST_POISON2 (on arm64 the 0xdead… prefix comes from
  CONFIG_ILLEGAL_POINTER_VALUE), i.e. a dereference of a poisoned list pointer.
• ESR = 0x96000044 → WnR = 1: it is a write fault, which matches
  rcu_assign_pointer(*(struct hlist_node **)old->pprev, new) inside hlist_replace_rcu().
• The PC is inside KP Region and the caller frame inside KP Hook — i.e. KernelPatch's own code,
  consistent with hlist_replace_rcu() being static inline and folded into write_kstorage().
• The line immediately before the panic is
  [+] KP D su_add_allow_uid: uid: 2000, to_uid: 0, sctx: u:r:magisk:s0, rc: 0.
• The device had recorded 11 abnormal reboots.

4. Disassembly from the device's running build

I pulled the boot partition and extracted the KP payload to confirm the code path. write_kstorage
looks like this (link base 0xd000, addresses from the running 0.13.3 build):

0x195c4  blr   x0                     ; rcu_read_lock()
0x195dc  ldr   x19, [x25, x1, lsl#3]  ; walk bucket  — NO spinlock held here
0x195e8  ldr   x19, [x19]
0x195f8  b.ne  #0x195e8
0x19614  blr   x1                     ; spin_lock(lock)   ← taken only AFTER the lookup
0x19618  ldp   x0, x1, [x19]          ; old->next / old->pprev
0x19624  stlr  x21, [x1]              ; *old->pprev = new     ← the faulting store
0x19638  mov   x0, #0x122
0x1963c  movk  x0, #0xdead, lsl #48
0x19640  str   x0, [x19, #8]          ; old->pprev = LIST_POISON2
0x1964c  blr   x1                     ; spin_unlock
0x1965c  blr   x0                     ; rcu_read_unlock()

The lookup is outside the lock and the replace is inside it — so two racers can hold the same old,
and the second one writes through a pointer the first already poisoned. With the patch applied the
same function is 624 bytes / 156 instructions instead of 668 / 167, and spin_lock precedes the walk.

5. About "requiring manual handling of the reading and writing order"

If I understand the concern correctly, I think the missing property here is mutual exclusion, not
ordering. hlist_replace_rcu() / hlist_add_head_rcu() publish with rcu_assign_pointer
(compiled to stlr), and readers use rcu_dereference — so the read/write ordering is already correct
and no manual barrier is needed. The defect is that the "find" is not covered by the same lock as the
"replace", so a writer can act on a stale old. Moving the find inside the lock is the minimal fix;
if you'd prefer a different shape (per-bucket locks, for instance) I'm happy to rework it.

remove_kstorage() is unaffected — it already does find + del under the lock.

---

I'd like to get the correctness fix in, in whatever shape you prefer. If there's a specific scenario
you want me to demonstrate first, tell me what it is and I'll build the repro.

[AYwlilwYA]

Follow-up: I could not reopen this PR — GitHub rejects both the GraphQL reopenPullRequest mutation
and the REST PATCH {state: "open"} with a 422, even though the head branch is intact
(AYwlilwYA:fix-kstorage-race @ bf264c6) and the PR is neither locked nor from a deleted fork.

So the same patch has been resubmitted as #309, with the analysis above as its description.
Leaving the reasoning here as well for the record.