Hello together,
On 11/5/21 22:34, Eric W. Biederman wrote:
I have to dash so this is short.
This is what I am thinking this change should look like.
I am not certain this is truly reviewable as a single change, so I will break it into a couple of smaller ones next time I get the chance.
I think we should concentrate to check the commit from Alexander.
What I did is to write two additional stress test apps - and now I'm able to trigger the use-after-free bug.
It is much simpler, the exclusion of exit_shm() and IPC_RMID didn't work - regardless if your approach or the approach from Alexander/myself is used.
+static inline void shm_clist_del(struct shmid_kernel *shp) +{
- struct task_struct *creator;
- rcu_read_lock();
- creator = rcu_dereference(shp->shm_creator);
- if (creator) {
task_lock(creator);list_del(&shp->shm_clist);task_unlock(creator);- }
- rcu_read_unlock();
+}
shm_clist_del() only synchronizes against exit_shm() when shm_creator is not NULL.
list_del(&shp->shm_clist);rcu_assign_pointer(shp->shm_creator, NULL);
We set shm_creator to NULL -> no more synchronization.
Now IPC_RMID can run in parallel - regardless if we test for list_empty() or shm_creator.
/* Guarantee shp lives after task_lock is dropped */ipc_getref(&shp->shm_perm);
task_lock() doesn't help: As soon as shm_creator is set to NULL, IPC_RMID won't acquire task_lock() anymore.
Thus shp can disappear before we arrive at this ipc_getref.
[Yes, I think I have introduced this bug. ]
Corrected version attached.
I'll reboot and retest the patch, then I would send it to akpm as replacement for current patch in mmotm.
--
Manfred