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@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>