From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933593AbcBBVc2 (ORCPT ); Tue, 2 Feb 2016 16:32:28 -0500 Received: from mail-wm0-f46.google.com ([74.125.82.46]:34937 "EHLO mail-wm0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754860AbcBBVc0 (ORCPT ); Tue, 2 Feb 2016 16:32:26 -0500 MIME-Version: 1.0 In-Reply-To: <20160201192550.6da19ac1.akpm@linux-foundation.org> 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> <20160201192550.6da19ac1.akpm@linux-foundation.org> From: Dmitry Vyukov Date: Tue, 2 Feb 2016 22:32:04 +0100 Message-ID: Subject: Re: GPF in shm_lock ipc To: Andrew Morton Cc: syzkaller , "Kirill A. Shutemov" , Dave Hansen , Hugh Dickins , Joe Perches , sds@tycho.nsa.gov, Oleg Nesterov , "Kirill A. Shutemov" , Rik van Riel , Michal Hocko , Chen Gang , Peter Feiner , Andrea Arcangeli , "linux-mm@kvack.org" , LKML , Kostya Serebryany , Alexander Potapenko , Andrey Konovalov , Sasha Levin , Manfred Spraul Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Feb 2, 2016 at 4:25 AM, Andrew Morton wrote: > 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? Well, I can say that this patch fixes the original reproducer for me. I will restart the fuzzer with it. > 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) > _ >