On Wed, Nov 11, 2015 at 09:03:47AM -0800, Davidlohr Bueso wrote:
> On Wed, 11 Nov 2015, Kirill A. Shutemov wrote:
>
> >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 <stdlib.h>
> > #include <sys/ipc.h>
> > #include <sys/mman.h>
> > #include <sys/shm.h>
> >
> > #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
>
> So this is a very similar approach that I posted back when this discussion
> arose: https://lkml.org/lkml/2015/10/12/959 -- There are a few differences
> for which I prefer mine :)
And I had concern about your approach:
If I read it correctly, with the patch we would ignore locking
failure inside shm_open() and mmap will succeed in this case. So
the idea is to have shm_close() no-op and therefore symmetrical.
That's look fragile to me. We would silently miss some other
broken open/close pattern.
>
> o My shm_check_vma_validity() also deals with IPC_RMID as we do the
> ipc_valid_object() check.
Mine too:
shm_mmap()
__shm_open()
shm_lock()
ipc_lock()
ipc_valid_object()
Or I miss something?
> o We have a new WARN where necessary, instead of having one now is shm_open.
I'm not sure why you think that shm_close() which was never paired with
successful shm_open() doesn't deserve WARN().
> o My no-ops explicitly pair.
As I said before, I don't think we should ignore locking error in
shm_open(). If we propagate the error back to caller shm_close() should
never happen, therefore no-op is unneeded in shm_close(): my patch trigger
WARN() there.
> > ret = sfd->file->f_op->mmap(sfd->file, vma);
> >- if (ret != 0)
> >+ if (ret) {
> >+ shm_close(vma);
> > return ret;
> >+ }
>
> Hmm what's this shm_close() about?
Undo shp->shm_nattch++ in successful __shm_open().
I've got impression that I miss something important about how locking in
IPC/SHM works, but I cannot grasp what.. Hm?.
--
Kirill A. Shutemov
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
