On Tue, Apr 10, 2018 at 10:58:22AM +0300, Kirill A. Shutemov wrote: > On Mon, Apr 09, 2018 at 01:36:35PM -0700, Eric Biggers wrote: > > On Mon, Apr 09, 2018 at 01:12:32PM -0700, Davidlohr Bueso wrote: > > > On Mon, 09 Apr 2018, Eric Biggers wrote: > > > > > > > It's necessary because if we don't hold a reference to sfd->file, then it can be > > > > a stale pointer when we compare it in __shm_open(). In particular, if the new > > > > struct file happened to be allocated at the same address as the old one, then > > > > 'sfd->file == shp->shm_file' so the mmap would be allowed. But, it will be a > > > > different shm segment than was intended. The caller may not even have > > > > permissions to map it normally, yet it would be done anyway. > > > > > > > > In the end it's just broken to have a pointer to something that can be freed out > > > > from under you... > > > > > > So this is actually handled by shm_nattch, serialized by the ipc perm->lock. > > > shm_destroy() is called when 0, which in turn does the fput(shm_file). Note > > > that shm_file is given a count of 1 when a new segment is created (deep in > > > get_empty_filp()). So I don't think the pointer is going anywhere, or am I missing > > > something? > > > > > > Thanks, > > > Davidlohr > > > > In the remap_file_pages() case, a reference is taken to the ->vm_file, then the > > segment is unmapped. If that brings ->shm_nattch to 0, then the underlying shm > > segment and ID can be removed, which (currently) causes the real shm file to be > > freed. But, the outer file still exists and will have ->mmap() called on it. > > That's why the outer file needs to hold a reference to the real shm file. > > Okay, fair enough. Logic in SysV IPC implementation is often hard to follow. > Could you include the description in the commit message? > > And feel free to use my > > Acked-by: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx> > I'll send v2 to update the commit message and add a comment. Thanks, Eric
