[Code Health] CTRL_FREE hands the chip child a bare device pointer it frees unconditionally

[YunjiQin]

Category

Missing Validation

Component

Orchestration

Description

The three control commands that cross into a forked chip child do not agree on how a
device backing is named, and the one that destroys memory is the one that names it
most weakly.

_CTRL_COPY_TO / _CTRL_COPY_FROM send a ControlCopyRequest carrying two
BufferDescriptors. The child resolves each through its own ImportRegistry
(materialize(desc)), which keys on canonical identity, refuses a descriptor that
conflicts with one already mapped under that identity, and only then yields a base
address.

_CTRL_MALLOC / _CTRL_FREE carry a bare uint64. The child does:

elif sub_cmd == _CTRL_FREE:
    ptr = struct.unpack_from("Q", buf, _CTRL_OFF_ARG0)[0]
    cw.free(ptr)

There is no check that ptr is an allocation this child ever handed out. Whatever
uint64 reaches that slot is passed straight to the device allocator, so a wrong or
corrupted value frees an unrelated allocation, and the failure surfaces later as
memory corruption on a chip whose owner did nothing wrong.

The owner-side guard is real but it is on the wrong side of the boundary. Since
3f992487 the parent authorizes a free against its identity-keyed table
(Worker._child_alloc) and takes the address from the registered handle rather
than the caller's, so a bad handle cannot reach the mailbox through Worker.free.
That protects the API, not the frame: the child is trusting a number, and the trust
is unconditional.

Note the asymmetry is not "device memory cannot be named by identity". It is that
identity never crosses this particular boundary. Even on the copy path the identity
is a map-once cache key and a consistency check — the address still comes out of the
descriptor body via AdapterProfile.DEVICE_LOCAL / _resolve_body_as_va, because a
device VA allocated by the child is meaningful in the child. What is missing is a
child-side record of which identities it allocated.

Why the identity is absent today: the child is the allocator. CTRL_MALLOC carries
only size, the child returns a device VA, and the parent mints the identity
(_next_buffer_id()) afterwards when it wraps that VA in a Buffer. At
CTRL_MALLOC time there is no identity to send, so the child has never seen one.

Location

• python/simpler/worker.py:2858-2864 — the child's _CTRL_MALLOC / _CTRL_FREE handler
• python/simpler/worker.py:2865-2874 — the _CTRL_COPY_* handler, for contrast (descriptors + ImportRegistry)
• src/common/hierarchical/worker_manager.cpp:1238 — write_control_args(mbox(), CTRL_MALLOC, size)
• src/common/hierarchical/worker_manager.cpp:1356 — write_control_args(mbox(), CTRL_FREE, ptr)
• src/common/hierarchical/worker_manager.h:238-248 — ControlCopyRequest, the identity-bearing shape the copy path already uses
• python/simpler/buffer.py:613-617 — _RESOLVERS, showing DEVICE_LOCAL resolves the address from the descriptor body

Proposed Fix

Two options, in increasing order of what they actually buy:

1. Make CTRL_FREE carry a BufferDescriptor instead of a uint64, so all three
   control commands have one shape. Cheap, and it lets the child reject a descriptor
   that conflicts with one it already has mapped. But the address still comes from the
   body, and a device allocation is only in the child's ImportRegistry if a copy
   happened to touch it, so for most allocations there is nothing to check against.

2. Give the child its own identity to address table. The parent pre-mints the
   buffer_id and sends the identity with CTRL_MALLOC; the child records
   identity -> ptr when its allocator returns; CTRL_FREE then carries only the
   identity and the child resolves the address itself. This is the version that
   actually closes the hole — the child stops trusting an address it was handed — at
   the cost of a second lifetime that must stay in step with the parent's table
   (and a decision about what the child does when the two disagree).

Option 2 is the real fix; option 1 is mostly cosmetic. Neither is urgent: no path in
the repo produces a wrong pointer today, because identity to (owner_worker_id, base, nbytes) is injective on the owner side (every device Buffer is minted once with a
fresh _next_buffer_id() under _registry_lock).

Priority

Low (no impact today, good to fix eventually)