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

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

[Yi Xie]

do_shmat() calls shm_lock() in the out_nattch branch and
immediately dereferences it, however shm_lock() can return an
error.

Check for an error and handle it if there is one.

Signed-off-by: Yi Xie <xieyi@kylinos.cn>
---
 ipc/shm.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/ipc/shm.c b/ipc/shm.c
index b3e8a58e177d..d243137c4dbd 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -1677,12 +1677,16 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg,
 out_nattch:
 	down_write(&shm_ids(ns).rwsem);
 	shp = shm_lock(ns, shmid);
-	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);
+	}
 	up_write(&shm_ids(ns).rwsem);
 	return err;
 
-- 
2.25.1

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

[Andrew Morton]

On Thu, 16 Jul 2026 15:53:30 +0800 Yi Xie <xieyi@kylinos.cn> wrote:

> do_shmat() calls shm_lock() in the out_nattch branch and
> immediately dereferences it, however shm_lock() can return an
> error.
> 
> Check for an error and handle it if there is one.

Thanks.

ipc/ doesn't have a listed maintainer, but Davidlohr usually helps out.

> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -1677,12 +1677,16 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg,
>  out_nattch:
>  	down_write(&shm_ids(ns).rwsem);
>  	shp = shm_lock(ns, shmid);
> -	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);
> +	}
>  	up_write(&shm_ids(ns).rwsem);
>  	return err;

Sashiko says that shm_lock() cannot actually fail in this situation.

	https://sashiko.dev/#/patchset/20260716075330.96378-1-xieyi@kylinos.cn

It's pretty horrid (and fragile) for calling code to "know" this is the
case, so I think it's good from a maintainability point of view to add
code to handle this cannot-happen case.  Or, better, to do some
refactoring to eliminate this situation without adding overhead.

If we decide to keep the code as-is then it definitely needs a code
comment explaining why we don't need to check the shm_lock() return
here.

Sashiko also suggests that the proposed error-return handling is inaccurate.

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

[Lorenzo Stoakes (ARM)]

On Thu, Jul 16, 2026 at 03:53:30PM +0800, Yi Xie wrote:
> do_shmat() calls shm_lock() in the out_nattch branch and
> immediately dereferences it, however shm_lock() can return an
> error.
>
> Check for an error and handle it if there is one.
>
> Signed-off-by: Yi Xie <xieyi@kylinos.cn>

LGTM, so:

Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>

> ---

Small note - it's useful to put a list of changes per version and links to the
previous versions on lore after the '---' for reviewers. Anything after the ---
and before the patch won't go upstream.

>  ipc/shm.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/ipc/shm.c b/ipc/shm.c
> index b3e8a58e177d..d243137c4dbd 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -1677,12 +1677,16 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg,
>  out_nattch:
>  	down_write(&shm_ids(ns).rwsem);
>  	shp = shm_lock(ns, shmid);
> -	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);
> +	}
>  	up_write(&shm_ids(ns).rwsem);
>  	return err;
>
> --
> 2.25.1
>

Cheers, Lorenzo