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.

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.

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);
 }
 
--

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);
>  }
>  
> --

* Jeongjun Park <aha310510@gmail.com> [250219 08:29]:

Thanks for looking at this.

Your subject should be v2.

> 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.
> 
> 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.

The way rcu and reference counting is used in shm is very odd.

> 
> Therefore, when calling idr_for_each() in shm_destroy_orphaned(), it should
> be modify to protect it within the RCU read critical section.

Did you test this with lockdep and prove rcu?

Taking the rcu lock has some extra constraints that may be violated in
the call chain.

> 
> Reported-by: syzbot+a2b84e569d06ca3a949c@syzkaller.appspotmail.com
> Fixes: b34a6b1da371 ("ipc: introduce shm_rmid_forced sysctl")
> Signed-off-by: Jeongjun Park <aha310510@gmail.com>

WBN to Cc: the person that looked at v1 for you (especially since he's
already Cc'ed on the email).

> ---
>  ipc/shm.c | 2 ++
>  1 file changed, 2 insertions(+)

Since this is a single patch without a cover letter (which is good), you
can put some stuff here that you don't want to have in the git log to be
used for review, such as:

v1 link
changes since v1


> 
> 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);
>  }
>  
> --