On Mon, Sep 09, 2019 at 06:55:21AM -0700, Matthew Wilcox wrote: > On Mon, Sep 02, 2019 at 05:20:30PM +0300, Kirill A. Shutemov wrote: > > On Mon, Sep 02, 2019 at 06:52:54AM -0700, Matthew Wilcox wrote: > > > On Sat, Aug 31, 2019 at 12:58:26PM +0800, Hillf Danton wrote: > > > > On Fri, 30 Aug 2019 12:40:06 -0700 > > > > > syzbot found the following crash on: > > > > > > > > > > HEAD commit: a55aa89a Linux 5.3-rc6 > > > > > git tree: upstream > > > > > console output: > > > > > https://syzkaller.appspot.com/x/log.txt?x=12f4beb6600000 > > > > > kernel config: > > > > > https://syzkaller.appspot.com/x/.config?x=2a6a2b9826fdadf9 > > > > > dashboard link: > > > > > https://syzkaller.appspot.com/bug?extid=03ee87124ee05af991bd > > > > > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > > > > > > > > > ================================================================== > > > > > BUG: KASAN: use-after-free in perf_trace_lock_acquire+0x401/0x530 > > > > > include/trace/events/lock.h:13 > > > > > Read of size 8 at addr ffff8880a5cf2c50 by task syz-executor.0/26173 > > > > > > > > --- a/mm/shmem.c > > > > +++ b/mm/shmem.c > > > > @@ -2021,6 +2021,12 @@ static vm_fault_t shmem_fault(struct vm_ > > > > shmem_falloc_waitq = shmem_falloc->waitq; > > > > prepare_to_wait(shmem_falloc_waitq, > > > > &shmem_fault_wait, > > > > TASK_UNINTERRUPTIBLE); > > > > + /* > > > > + * it is not trivial to see what will take > > > > place after > > > > + * releasing i_lock and taking a nap, so hold > > > > inode to > > > > + * be on the safe side. > > > > > > I think the comment could be improved. How about: > > > > > > * The file could be unmapped by another thread after > > > * releasing i_lock, and the inode then freed. Hold > > > * a reference to the inode to prevent this. > > > > It only can happen if mmap_sem was released, so it's better to put > > __iget() to the branch above next to up_read(). I've got confused at first > > how it is possible from ->fault(). > > > > This way iput() below should only be called for ret == VM_FAULT_RETRY. > > Looking at the rather similar construct in filemap.c, should we solve > it the same way, where we inc the refcount on the struct file instead > of the inode before releasing the mmap_sem?
Are you talking about maybe_unlock_mmap_for_io()? Yeah, worth moving it to mm/internal.h and reuse. Care to prepare the patch? :P -- Kirill A. Shutemov
