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> -- Kirill A. Shutemov
