lkml.org 
[lkml]   [2016]   [Jan]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH, RESEND] ipc/shm: handle removed segments gracefully in shm_mmap()
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

\
 
 \ /
  Last update: 2016-01-04 15:21    [W:0.063 / U:0.140 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site