From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-f178.google.com (mail-pf0-f178.google.com [209.85.192.178]) by kanga.kvack.org (Postfix) with ESMTP id 879FD6B0253 for ; Mon, 1 Feb 2016 22:23:04 -0500 (EST) Received: by mail-pf0-f178.google.com with SMTP id o185so90069769pfb.1 for ; Mon, 01 Feb 2016 19:23:04 -0800 (PST) Received: from mail.linuxfoundation.org (mail.linuxfoundation.org. [140.211.169.12]) by mx.google.com with ESMTPS id a7si17622559pas.120.2016.02.01.19.23.03 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 01 Feb 2016 19:23:03 -0800 (PST) Date: Mon, 1 Feb 2016 19:25:50 -0800 From: Andrew Morton Subject: Re: GPF in shm_lock ipc Message-Id: <20160201192550.6da19ac1.akpm@linux-foundation.org> In-Reply-To: References: <20151012122702.GC2544@node> <20151012174945.GC3170@linux-uzut.site> <20151012181040.GC6447@node> <20151012185533.GD3170@linux-uzut.site> <20151013031821.GA3052@linux-uzut.site> <20151013123028.GA12934@node> <20151105142336.46D907FD@black.fi.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Dmitry Vyukov Cc: syzkaller , "Kirill A. Shutemov" , Dave Hansen , Hugh Dickins , Joe Perches , sds@tycho.nsa.gov, Oleg Nesterov , "Kirill A. Shutemov" , Rik van Riel , mhocko@suse.cz, gang.chen.5i5j@gmail.com, Peter Feiner , Andrea Arcangeli , "linux-mm@kvack.org" , LKML , Kostya Serebryany , Alexander Potapenko , Andrey Konovalov , Sasha Levin , Manfred Spraul On Mon, 21 Dec 2015 16:44:34 +0100 Dmitry Vyukov wrote: > On Thu, Nov 5, 2015 at 3:23 PM, Kirill A. Shutemov > 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" 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 #include #include #include #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 Reported-by: Dmitry Vyukov Cc: Davidlohr Bueso Cc: Manfred Spraul Cc: Signed-off-by: Andrew Morton --- 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) _ -- 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: email@kvack.org