Avoid delays in binder transactions due to preemption of
binder_deferred_func() with binder_deferred_lock held.
Bug: 66914906
Test: boots; binder tests pass; trace analysis in bug
Change-Id: I1a89ba15b27e43552890292f135a5e2d969231e2
Signed-off-by: Corey Tabaka <eieio@google.com>
Binder driver allocates buffer meta data in a region that is mapped
in user space. These meta data contain pointers in the kernel.
This patch allocates buffer meta data on the kernel heap that is
not mapped in user space, and uses a pointer to refer to the data mapped.
Also move alloc->buffers initialization from mmap to init since it's
now used even when mmap failed or was not called.
Bug: 36007193
Change-Id: I66d4221d257bbb539a5e5f259d383084746e6773
Signed-off-by: Sherry Yang <sherryy@android.com>
binder_alloc_selftest tests that alloc_new_buf handles page allocation and
deallocation properly when allocate and free buffers. The test allocates 5
buffers of various sizes to cover all possible page alignment cases, and
frees the buffers using a list of exhaustive freeing order.
Test: boot the device with ANDROID_BINDER_IPC_SELFTEST config option enabled.
Allocator selftest passes.
Bug: 36007193
Change-Id: I7b903390ac4ba5b59a15b28dff620ea038c86bf7
Signed-off-by: Sherry Yang <sherryy@android.com>
Certain usecases like camera are constantly allocating and freeing
binder buffers beyond the first 4k resulting in mmap_sem contention.
If we expand the allocated range from 4k to something higher, we can
reduce the contention. Tests show that 6 pages is enough to cause very
little update_page_range operations and reduces contention.
Bug: 36727951
Change-Id: I28bc3fb9b33c764c257e28487712fce2a3c1078b
Reported-by: Tim Murray <timmurray@google.com>
Signed-off-by: Joel Fernandes <joelaf@google.com>
Pre-allocate 1 instead of 6 pages as in the original patch,
as we use this pre-allocated page to prevent the first page
from getting unpinned after removing the buffer headers,
rather than pinning pages to speedup larger transactions.
Change-Id: Id027adcfd61b2d6b37f69a3f6009a068e90e84f0
Signed-off-by: Sherry Yang <sherryy@android.com>
The BINDER_GET_NODE_DEBUG_INFO ioctl will return debug info on
a node. Each successive call reusing the previous return value
will return the next node. The data will be used by
libmemunreachable to mark the pointers with kernel references
as reachable.
Bug: 28275695
Change-Id: Idbbafa648a33822dc023862cd92b51a595cf7c1c
Signed-off-by: Colin Cross <ccross@android.com>
Signed-off-by: Martijn Coenen <maco@android.com>
Because we're not guaranteed that subsequent calls
to poll() will have a poll_table_struct parameter
with _qproc set. When _qproc is not set, poll_wait()
is a noop, and we won't be woken up correctly.
Bug: 64552728
Change-Id: I5b904c9886b6b0994d1631a636f5c5e5f6327950
Test: binderLibTest stops hanging with new test
Signed-off-by: Martijn Coenen <maco@android.com>
This can cause issues with processes using the poll()
interface:
1) client sends two oneway transactions
2) the second one gets queued on async_todo
(because the server didn't handle the first one
yet)
3) server returns from poll(), picks up the
first transaction and does transaction work
4) server is done with the transaction, sends
BC_FREE_BUFFER, and the second transaction gets
moved to thread->todo
5) libbinder's handlePolledCommands() only handles
the commands in the current data buffer, so
doesn't see the new transaction
6) the server continues running and issues a new
outgoing transaction. Now, it suddenly finds
the incoming oneway transaction on its thread
todo, and returns that to userspace.
7) userspace does not expect this to happen; it
may be holding a lock while making the outgoing
transaction, and if handling the incoming
trasnaction requires taking the same lock,
userspace will deadlock.
By queueing the async transaction to the proc
workqueue, we make sure it's only picked up when
a thread is ready for proc work.
Bug: 38201220
Bug: 63075553
Bug: 63079216
Change-Id: I3a6ac54c71b4880afaedd729152b5ad75da223bf
Signed-off-by: Martijn Coenen <maco@android.com>
Let's say we have the following transaction
stack:
Thread1 - t1 -> Thread2
Thread2 - t2 -> Thread1
Thread1 - t3 -> Thread3
The processes hosting Thread1 and Thread 2 both die;
when Thread3 tries to send a reply to t3, it finds
that the process is dead, and calls
binder_send_failed_reply() on t3. At the same time,
binder_free_thread() starts running on Thread1
and Thread2.
binder_free_thread() walks the transaction stack of
Thread2 first, and makes the following change:
t2->from = NULL;
binder_free_thread() then walks the transaction
stack of Thread1, and makes the following change:
t3->from = NULL;
It then suspends, and binder_send_failed_reply(t3)
runs; it sees t3->from = NULL, so continues walking
the transaction stack. It finds t2->from = NULL,
and continues with t1. It sees that t1->from is
still set, and assigns it to target_thread. At
that point, it tries to take the proc lock of that
thread, and blocks because binder_free_thread()
still has that lock. binder_free_thread() completes,
and drops the lock.
binder_send_failed_reply() acquires the lock,
and tries to pop t1 from the transaction stack.
This fails with a BUG_ON(), because the transaction
stack of Thread1 is still pointing to t3.
Fixed by checking whether we've marked the thread
as a zombie after taking the lock; in that case,
just continue unwinding the stack (if needed).
Bug: 64074090
Change-Id: I3b5ca0058b14b21038764147f78186310b812f80
Signed-off-by: Martijn Coenen <maco@android.com>
When we create a new reference to a zombie node
(which is allowed as long as there are other refs
to the node), we don't correctly initialize the
node_is_zombie field of the ref, which can lead to
death recipients never being fired.
Bug: 63988502
Test: new binderLibTest
Change-Id: I06287947d7a2f59c25362850b389c20c8a3b6929
Signed-off-by: Martijn Coenen <maco@android.com>
When inserting a new binder_thread into the proc->threads
rbtree, the "parent" node in the tree is not recalculated
if the tree becomes empty between the initial lookup and
the recalc of the insertion point in binder_get_thread().
If the tree is empty on the recalc, the parent found in
the first lookup was used, corrupting the tree and causing
the crash. Reinitialize the parent pointer between lookups.
Bug: 62931241
Bug: 62272866
Bug: 62193133
Test: tested manually
Change-Id: I039cfe419022a39441220618e8a92433ecc2a2c5
Signed-off-by: Todd Kjos <tkjos@google.com>
The binder debug message for transaction failed included
the sender information, but not the target. Add target pid:tid
to the message.
Bug: 62976829
Test: tested manually
Change-Id: I985a5107c7ba16bcf0bfe6f513ddbe5f000825fd
Signed-off-by: Todd Kjos <tkjos@google.com>
Because we have disabled RT priority inheritance for
the regular binder domain, the following can happen:
1) thread A (prio 98) calls into thread B
2) because RT prio inheritance is disabled, thread B
runs at the lowest nice (prio 100) instead
3) thread B calls back into A; A will run at prio 100
for the duration of the transaction
4) When thread A is done with the call from B, we will
try to restore the prio back to 98. But, we fail
because the process doesn't hold CAP_SYS_NICE,
neither is RLIMIT_RT_PRIO set.
While the proper fix going forward will be to
correctly apply CAP_SYS_NICE or RLIMIT_RT_PRIO,
for now it seems reasonable to not check permissions
on the restore path.
Bug: 62043063
Test: boots
Change-Id: Ibede5960c9b7bb786271c001e405de50be64d944
Signed-off-by: Martijn Coenen <maco@google.com>
A race existed where one thread could register
a death notification for a node, while another
thread was cleaning up that node and sending
out death notifications for its references,
causing simultaneous access to ref->death
because different locks were held.
This changes binder_node_release to hold
the proc lock of the reference while accessing
ref->death. Since this requires dropping the
lock of the node proc, we have to move all
node references to a temporarily list, in
order to safely process all references.
Test: boots, manual testing
Bug: 62139399
Change-Id: Iff73312f34f70374f417beba4c4c82dd33cac119
Signed-off-by: Martijn Coenen <maco@google.com>
RB nodes are not being initialized which means that
node->__rb_parent_color is initialized to NULL due to
kzalloc instead of being initialized as:
node->__rb_parent_color = node
This has not caused problems in the past, but recently
resulted in regular crashes
Bug: 62193133
Test: tested manually
Signed-off-by: Todd Kjos <tkjos@google.com>
Change-Id: I55c91196b85e614809bdb1a3f00bd121a03677c4
A BUG_ON was triggered when a thread initiating a transaction
to a dying process called binder_alloc_get_user_buffer_offset()
after alloc->vma has been set to NULL. Removed the BUG_ON since
it is possible for the target proc to die while the transaction
is being initiated. Ultimately, the transaction reply will be
a BR_DEAD_REPLY failure.
Bug: 38513317
Test: tested manually
Change-Id: Id116b366777adab4233bce87bd20dc3f543a9625
Signed-off-by: Todd Kjos <tkjos@google.com>
binder_mmap is called with mmap_sem held. binder_alloc_mmap_handler
acquires alloc->mutex so the order is mmap_sem --> alloc->mutex.
In other binder_alloc functions, the mmap_sem is acquired with
alloc->mutex held which could lead to a deadlock (though in practice
it may be impossible to hit because mmap runs once for binder
process and the other paths can't be reached unless mmap has run).
Bug: 38397347
Test: tested manually
Change-Id: I1dd926bcc25980301dfc42fa5d830fc05a2efef9
Signed-off-by: Todd Kjos <tkjos@google.com>
The log->next index for the transaction log was
not protected when incremented. This led to a
case where log->next++ resulted in an index
larger than ARRAY_SIZE(log->entry) and eventually
a bad access to memory.
Fixed by making the log index an atomic64 and
converting to an array by using "% ARRAY_SIZE(log->entry)"
Bug: 62038227
Test: tested manually
Change-Id: I1bb1c1a332a6ac458a626f5bedd05022b56b91f2
Signed-off-by: Todd Kjos <tkjos@google.com>
By selecting a thread to handle the oneway transaction
and raise its priority *before* we do the wake-up.
Since the oneway path is mostly similar to the synchronous
path here, split the work out into a separate function.
Bug: 34461621
Bug: 37293077
Change-Id: I50a67b9a1ff7587dc7005020234197ea07f483da
Signed-off-by: Martijn Coenen <maco@google.com>
This change adds flags to flat_binder_object.flags
to allow indicating a minimum scheduling policy for
the node. It also clarifies the valid value range
for the priority bits in the flags.
Internally, we use the priority map that the kernel
uses, e.g. [0..99] for real-time policies and [100..139]
for the SCHED_NORMAL/SCHED_BATCH policies.
We also need to start keeping track of the default
scheduling policy for a process, as that is what
we will restore to after handling oneway transactions
that temporarily increased the priority.
Bug: 34461621
Bug: 37293077
Change-Id: Ifc6a0d691c2feb48e8349a21f56fb2eeb22f1bb5
Signed-off-by: Martijn Coenen <maco@google.com>
Clean up priority inheritance code, and make sure that
we check whether we can set real-time priorities (and
fall back to the lowest possible nice if we can't).
This also makes the inheritance code consistent between
nice and scheduling policy: for synchronous transactions, we
can inherit the scheduling policy of the caller even if
it is a "lower" one.
Bug: 34461621
Change-Id: If808e1dee902947c40223778999da278cc469aa8
Signed-off-by: Martijn Coenen <maco@google.com>
When printing transactions there were several race conditions
that could cause a stale pointer to be deferenced. Fixed by
reading the pointer once and using it if valid (which is
safe). The transaction buffer also needed protection via proc
lock, so it is only printed if we are holding the correct lock.
Also adds protection against malicious user code from creating
two threads to free the same buffer at the same time.
Bug: 36650912
Test: tested manually
Change-Id: I78240f99cc1a070d70a841c0d84d4306e2fd528d
Signed-off-by: Todd Kjos <tkjos@google.com>
Adds protection against malicious user code freeing
the same buffer at the same time which could cause
a crash. Cannot happen under normal use.
Bug: 36650912
Change-Id: I43e078cbf31c0789aaff5ceaf8f1a94c75f79d45
Test: tested manually
Signed-off-by: Todd Kjos <tkjos@google.com>
Since errors are tracked in the return_error/return_error2
fields of the binder_thread object and BR_TRANSACTION_COMPLETEs
can be tracked either in those fields or via the thread todo
work list, it is possible for errors to be reported ahead
of the associated txn complete.
Use the thread todo work list for errors to guarantee
order.
Bug: 37218618
Test: tested manually
Change-Id: I196cfaeed09fdcd697f8ab25eea4e04241fdb08f
Signed-off-by: Todd Kjos <tkjos@google.com>
By setting the priority of a new incoming transaction
before waking up the thread that will handle it. Note
that this change doesn't affect the case where no
thread is available to handle work; we will fall
back to the old behavior in that case.
This change also delays restoring the priority in the
return path: it only restores the priority after the
original caller has been woken up.
Change-Id: I94034a28464ccd3ee16856d817497db1f4fb8e21
Signed-off-by: Martijn Coenen <maco@google.com>
Instead of pushing new synchronous transactions
to the process waitqueue, select a thread that is
waiting on proc work to handle the transaction.
This will make it easier to do proper priority
inheritance.
If we can't find a waiting thread, submit the work
to the proc waitqueue instead as we did previously.
Change-Id: Ib9385ba2c3e8b1e6d2106f1fce5b60bb117ec308
Signed-off-by: Martijn Coenen <maco@google.com>
Removes the process waitqueue, so that threads
can only wait on the thread waitqueue. Whenever
there is process work to do, pick a thread and
wake it up.
This also fixes an issue with using epoll(),
since we no longer have to block on different
waitqueues.
Bug: 34461621
Change-Id: I7d928de04f9f32168ed60dc1842af6eed9d308d8
Signed-off-by: Martijn Coenen <maco@google.com>
A code review found a case where thread->transaction_stack is not
protected by the proc lock and should be serialized with
a todo list check. This can result in a thread waiting for
proc work when there is thread work to do. This has a workaround
in the code, which can now be reverted. Also found missing protection
for thread->return_error.
Added proc lock protection for transaction stack and for cases where
return_error needs to be serialized.
Bug: 36792545
Change-Id: I848ae994cc27b3cd083cff2dbd1071762784f4a3
Test: tested manually
Signed-off-by: Todd Kjos <tkjos@google.com>
A recent change disallowed new refs to zombie nodes, however
it turns out this is overly restrictive. Change to allow
new refs to zombie nodes if there is at least 1 other ref.
Change-Id: I470dcfcba84c716931a860931af3a61ab9dc8359
Test: verfied fix for gmscore crash loop case
Bug: 36406078
Signed-off-by: Todd Kjos <tkjos@google.com>
Add a new ioctl to binder to control whether FIFO inheritance should happen.
In particular, hwbinder should inherit FIFO priority from callers, but standard
binder threads should not.
Test: boots
bug 36516194
Signed-off-by: Tim Murray <timmurray@google.com>
Change-Id: I8100c4364b7d15d1bf00a8ca5c286e4d4b23ce85
Add additional information to determine the cause of binder
failures. Adds the following to failed transaction log and
kernel messages:
return_error : value returned for transaction
return_error_param : errno returned by binder allocator
return_error_line : line number where error detected
Bug: 36406078
Change-Id: Ifc8881fa5adfcced3f2d67f9030fbd3efa3e2cab
Test: tested manually
Signed-off-by: Todd Kjos <tkjos@google.com>
Fix possible deadlock:
1. In most binder_alloc paths, the binder_alloc mutex is acquired before
calling into mm functions which can acquire the mmap_sem
2. During address space teardown, the mm system acquires mmap_sem which
can call into binder_alloc_vma_close which acquired the binder_alloc
mutex
Since they are acquired in opposite order, a thread doing #1 can
deadlock with a thread doing #2. There are no known cases where
this was seen, but it is possible.
The binder alloc mutex doesn't need to be acquired in the vma_close
path.
Bug: 36524239
Test: tested manually
Change-Id: I40b077fc3bc01e37b389043f2966257797ee9ce5
Signed-off-by: Todd Kjos <tkjos@google.com>
binder_enqueue_work asserts that the object is not
already queued by testing if work->wlist != NULL. The
assertion is outside the critical section, so it is
possible that the assertion can trip when the work
item is off the queue, but before work->wlist is
cleared (as was the case in b/36511858). Move the
assertion into the critical section so wlist check
is atomic with the actual list.
Bug: 36511858
Change-Id: I4d65e5abaa8a4bb0e3c122869ca8cca0991b83ed
Test: tested manually
Signed-off-by: Todd Kjos <tkjos@google.com>
The binder driver bumps the local_strong_refs while it is
using the associated node. This needs to change to local_weak_refs
to avoid inadvertantly causing the node to be destroyed in
userspace.
Bug: 36093037
Change-Id: Id085851407b6ea2a99affddd27ae4d9908149847
Test: tested manually
Signed-off-by: Todd Kjos <tkjos@google.com>
In binder_thread_read, the BINDER_WORK_NODE command is used
to communicate the references on the node to userspace. It
often takes a couple of iterations in the loop to construct
the list of commands for user space. During the loop, the
proc lock is released which means state can change. The
case found in the bug was a inc-after-dec where the node
briefly had 0 references and this was reported to userspace,
but before deleting it in the kernel, a new reference was
taken. Unfortunately, the node was destroyed in userspace
so when the inc was reported to userspace there was a crash.
Fixed by changing the algorithm in binder_thread_read to
determine which commands to send to userspace atomically
so it stays consistent with the kernel view. If it is the
last reference, then the node becomes a zombie when the
userspace command will cause node object destruction.
Bug: 36093037
Change-Id: I02c5cfdd3ab6dadea7f07f2a275faf3e27be77ad
Test: tested manually
Signed-off-by: Todd Kjos <tkjos@google.com>
The check for zombies was being done before binder_put_thread
which meant that any object that became a zombie could not
be handled right away even if safe. Also, need to trigger
zombie handling when deferring put_files_struct().
This manifested as long delays in adb completing a command
because the files_struct stayed alive longer than it should
have.
Bug: 36358874
Change-Id: Ib8f8b6cd3ea583342b9f39f1bab71a7948ec4bac
Test: tested and confirmed by bug submitter
Signed-off-by: Todd Kjos <tkjos@google.com>
Death notifications weren't protected against being
enqueued simultaneously by two threads. Serialize
with proc lock.
Bug: 36371104
Test: tested manually
Change-Id: Ie0f04985b765c8dc1a69362f1609aa373806fb4c
Signed-off-by: Todd Kjos <tkjos@google.com>
Signed-off-by: Siqi Lin <siqilin@google.com>
Objects are reaped and kfree'd when we know that all threads
that were active when the object became a zombie have left
the kernel. The loop that checks this was reading the active
thread sequence number at the beginning and caching it. However,
if in the 1st iteration there are no threads, it will cache ~0ULL
and all zombies will be reaped even if new ones arrive and need
to be protected. Check the sequence number on each iteration.
Bug: 36220321
Change-Id: I694ff29800cf4ab8d37cb3bb1d5508a525dd88da
Test: tested manually
Signed-off-by: Todd Kjos <tkjos@google.com>
Signed-off-by: Siqi Lin <siqilin@google.com>
We are using an atomic_t for the active thread sequence counter
which could rollover and possibly result in premature reaping of
zombie objects. Change to atomic64_t.
Bug: 36072376
Change-Id: I6f5d3f2b9e81e4f1cab3fa330299c25bee4fb9ef
Test: tested manually
Signed-off-by: Todd Kjos <tkjos@google.com>
Signed-off-by: Siqi Lin <siqilin@google.com>
There was a race condition that allowed a new ref
to be associated with a zombie node. Normally this
is ok as long as the ref in the node's refs list,
however, the new ref was being added to the node
after being added to the proc's rb_tree which meant
the node could have no refs on the list and could
therefore be reaped (kfree'd) when a thread
looks up the ref. Fixed by modifying binder_ref
create delete to guarantee that it is in the node
refs list if it can be found by other threads.
Bug: 36072376
Test: tested manually
Change-Id: I3e73c890300f9d8078f92223186084ca81933cb7
Signed-off-by: Todd Kjos <tkjos@google.com>
Signed-off-by: Siqi Lin <siqilin@google.com>
To keep the driver consistent, and until we have
fine-grained locking in place.
Change-Id: Idda7ae8df889b5fae5e96bf343ab17782b4c46b1
Signed-off-by: Martijn Coenen <maco@android.com>
Git-repo: https://android.googlesource.com/kernel/common
Git-commit: 288f5164a4e434939ae92b86dae2768e57ca41ac
Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
The binder allocator assumes that the thread that
called binder_open will never die for the lifetime of
that proc. That thread is normally the group_leader,
however it may not be. Use the group_leader instead
of current.
Bug: 35707103
Test: Created test case to open with temporary thread
Change-Id: Id693f74b3591f3524a8c6e9508e70f3e5a80c588
Signed-off-by: Todd Kjos <tkjos@google.com>
If the source thread of a transaction runs with a real-time
policy such as SCHED_FIFO or SCHED_RR, have the thread handling
the transaction run with the same policy.
Change-Id: I5badf2bdc1702c83e20264b5c40e53a62d22ba61
Signed-off-by: Martijn Coenen <maco@android.com>
