Re: [PATCH] ipc/shm: check shm_lock() in do_shmat cleanup

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Tue, Jul 14, 2026 at 11:17:13AM +0800, Yi Xie wrote:
> shm_lock() can fail; don't dereference the error pointer.
>
> Signed-off-by: Yi Xie <xieyi@kylinos.cn>

Yikes yeah. Presumably this is pretty hard to hit, but seems legit.

> ---
>  ipc/shm.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/ipc/shm.c b/ipc/shm.c
> index b3e8a58e177d..bdcf1c0f221d 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -1677,6 +1677,10 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg,
>  out_nattch:
>  	down_write(&shm_ids(ns).rwsem);
>  	shp = shm_lock(ns, shmid);
> +	if (IS_ERR(shp)) {
> +		up_write(&shm_ids(ns).rwsem);
> +		return err;
> +	}

You're duplicating the ugly up_write() here. Probably better to do something like:

-       shp->shm_nattch--;
+       if (IS_ERR(shp)) {
+               err = PTR_ERR(shp);
+       } else {
+               shp->shm_nattch--;

-       if (shm_may_destroy(shp))
-               shm_destroy(ns, shp);
-       else
-               shm_unlock(shp);
+               if (shm_may_destroy(shp))
+                       shm_destroy(ns, shp);
+               else
+                       shm_unlock(shp);
+       }

And keep the up_write() shared.

>  	shp->shm_nattch--;
>
>  	if (shm_may_destroy(shp))
> --
> 2.25.1
>

Thanks, Lorenzo