From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751943AbdLBPfX (ORCPT <rfc822;w@1wt.eu>);
        Sat, 2 Dec 2017 10:35:23 -0500
Received: from relay-out6.mail.masterhost.ru ([83.222.12.16]:45643 "EHLO
        relay-out6.mail.masterhost.ru" rhost-flags-OK-OK-OK-OK)
        by vger.kernel.org with ESMTP id S1751747AbdLBPfV (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Sat, 2 Dec 2017 10:35:21 -0500
From: Philippe Mikoyan <philippe.mikoyan@skat.systems>
To: akpm@linux-foundation.org
Cc: viro@zeniv.linux.org.uk, manfred@colorfullife.com,
        linux-kernel@vger.kernel.org, dave@stgolabs.net,
        philippe.mikoyan@skat.systems
Subject: [PATCH v2] ipc: Fix ipc data structures inconsistency
Date: Sat,  2 Dec 2017 18:34:56 +0300
Message-Id: <20171202153456.6514-1-philippe.mikoyan@skat.systems>
X-Mailer: git-send-email 2.11.0
X-KLMS-Rule-ID: 1
X-KLMS-Message-Action: clean
X-KLMS-AntiSpam-Lua-Profiles: 119397 [Dec 02 2017]
X-KLMS-AntiSpam-Version: 5.7.67.0
X-KLMS-AntiSpam-Envelope-From: philippe.mikoyan@skat.systems
X-KLMS-AntiSpam-Rate: 0
X-KLMS-AntiSpam-Status: not_detected
X-KLMS-AntiSpam-Method: none
X-KLMS-AntiSpam-Info: LuaCore: 93 93 df0a2856d02e86f3280f253179d00609db7f0d5d, Auth:dkim=none, {DNS response errors}
X-KLMS-AntiSpam-Interceptor-Info: scan successful
X-KLMS-AntiPhishing: not scanned, disabled by settings
X-KLMS-AntiVirus: Kaspersky Security 8.0 for Linux Mail Server, version 8.0.1.721, not scanned, license restriction
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

As described in the title, this patch fixes <ipc>id_ds
inconsistency when <ipc>ctl_stat executes concurrently
with some ds-changing function, e.g. shmat, msgsnd or
whatever.

For instance, if shmctl(IPC_STAT) is running concurrently
with shmat, following data structure can be returned:
{... shm_lpid = 0, shm_nattch = 1, ...}

Signed-off-by: Philippe Mikoyan <philippe.mikoyan@skat.systems>
Reviewed-by: Davidlohr Bueso <dbueso@suse.de>
---
Changes in v2:
    - Fixed 'operatoins' typo in util.c

 ipc/msg.c  | 20 ++++++++++++++------
 ipc/sem.c  | 10 ++++++++++
 ipc/shm.c  | 19 ++++++++++++++-----
 ipc/util.c |  5 ++++-
 4 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/ipc/msg.c b/ipc/msg.c
index 06be5a9adfa4..047579b42de4 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -475,9 +475,9 @@ static int msgctl_info(struct ipc_namespace *ns, int msqid,
 static int msgctl_stat(struct ipc_namespace *ns, int msqid,
 			 int cmd, struct msqid64_ds *p)
 {
-	int err;
 	struct msg_queue *msq;
-	int success_return;
+	int id = 0;
+	int err;

 	memset(p, 0, sizeof(*p));

@@ -488,14 +488,13 @@ static int msgctl_stat(struct ipc_namespace *ns, int msqid,
 			err = PTR_ERR(msq);
 			goto out_unlock;
 		}
-		success_return = msq->q_perm.id;
+		id = msq->q_perm.id;
 	} else {
 		msq = msq_obtain_object_check(ns, msqid);
 		if (IS_ERR(msq)) {
 			err = PTR_ERR(msq);
 			goto out_unlock;
 		}
-		success_return = 0;
 	}

 	err = -EACCES;
@@ -506,6 +505,14 @@ static int msgctl_stat(struct ipc_namespace *ns, int msqid,
 	if (err)
 		goto out_unlock;

+	ipc_lock_object(&msq->q_perm);
+
+	if (!ipc_valid_object(&msq->q_perm)) {
+		ipc_unlock_object(&msq->q_perm);
+		err = -EIDRM;
+		goto out_unlock;
+	}
+
 	kernel_to_ipc64_perm(&msq->q_perm, &p->msg_perm);
 	p->msg_stime  = msq->q_stime;
 	p->msg_rtime  = msq->q_rtime;
@@ -515,9 +522,10 @@ static int msgctl_stat(struct ipc_namespace *ns, int msqid,
 	p->msg_qbytes = msq->q_qbytes;
 	p->msg_lspid  = msq->q_lspid;
 	p->msg_lrpid  = msq->q_lrpid;
-	rcu_read_unlock();

-	return success_return;
+	ipc_unlock_object(&msq->q_perm);
+	rcu_read_unlock();
+	return id;

 out_unlock:
 	rcu_read_unlock();
diff --git a/ipc/sem.c b/ipc/sem.c
index f7385bce5fd3..9b6f80d1b3f1 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1211,10 +1211,20 @@ static int semctl_stat(struct ipc_namespace *ns, int semid,
 	if (err)
 		goto out_unlock;

+	ipc_lock_object(&sma->sem_perm);
+
+	if (!ipc_valid_object(&sma->sem_perm)) {
+		ipc_unlock_object(&sma->sem_perm);
+		err = -EIDRM;
+		goto out_unlock;
+	}
+
 	kernel_to_ipc64_perm(&sma->sem_perm, &semid64->sem_perm);
 	semid64->sem_otime = get_semotime(sma);
 	semid64->sem_ctime = sma->sem_ctime;
 	semid64->sem_nsems = sma->sem_nsems;
+
+	ipc_unlock_object(&sma->sem_perm);
 	rcu_read_unlock();
 	return id;

diff --git a/ipc/shm.c b/ipc/shm.c
index 565f17925128..8f58faba7429 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -896,9 +896,11 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid,
 			int cmd, struct shmid64_ds *tbuf)
 {
 	struct shmid_kernel *shp;
-	int result;
+	int id = 0;
 	int err;

+	memset(tbuf, 0, sizeof(*tbuf));
+
 	rcu_read_lock();
 	if (cmd == SHM_STAT) {
 		shp = shm_obtain_object(ns, shmid);
@@ -906,14 +908,13 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid,
 			err = PTR_ERR(shp);
 			goto out_unlock;
 		}
-		result = shp->shm_perm.id;
+		id = shp->shm_perm.id;
 	} else {
 		shp = shm_obtain_object_check(ns, shmid);
 		if (IS_ERR(shp)) {
 			err = PTR_ERR(shp);
 			goto out_unlock;
 		}
-		result = 0;
 	}

 	err = -EACCES;
@@ -924,7 +925,14 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid,
 	if (err)
 		goto out_unlock;

-	memset(tbuf, 0, sizeof(*tbuf));
+	ipc_lock_object(&shp->shm_perm);
+
+	if (!ipc_valid_object(&shp->shm_perm)) {
+		ipc_unlock_object(&shp->shm_perm);
+		err = -EIDRM;
+		goto out_unlock;
+	}
+
 	kernel_to_ipc64_perm(&shp->shm_perm, &tbuf->shm_perm);
 	tbuf->shm_segsz	= shp->shm_segsz;
 	tbuf->shm_atime	= shp->shm_atim;
@@ -934,8 +942,9 @@ static int shmctl_stat(struct ipc_namespace *ns, int shmid,
 	tbuf->shm_lpid	= shp->shm_lprid;
 	tbuf->shm_nattch = shp->shm_nattch;

+	ipc_unlock_object(&shp->shm_perm);
 	rcu_read_unlock();
-	return result;
+	return id;

 out_unlock:
 	rcu_read_unlock();
diff --git a/ipc/util.c b/ipc/util.c
index 78755873cc5b..9736cd66b4cc 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -22,9 +22,12 @@
  *	    tree.
  *	    - perform initial checks (capabilities, auditing and permission,
  *	      etc).
- *	    - perform read-only operations, such as STAT, INFO commands.
+ *	    - perform read-only operations, such as INFO command, that
+ *	      do not demand atomicity
  *	      acquire the ipc lock (kern_ipc_perm.lock) through
  *	      ipc_lock_object()
+ *		- perform read-only operations that demand atomicity,
+ *		  such as STAT command.
  *		- perform data updates, such as SET, RMID commands and
  *		  mechanism-specific operations (semop/semtimedop,
  *		  msgsnd/msgrcv, shmat/shmdt).
--
2.11.0