On Tue, 15 Dec 2015, David Laight wrote: > From: Alan Stern [mailto:stern@xxxxxxxxxxxxxxxxxxx] > > Sent: 14 December 2015 17:59 > > On Mon, 14 Dec 2015, David Laight wrote: > > > > > From: Alan Stern > > > > Sent: 14 December 2015 15:28 > > > ... > > > > I thought of something else, a more serious problem. According to the > > > > man page for munmap(2), closing the file descriptor does not unmap the > > > > region. Therefore we need to explicitly make sure that the usbcore > > > > module cannot be unloaded while any memory mappings exist, by calling > > > > try_module_get(THIS_MODULE) when a mapping is created and > > > > module_put(THIS_MODULE) when a mapping is removed. > > > > > > Are you sure this is a problem? > > > > I haven't actually tried to provoke a crash, if that's what you're > > asking. Apart from that, yes, I'm sure this is a problem. > > There are certainly potential issues with both remap_pfn_range() > and vm_iomap_memory(). What are you thinking of? > > > It is more likely that either: > > > 1) the mmap() holds a reference to the physical pages - so they don't > > > get freed even though the driver frees its reference to the memory. > > > > The problem has nothing to do with the pages being freed. > > > > > 2) when the driver frees the memory the user-space page tables are > > > invalidated - so the next access generates EFAULT. > > > > I'm not sure what you're getting at here. The page tables are > > invalidated when the user program calls munmap (or exits), and the > > driver frees the memory when that happens. > > > > > Not that I've done the experiment (or tried to read the code). > > > > > > If you need to monitor the mapping, you need to allow for partial unmaps. > > > > That's not the problem either -- although I don't know how we should > > handle partial unmaps. Can we disallow them? > > > > The problem is that the user can map the memory, then close the file > > descriptor, then rmmod the usbcore module, then unmap the memory > > region. When the unmap occurs, the memory subsystem will try to > > dereference the usbdev_vm_ops structure -- which has been unloaded > > along with the rest of usbcore. This will cause an oops. > > I was thinking that some side effect of dma_free_coherent() would > find the user mapping for the pages, force unmap them and invalidate > the user PTE (although the address range would have to remain reserved). > Maybe that is wishful thinking. It is. dma_free_coherent() does nothing but remove the DMA mapping and deallocate the memory. It doesn't touch any user page mappings. > Similarly for iounmap(). > > > In theory we could prevent this problem by unmapping the memory when > > the file descriptor is closed. But doing so would violate the > > guarantee in the munmap(2) documentation. > > That doesn't help. > The mapping is inherited by fork() but you only see the last close(). > So the close() need not be in the context of the process that has > the pages mapped. Good point. So it seems that the try_module_get() / module_put() solution is the only one. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
