[PATCH 1/1] ipc/shm: serialize orphan cleanup with shm_nattch updates

[PATCH 1/1] ipc/shm: serialize orphan cleanup with shm_nattch updates

[Ren Wei]

From: Yilin Zhu <zylzyl2333@gmail.com>

shm_destroy_orphaned() walks the shm idr under shm_ids(ns).rwsem, but
that does not serialize all fields tested by shm_may_destroy().  In
particular, shm_nattch is updated while holding shm_perm.lock, and attach
paths can do that without holding the rwsem.

Do not decide that an orphaned segment is unused before taking the object
lock.  Move the shm_may_destroy() check under shm_perm.lock, matching the
other destroy paths, and unlock the segment when it no longer qualifies
for removal.

Fixes: 4c677e2eefdb ("shm: optimize locking and ipc_namespace getting")
Cc: stable@kernel.org
Reported-by: Yuan Tan <yuantan098@gmail.com>
Reported-by: Yifan Wu <yifanwucs@gmail.com>
Reported-by: Juefei Pu <tomapufckgml@gmail.com>
Reported-by: Xin Liu <bird@lzu.edu.cn>
Signed-off-by: Yilin Zhu <zylzyl2333@gmail.com>
Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
---
 ipc/shm.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/ipc/shm.c b/ipc/shm.c
index a95dae447707..b3e8a58e177d 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -418,15 +418,17 @@ static int shm_try_destroy_orphaned(int id, void *p, void *data)
 	 * We want to destroy segments without users and with already
 	 * exit'ed originating process.
 	 *
-	 * As shp->* are changed under rwsem, it's safe to skip shp locking.
+	 * shm_nattch can be changed under shm_perm.lock without holding the
+	 * rwsem, so take the object lock before checking shm_may_destroy().
 	 */
 	if (!list_empty(&shp->shm_clist))
 		return 0;
 
-	if (shm_may_destroy(shp)) {
-		shm_lock_by_ptr(shp);
+	shm_lock_by_ptr(shp);
+	if (shm_may_destroy(shp))
 		shm_destroy(ns, shp);
-	}
+	else
+		shm_unlock(shp);
 	return 0;
 }
 
-- 
2.47.3

Re: [PATCH 1/1] ipc/shm: serialize orphan cleanup with shm_nattch updates

[Andrew Morton]

On Thu, 30 Apr 2026 13:21:34 +0800 Ren Wei <n05ec@lzu.edu.cn> wrote:

> From: Yilin Zhu <zylzyl2333@gmail.com>
> 
> shm_destroy_orphaned() walks the shm idr under shm_ids(ns).rwsem, but
> that does not serialize all fields tested by shm_may_destroy().  In
> particular, shm_nattch is updated while holding shm_perm.lock, and attach
> paths can do that without holding the rwsem.
> 
> Do not decide that an orphaned segment is unused before taking the object
> lock.  Move the shm_may_destroy() check under shm_perm.lock, matching the
> other destroy paths, and unlock the segment when it no longer qualifies
> for removal.

Thanks.

> Fixes: 4c677e2eefdb ("shm: optimize locking and ipc_namespace getting")

Let's cc more people who were involved in 4c677e2eefdb.

And Davidlohr, who might have opinions.

> Cc: stable@kernel.org
> Reported-by: Yuan Tan <yuantan098@gmail.com>
> Reported-by: Yifan Wu <yifanwucs@gmail.com>
> Reported-by: Juefei Pu <tomapufckgml@gmail.com>
> Reported-by: Xin Liu <bird@lzu.edu.cn>
> Signed-off-by: Yilin Zhu <zylzyl2333@gmail.com>
> Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
> ---
>  ipc/shm.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/ipc/shm.c b/ipc/shm.c
> index a95dae447707..b3e8a58e177d 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -418,15 +418,17 @@ static int shm_try_destroy_orphaned(int id, void *p, void *data)
>  	 * We want to destroy segments without users and with already
>  	 * exit'ed originating process.
>  	 *
> -	 * As shp->* are changed under rwsem, it's safe to skip shp locking.
> +	 * shm_nattch can be changed under shm_perm.lock without holding the
> +	 * rwsem, so take the object lock before checking shm_may_destroy().
>  	 */
>  	if (!list_empty(&shp->shm_clist))
>  		return 0;
>  
> -	if (shm_may_destroy(shp)) {
> -		shm_lock_by_ptr(shp);
> +	shm_lock_by_ptr(shp);
> +	if (shm_may_destroy(shp))
>  		shm_destroy(ns, shp);
> -	}
> +	else
> +		shm_unlock(shp);
>  	return 0;
>  }

Re: [PATCH 1/1] ipc/shm: serialize orphan cleanup with shm_nattch updates

[Davidlohr Bueso]

On Thu, 30 Apr 2026, Andrew Morton wrote:

>On Thu, 30 Apr 2026 13:21:34 +0800 Ren Wei <n05ec@lzu.edu.cn> wrote:
>
>> From: Yilin Zhu <zylzyl2333@gmail.com>
>>
>> shm_destroy_orphaned() walks the shm idr under shm_ids(ns).rwsem, but
>> that does not serialize all fields tested by shm_may_destroy().  In
>> particular, shm_nattch is updated while holding shm_perm.lock, and attach
>> paths can do that without holding the rwsem.
>>
>> Do not decide that an orphaned segment is unused before taking the object
>> lock.  Move the shm_may_destroy() check under shm_perm.lock, matching the
>> other destroy paths, and unlock the segment when it no longer qualifies
>> for removal.
>
>Thanks.
>
>> Fixes: 4c677e2eefdb ("shm: optimize locking and ipc_namespace getting")
>
>Let's cc more people who were involved in 4c677e2eefdb.
>
>And Davidlohr, who might have opinions.

The same feedback as the previous (duplicate) patch - there is no need to
unconditionally take the lock.

https://lore.kernel.org/all/20260406193322.cu2rrtfmpixlv7yy@offworld/

Thanks,
Davidlohr