Date: Wed, 11 Nov 2015 21:50:23 +0200
From: "Kirill A. Shutemov" <kirill@shutemov.name>
To: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        Dmitry Vyukov <dvyukov@google.com>
Subject: Re: [PATCH, RESEND] ipc/shm: handle removed segments gracefully in
 shm_mmap()
Message-ID: <20151111195023.GA17310@node.shutemov.name>
References: <1447232220-36879-1-git-send-email-kirill.shutemov@linux.intel.com>
 <20151111170347.GA3502@linux-uzut.site>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20151111170347.GA3502@linux-uzut.site>
User-Agent: Mutt/1.5.23.1 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2771
Lines: 94

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/