Re: [PATCH, RESEND] ipc/shm: handle removed segments gracefully in shm_mmap()

["Kirill A. Shutemov" <kirill@shutemov.name>]

On Sat, Jan 02, 2016 at 12:45:07PM +0100, Manfred Spraul wrote:
> On 11/13/2015 08:23 PM, Davidlohr Bueso wrote:
> >
> >So considering EINVAL, even your approach to bumping up nattach by calling
> >_shm_open earlier isn't enough. Races exposed to user called rmid can
> >still
> >occur between dropping the lock and doing ->mmap(). Ultimately this leads
> >to
> >all ipc_valid_object() checks, as we totally ignore SHM_DEST segments
> >nowadays
> >since we forbid mapping previously removed segments.
> >
> >I think this is the first thing we must decide before going forward with
> >this
> >mess. ipc currently defines invalid objects by merely checking the deleted
> >flag.
> >
> >Manfred, any thoughts?
> >
> With regards to locking: Sorry, shm is too different to msg/sem/mqueue.
> 
> With regards to EIDRM / EINVAL:
> When all kernel memory was released, then the kernel cannot find out if the
> ID was valid at one time or not.
> Thus EIDRM can only be a hint, the OS (kernel/libc) cannot guarantee that
> user space will never see something else.
> (trivial example: user space sleeps just before the syscall)
> 
> So I would not create special code to optimize EIDRM handling for races. If
> we sometimes report EINVAL, it would be probably ok as well.

Guys, here's yet another attempt to fix the issue.

The key idea this time is to use shm_ids(ns).rwsem taken for read in shm_mmap()
to prevent rmid under us.

Any problem with this?

diff --git a/ipc/shm.c b/ipc/shm.c
index ed3027d0f277..b306fb3d9586 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -156,11 +156,12 @@ static inline struct shmid_kernel *shm_lock(struct ipc_namespace *ns, int id)
 	struct kern_ipc_perm *ipcp = ipc_lock(&shm_ids(ns), id);
 
 	/*
-	 * We raced in the idr lookup or with shm_destroy().  Either way, the
-	 * ID is busted.
+	 * Callers of shm_lock() must validate the status of the returned ipc
+	 * object pointer (as returned by ipc_lock()), and error out as
+	 * appropriate.
 	 */
-	WARN_ON(IS_ERR(ipcp));
-
+	if (IS_ERR(ipcp))
+		return (void *)ipcp;
 	return container_of(ipcp, struct shmid_kernel, shm_perm);
 }
 
@@ -194,6 +195,14 @@ static void shm_open(struct vm_area_struct *vma)
 	struct shmid_kernel *shp;
 
 	shp = shm_lock(sfd->ns, sfd->id);
+
+	/*
+	 * We raced in the idr lookup or with shm_destroy().
+	 * Either way, the ID is busted.
+	 */
+	if (WARN_ON(IS_ERR(shp)))
+		return ;
+
 	shp->shm_atim = get_seconds();
 	shp->shm_lprid = task_tgid_vnr(current);
 	shp->shm_nattch++;
@@ -386,18 +395,34 @@ static struct mempolicy *shm_get_policy(struct vm_area_struct *vma,
 static int shm_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	struct shm_file_data *sfd = shm_file_data(file);
+	struct shmid_kernel *shp;
 	int ret;
 
+	/* Prevent rmid under us */
+	down_read(&shm_ids(sfd->ns).rwsem);
+
+	/* Check if we can map the segment */
+	shp = shm_lock(sfd->ns, sfd->id);
+	if (IS_ERR(shp)) {
+		ret = PTR_ERR(shp);
+		goto out;
+	}
+	ret = shp->shm_perm.mode & SHM_DEST ? -EINVAL : 0;
+	shm_unlock(shp);
+	if (ret)
+		goto out;
+
 	ret = sfd->file->f_op->mmap(sfd->file, vma);
 	if (ret != 0)
-		return ret;
+		goto out;
 	sfd->vm_ops = vma->vm_ops;
 #ifdef CONFIG_MMU
 	WARN_ON(!sfd->vm_ops->fault);
 #endif
 	vma->vm_ops = &shm_vm_ops;
 	shm_open(vma);
-
+out:
+	up_read(&shm_ids(sfd->ns).rwsem);
 	return ret;
 }
 
-- 
 Kirill A. Shutemov

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>