From: Andrew Morton <akpm@linux-foundation.org>
To: Dmitry Vyukov <dvyukov@google.com>
Cc: syzkaller <syzkaller@googlegroups.com>,
"Kirill A. Shutemov" <kirill@shutemov.name>,
Dave Hansen <dave.hansen@linux.intel.com>,
Hugh Dickins <hughd@google.com>, Joe Perches <joe@perches.com>,
sds@tycho.nsa.gov, Oleg Nesterov <oleg@redhat.com>,
"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
Rik van Riel <riel@redhat.com>,
mhocko@suse.cz, gang.chen.5i5j@gmail.com,
Peter Feiner <pfeiner@google.com>,
Andrea Arcangeli <aarcange@redhat.com>,
"linux-mm@kvack.org" <linux-mm@kvack.org>,
LKML <linux-kernel@vger.kernel.org>,
Kostya Serebryany <kcc@google.com>,
Alexander Potapenko <glider@google.com>,
Andrey Konovalov <andreyknvl@google.com>,
Sasha Levin <sasha.levin@oracle.com>,
Manfred Spraul <manfred@colorfullife.com>
Subject: Re: GPF in shm_lock ipc
Date: Mon, 1 Feb 2016 19:25:50 -0800 [thread overview]
Message-ID: <20160201192550.6da19ac1.akpm@linux-foundation.org> (raw)
In-Reply-To: <CACT4Y+bwixTW5YZjPsN7qgCbhR=HR=SMoZi9yHfBaFWdqDkoXQ@mail.gmail.com>
On Mon, 21 Dec 2015 16:44:34 +0100 Dmitry Vyukov <dvyukov@google.com> wrote:
> On Thu, Nov 5, 2015 at 3:23 PM, Kirill A. Shutemov
> <kirill.shutemov@linux.intel.com> wrote:
> > What about this:
>
>
> Ping. This is still happening for me on tip. Can we pull in this fix
> if it looks good to everybody?
>
Well we have at least three patches to choose from in this thread, most
of them missing most signs of having been reviewed or tested.
So I grabbed the last one, below. Can we please rev this up again,
test it, see if we can agree that this is the way to go forward?
Thanks.
From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Subject: ipc/shm: handle removed segments gracefully in shm_mmap()
remap_file_pages(2) emulation can reach file which represents removed IPC
ID as long as a memory segment is mapped. It breaks expectations of IPC
subsystem.
Test case (rewritten to be more human readable, originally autogenerated
by syzkaller[1]):
#define _GNU_SOURCE
#include <stdlib.h>
#include <sys/ipc.h>
#include <sys/mman.h>
#include <sys/shm.h>
#define PAGE_SIZE 4096
int main()
{
int id;
void *p;
id = shmget(IPC_PRIVATE, 3 * PAGE_SIZE, 0);
p = shmat(id, NULL, 0);
shmctl(id, IPC_RMID, NULL);
remap_file_pages(p, 3 * PAGE_SIZE, 0, 7, 0);
return 0;
}
The patch changes shm_mmap() and code around shm_lock() to propagate
locking error back to caller of shm_mmap().
[1] http://github.com/google/syzkaller
Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Manfred Spraul <manfred@colorfullife.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
ipc/shm.c | 53 ++++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 43 insertions(+), 10 deletions(-)
diff -puN ipc/shm.c~gpf-in-shm_lock-ipc ipc/shm.c
--- a/ipc/shm.c~gpf-in-shm_lock-ipc
+++ a/ipc/shm.c
@@ -156,11 +156,12 @@ static inline struct shmid_kernel *shm_l
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);
}
@@ -186,18 +187,33 @@ static inline void shm_rmid(struct ipc_n
}
-/* This is called by fork, once for every shm attach. */
-static void shm_open(struct vm_area_struct *vma)
+static int __shm_open(struct vm_area_struct *vma)
{
struct file *file = vma->vm_file;
struct shm_file_data *sfd = shm_file_data(file);
struct shmid_kernel *shp;
shp = shm_lock(sfd->ns, sfd->id);
+
+ if (IS_ERR(shp))
+ return PTR_ERR(shp);
+
shp->shm_atim = get_seconds();
shp->shm_lprid = task_tgid_vnr(current);
shp->shm_nattch++;
shm_unlock(shp);
+ return 0;
+}
+
+/* This is called by fork, once for every shm attach. */
+static void shm_open(struct vm_area_struct *vma)
+{
+ int err = __shm_open(vma);
+ /*
+ * We raced in the idr lookup or with shm_destroy().
+ * Either way, the ID is busted.
+ */
+ WARN_ON_ONCE(err);
}
/*
@@ -260,6 +276,14 @@ static void shm_close(struct vm_area_str
down_write(&shm_ids(ns).rwsem);
/* remove from the list of attaches of the shm segment */
shp = shm_lock(ns, sfd->id);
+
+ /*
+ * We raced in the idr lookup or with shm_destroy().
+ * Either way, the ID is busted.
+ */
+ if (WARN_ON_ONCE(IS_ERR(shp)))
+ goto done; /* no-op */
+
shp->shm_lprid = task_tgid_vnr(current);
shp->shm_dtim = get_seconds();
shp->shm_nattch--;
@@ -267,6 +291,7 @@ static void shm_close(struct vm_area_str
shm_destroy(ns, shp);
else
shm_unlock(shp);
+done:
up_write(&shm_ids(ns).rwsem);
}
@@ -388,17 +413,25 @@ static int shm_mmap(struct file *file, s
struct shm_file_data *sfd = shm_file_data(file);
int ret;
+ /*
+ * In case of remap_file_pages() emulation, the file can represent
+ * removed IPC ID: propogate shm_lock() error to caller.
+ */
+ ret =__shm_open(vma);
+ if (ret)
+ return ret;
+
ret = sfd->file->f_op->mmap(sfd->file, vma);
- if (ret != 0)
+ if (ret) {
+ shm_close(vma);
return ret;
+ }
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);
-
- return ret;
+ return 0;
}
static int shm_release(struct inode *ino, struct file *file)
_
next prev parent reply other threads:[~2016-02-02 3:23 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-10-12 9:55 GPF in shm_lock ipc Dmitry Vyukov
2015-10-12 11:41 ` Vlastimil Babka
2015-10-12 11:44 ` Dmitry Vyukov
2015-10-12 12:27 ` Kirill A. Shutemov
2015-10-12 17:49 ` Davidlohr Bueso
2015-10-12 18:10 ` Kirill A. Shutemov
2015-10-12 18:55 ` Davidlohr Bueso
2015-10-13 3:18 ` Davidlohr Bueso
2015-10-13 12:30 ` Kirill A. Shutemov
2015-10-29 15:33 ` Dmitry Vyukov
2015-11-05 14:23 ` Kirill A. Shutemov
2015-12-21 15:44 ` Dmitry Vyukov
2016-01-02 11:33 ` Manfred Spraul
2016-01-02 12:19 ` Dmitry Vyukov
2016-01-02 15:58 ` Manfred Spraul
2016-02-02 3:25 ` Andrew Morton [this message]
2016-02-02 21:32 ` Dmitry Vyukov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160201192550.6da19ac1.akpm@linux-foundation.org \
--to=akpm@linux-foundation.org \
--cc=aarcange@redhat.com \
--cc=andreyknvl@google.com \
--cc=dave.hansen@linux.intel.com \
--cc=dvyukov@google.com \
--cc=gang.chen.5i5j@gmail.com \
--cc=glider@google.com \
--cc=hughd@google.com \
--cc=joe@perches.com \
--cc=kcc@google.com \
--cc=kirill.shutemov@linux.intel.com \
--cc=kirill@shutemov.name \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=manfred@colorfullife.com \
--cc=mhocko@suse.cz \
--cc=oleg@redhat.com \
--cc=pfeiner@google.com \
--cc=riel@redhat.com \
--cc=sasha.levin@oracle.com \
--cc=sds@tycho.nsa.gov \
--cc=syzkaller@googlegroups.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).