Re: [PATCH] ipc: fix to protect IPCS lookups using RCU

[Matthew Wilcox <willy@infradead.org>]

On Wed, Feb 19, 2025 at 10:29:05PM +0900, Jeongjun Park wrote:
> In shm_destroy_orphaned(), we are not performing updates to the IPCS and are
> only calling idr_for_each(), which can be protected by the RCU read-critical
> section.

Well, no, that's not true.  The IPCS is updated by the callback passed
to idr_for_each().

> And if idr_for_each() is not protected by the RCU read-critical section,
> then when radix_tree_node_free() is called to free the struct radix_tree_node
> through call_rcu(), the node will be freed immediately, and when reading the
> next node in radix_tree_for_each_slot(), the memory that has already been
> freed may be read.
> 
> Therefore, when calling idr_for_each() in shm_destroy_orphaned(), it should
> be modify to protect it within the RCU read critical section.

This is a very complicated way of not describing what the problem is.
How about:

Holding the rwsem is insufficient to protect the IDR from concurrent
modification.  That allows radix tree nodes to be freed while we are
iterating the IDR.  We can prevent this by holding the RCU read lock
in addition to the rwsem.

(a really good commit message would explain why holding the rwsem is
insufficient, but the way ipc uses the IDR and RCU is very complicated,
and I don't remember)

> Reported-by: syzbot+a2b84e569d06ca3a949c@syzkaller.appspotmail.com
> Fixes: b34a6b1da371 ("ipc: introduce shm_rmid_forced sysctl")
> Signed-off-by: Jeongjun Park <aha310510@gmail.com>
> ---
>  ipc/shm.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 99564c870084..baef5afadfb9 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -431,8 +431,10 @@ static int shm_try_destroy_orphaned(int id, void *p, void *data)
>  void shm_destroy_orphaned(struct ipc_namespace *ns)
>  {
>  	down_write(&shm_ids(ns).rwsem);
> +	rcu_read_lock();
>  	if (shm_ids(ns).in_use)
>  		idr_for_each(&shm_ids(ns).ipcs_idr, &shm_try_destroy_orphaned, ns);
> +	rcu_read_unlock();
>  	up_write(&shm_ids(ns).rwsem);
>  }
>  
> --