Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
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, edgar.kaziakhmedov@virtuozzo.com,
        philippe.mikoyan@skat.systems
Subject: [PATCH 2/2] ipc: Fix ipc data structures inconsistency
Date:   Thu, 30 Nov 2017 09:12:24 +0300
Message-Id: <20171130061224.25466-3-philippe.mikoyan@skat.systems>
In-Reply-To: <20171130061224.25466-1-philippe.mikoyan@skat.systems>
References: <20171130061224.25466-1-philippe.mikoyan@skat.systems>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

As described in the title, this patch fixes <ipc>id_ds inconsistency
when <ipc>ctl_stat runs 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>
---
 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..8d3c3946c825 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 operatoins 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


From 1585687138664805261@xxx Sat Dec 02 15:38:18 +0000 2017
X-GM-THRID: 1585323514263946873
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread