[PATCH, RESEND] ipc/shm: handle removed segments gracefully in shm_mmap()

[PATCH, RESEND] ipc/shm: handle removed segments gracefully in shm_mmap()

[Kirill A. Shutemov]

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

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
---
 ipc/shm.c | 53 +++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 10 deletions(-)

diff --git a/ipc/shm.c b/ipc/shm.c
index 41787276e141..3174634ca4e5 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -156,11 +156,12 @@ static inline struct shmid_kernel *shm_lock(struct ipc_namespace *ns, int id)
 	struct kern_ipc_perm *ipcp = ipc_lock(&shm_ids(ns), id);
 
 	/*
-	 * We raced in the idr lookup or with shm_destroy().  Either way, the
-	 * ID is busted.
+	 * Callers of shm_lock() must validate the status of the returned ipc
+	 * object pointer (as returned by ipc_lock()), and error out as
+	 * appropriate.
 	 */
-	WARN_ON(IS_ERR(ipcp));
-
+	if (IS_ERR(ipcp))
+		return (void *)ipcp;
 	return container_of(ipcp, struct shmid_kernel, shm_perm);
 }
 
@@ -186,18 +187,33 @@ static inline void shm_rmid(struct ipc_namespace *ns, struct shmid_kernel *s)
 }
 
 
-/* This is called by fork, once for every shm attach. */
-static void shm_open(struct vm_area_struct *vma)
+static int __shm_open(struct vm_area_struct *vma)
 {
 	struct file *file = vma->vm_file;
 	struct shm_file_data *sfd = shm_file_data(file);
 	struct shmid_kernel *shp;
 
 	shp = shm_lock(sfd->ns, sfd->id);
+
+	if (IS_ERR(shp))
+		return PTR_ERR(shp);
+
 	shp->shm_atim = get_seconds();
 	shp->shm_lprid = task_tgid_vnr(current);
 	shp->shm_nattch++;
 	shm_unlock(shp);
+	return 0;
+}
+
+/* This is called by fork, once for every shm attach. */
+static void shm_open(struct vm_area_struct *vma)
+{
+	int err = __shm_open(vma);
+	/*
+	 * We raced in the idr lookup or with shm_destroy().
+	 * Either way, the ID is busted.
+	 */
+	WARN_ON_ONCE(err);
 }
 
 /*
@@ -260,6 +276,14 @@ static void shm_close(struct vm_area_struct *vma)
 	down_write(&shm_ids(ns).rwsem);
 	/* remove from the list of attaches of the shm segment */
 	shp = shm_lock(ns, sfd->id);
+
+	/*
+	 * We raced in the idr lookup or with shm_destroy().
+	 * Either way, the ID is busted.
+	 */
+	if (WARN_ON_ONCE(IS_ERR(shp)))
+		goto done; /* no-op */
+
 	shp->shm_lprid = task_tgid_vnr(current);
 	shp->shm_dtim = get_seconds();
 	shp->shm_nattch--;
@@ -267,6 +291,7 @@ static void shm_close(struct vm_area_struct *vma)
 		shm_destroy(ns, shp);
 	else
 		shm_unlock(shp);
+done:
 	up_write(&shm_ids(ns).rwsem);
 }
 
@@ -388,17 +413,25 @@ static int shm_mmap(struct file *file, struct vm_area_struct *vma)
 	struct shm_file_data *sfd = shm_file_data(file);
 	int ret;
 
+	/*
+	 * In case of remap_file_pages() emulation, the file can represent
+	 * removed IPC ID: propogate shm_lock() error to caller.
+	 */
+	ret =__shm_open(vma);
+	if (ret)
+		return ret;
+
 	ret = sfd->file->f_op->mmap(sfd->file, vma);
-	if (ret != 0)
+	if (ret) {
+		shm_close(vma);
 		return ret;
+	}
 	sfd->vm_ops = vma->vm_ops;
 #ifdef CONFIG_MMU
 	WARN_ON(!sfd->vm_ops->fault);
 #endif
 	vma->vm_ops = &shm_vm_ops;
-	shm_open(vma);
-
-	return ret;
+	return 0;
 }
 
 static int shm_release(struct inode *ino, struct file *file)
-- 
2.6.1

[PATCH, RESEND] ipc/shm: handle removed segments gracefully in shm_mmap()

[Kirill A. Shutemov]

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

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
---
 ipc/shm.c | 53 +++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 10 deletions(-)

diff --git a/ipc/shm.c b/ipc/shm.c
index 41787276e141..3174634ca4e5 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -156,11 +156,12 @@ static inline struct shmid_kernel *shm_lock(struct ipc_namespace *ns, int id)
 	struct kern_ipc_perm *ipcp = ipc_lock(&shm_ids(ns), id);
 
 	/*
-	 * We raced in the idr lookup or with shm_destroy().  Either way, the
-	 * ID is busted.
+	 * Callers of shm_lock() must validate the status of the returned ipc
+	 * object pointer (as returned by ipc_lock()), and error out as
+	 * appropriate.
 	 */
-	WARN_ON(IS_ERR(ipcp));
-
+	if (IS_ERR(ipcp))
+		return (void *)ipcp;
 	return container_of(ipcp, struct shmid_kernel, shm_perm);
 }
 
@@ -186,18 +187,33 @@ static inline void shm_rmid(struct ipc_namespace *ns, struct shmid_kernel *s)
 }
 
 
-/* This is called by fork, once for every shm attach. */
-static void shm_open(struct vm_area_struct *vma)
+static int __shm_open(struct vm_area_struct *vma)
 {
 	struct file *file = vma->vm_file;
 	struct shm_file_data *sfd = shm_file_data(file);
 	struct shmid_kernel *shp;
 
 	shp = shm_lock(sfd->ns, sfd->id);
+
+	if (IS_ERR(shp))
+		return PTR_ERR(shp);
+
 	shp->shm_atim = get_seconds();
 	shp->shm_lprid = task_tgid_vnr(current);
 	shp->shm_nattch++;
 	shm_unlock(shp);
+	return 0;
+}
+
+/* This is called by fork, once for every shm attach. */
+static void shm_open(struct vm_area_struct *vma)
+{
+	int err = __shm_open(vma);
+	/*
+	 * We raced in the idr lookup or with shm_destroy().
+	 * Either way, the ID is busted.
+	 */
+	WARN_ON_ONCE(err);
 }
 
 /*
@@ -260,6 +276,14 @@ static void shm_close(struct vm_area_struct *vma)
 	down_write(&shm_ids(ns).rwsem);
 	/* remove from the list of attaches of the shm segment */
 	shp = shm_lock(ns, sfd->id);
+
+	/*
+	 * We raced in the idr lookup or with shm_destroy().
+	 * Either way, the ID is busted.
+	 */
+	if (WARN_ON_ONCE(IS_ERR(shp)))
+		goto done; /* no-op */
+
 	shp->shm_lprid = task_tgid_vnr(current);
 	shp->shm_dtim = get_seconds();
 	shp->shm_nattch--;
@@ -267,6 +291,7 @@ static void shm_close(struct vm_area_struct *vma)
 		shm_destroy(ns, shp);
 	else
 		shm_unlock(shp);
+done:
 	up_write(&shm_ids(ns).rwsem);
 }
 
@@ -388,17 +413,25 @@ static int shm_mmap(struct file *file, struct vm_area_struct *vma)
 	struct shm_file_data *sfd = shm_file_data(file);
 	int ret;
 
+	/*
+	 * In case of remap_file_pages() emulation, the file can represent
+	 * removed IPC ID: propogate shm_lock() error to caller.
+	 */
+	ret =__shm_open(vma);
+	if (ret)
+		return ret;
+
 	ret = sfd->file->f_op->mmap(sfd->file, vma);
-	if (ret != 0)
+	if (ret) {
+		shm_close(vma);
 		return ret;
+	}
 	sfd->vm_ops = vma->vm_ops;
 #ifdef CONFIG_MMU
 	WARN_ON(!sfd->vm_ops->fault);
 #endif
 	vma->vm_ops = &shm_vm_ops;
-	shm_open(vma);
-
-	return ret;
+	return 0;
 }
 
 static int shm_release(struct inode *ino, struct file *file)
-- 
2.6.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH, RESEND] ipc/shm: handle removed segments gracefully in shm_mmap()

[Davidlohr Bueso]

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 :)

o My shm_check_vma_validity() also deals with IPC_RMID as we do the
ipc_valid_object() check.

o We have a new WARN where necessary, instead of having one now is shm_open.

o My no-ops explicitly pair.

[...]

> 	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?

Thanks,
Davidlohr

Re: [PATCH, RESEND] ipc/shm: handle removed segments gracefully in shm_mmap()

[Davidlohr Bueso]

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 :)

o My shm_check_vma_validity() also deals with IPC_RMID as we do the
ipc_valid_object() check.

o We have a new WARN where necessary, instead of having one now is shm_open.

o My no-ops explicitly pair.

[...]

> 	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?

Thanks,
Davidlohr

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH, RESEND] ipc/shm: handle removed segments gracefully in shm_mmap()

[Kirill A. Shutemov]

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

Re: [PATCH, RESEND] ipc/shm: handle removed segments gracefully in shm_mmap()

[Kirill A. Shutemov]

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, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH, RESEND] ipc/shm: handle removed segments gracefully in shm_mmap()

[Davidlohr Bueso]

On Wed, 11 Nov 2015, Kirill A. Shutemov wrote:
>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.

Both open and close are no-ops in the case the segment has been removed,
that's the symmetrical, and I'm not sure I follow -- we don't ignore locking
failure in shm_open _at all_. Just like your approach, all I do is return if
there's an error...

>	That's look fragile to me. We would silently miss some other
>	broken open/close pattern.

Such cases, if any, should be fixed and handled appropriately, not hide
it under the rung, methinks.

>>
>> 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?

Sorry, I meant ipc_obtain_object_idr, so EINVAL is also accounted for, we
the segment is already deleted and not only marked as such.

>
>> 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.

Yes, you WARN() in shm_close, but you still make it a no-op...

>
>> >	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().

Yeah that's just nasty.

>
>I've got impression that I miss something important about how locking in
>IPC/SHM works, but I cannot grasp what.. Hm?.

Could you be more specific? The only lock involved here is the ipc object lock,
if you haven't, you might want to refer to ipc/util.c which has a brief ipc
locking description.

Re: [PATCH, RESEND] ipc/shm: handle removed segments gracefully in shm_mmap()

[Davidlohr Bueso]

On Wed, 11 Nov 2015, Kirill A. Shutemov wrote:
>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.

Both open and close are no-ops in the case the segment has been removed,
that's the symmetrical, and I'm not sure I follow -- we don't ignore locking
failure in shm_open _at all_. Just like your approach, all I do is return if
there's an error...

>	That's look fragile to me. We would silently miss some other
>	broken open/close pattern.

Such cases, if any, should be fixed and handled appropriately, not hide
it under the rung, methinks.

>>
>> 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?

Sorry, I meant ipc_obtain_object_idr, so EINVAL is also accounted for, we
the segment is already deleted and not only marked as such.

>
>> 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.

Yes, you WARN() in shm_close, but you still make it a no-op...

>
>> >	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().

Yeah that's just nasty.

>
>I've got impression that I miss something important about how locking in
>IPC/SHM works, but I cannot grasp what.. Hm?.

Could you be more specific? The only lock involved here is the ipc object lock,
if you haven't, you might want to refer to ipc/util.c which has a brief ipc
locking description.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH, RESEND] ipc/shm: handle removed segments gracefully in shm_mmap()

[Kirill A. Shutemov]

On Thu, Nov 12, 2015 at 09:31:37PM -0800, Davidlohr Bueso wrote:
> On Wed, 11 Nov 2015, Kirill A. Shutemov wrote:
> >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.
> 
> Both open and close are no-ops in the case the segment has been removed,

The part I disagree is that shm_open() shouldn't be allowed for removed
segment. Basically, I prefer to keep the policy we have now.

> that's the symmetrical, and I'm not sure I follow -- we don't ignore locking
> failure in shm_open _at all_. Just like your approach, all I do is return if
> there's an error...

As you wrote in the comment, shm_check_vma_validity() check is racy. It's
just speculative check which doesn't guarantee that shm_lock() in
shm_open() will succeed. If this race happen, you just ignore this locking
failure and proceed. You compensate this, essentially failed shm_open(),
by no-op in shm_close().

In my opinion, failed shm_lock() in shm_open() should lead to returning
error from shm_mmap(). And there's no need in shm_close() hackery.
My patch tries to implement this.

> 
> >	That's look fragile to me. We would silently miss some other
> >	broken open/close pattern.
> 
> Such cases, if any, should be fixed and handled appropriately, not hide
> it under the rung, methinks.

But, don't you think you *do* hide such cases? With you patch pattern like
shm_open()-shm_close()-shm_close() will not trigger any visible effect.

> >>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.
> 
> Yes, you WARN() in shm_close, but you still make it a no-op...

We can crash kernel with BUG_ON() there, but would it help anyone?
The WARN() is just to make broken open/close visible.

> >>>	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().
> 
> Yeah that's just nasty.

I don't see why: we successfully opened the segment, but f_op->mmap
failed -- let's close the segment. It's normal error path.

-- 
 Kirill A. Shutemov

Re: [PATCH, RESEND] ipc/shm: handle removed segments gracefully in shm_mmap()

[Kirill A. Shutemov]

On Thu, Nov 12, 2015 at 09:31:37PM -0800, Davidlohr Bueso wrote:
> On Wed, 11 Nov 2015, Kirill A. Shutemov wrote:
> >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.
> 
> Both open and close are no-ops in the case the segment has been removed,

The part I disagree is that shm_open() shouldn't be allowed for removed
segment. Basically, I prefer to keep the policy we have now.

> that's the symmetrical, and I'm not sure I follow -- we don't ignore locking
> failure in shm_open _at all_. Just like your approach, all I do is return if
> there's an error...

As you wrote in the comment, shm_check_vma_validity() check is racy. It's
just speculative check which doesn't guarantee that shm_lock() in
shm_open() will succeed. If this race happen, you just ignore this locking
failure and proceed. You compensate this, essentially failed shm_open(),
by no-op in shm_close().

In my opinion, failed shm_lock() in shm_open() should lead to returning
error from shm_mmap(). And there's no need in shm_close() hackery.
My patch tries to implement this.

> 
> >	That's look fragile to me. We would silently miss some other
> >	broken open/close pattern.
> 
> Such cases, if any, should be fixed and handled appropriately, not hide
> it under the rung, methinks.

But, don't you think you *do* hide such cases? With you patch pattern like
shm_open()-shm_close()-shm_close() will not trigger any visible effect.

> >>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.
> 
> Yes, you WARN() in shm_close, but you still make it a no-op...

We can crash kernel with BUG_ON() there, but would it help anyone?
The WARN() is just to make broken open/close visible.

> >>>	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().
> 
> Yeah that's just nasty.

I don't see why: we successfully opened the segment, but f_op->mmap
failed -- let's close the segment. It's normal error path.

-- 
 Kirill A. Shutemov

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH, RESEND] ipc/shm: handle removed segments gracefully in shm_mmap()

[Davidlohr Bueso]

On Fri, 13 Nov 2015, Kirill A. Shutemov wrote:

>On Thu, Nov 12, 2015 at 09:31:37PM -0800, Davidlohr Bueso wrote:
>> On Wed, 11 Nov 2015, Kirill A. Shutemov wrote:
>> >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.
>>
>> Both open and close are no-ops in the case the segment has been removed,
>
>The part I disagree is that shm_open() shouldn't be allowed for removed
>segment. Basically, I prefer to keep the policy we have now.
>
>> that's the symmetrical, and I'm not sure I follow -- we don't ignore locking
>> failure in shm_open _at all_. Just like your approach, all I do is return if
>> there's an error...
>
>As you wrote in the comment, shm_check_vma_validity() check is racy. It's
>just speculative check which doesn't guarantee that shm_lock() in
>shm_open() will succeed. If this race happen, you just ignore this locking
>failure and proceed. You compensate this, essentially failed shm_open(),
>by no-op in shm_close().

With the exception of the call in shm_mmap, we handle shm_open and shm_close
the same way, we both consider them no-ops, just that you return the error
code from shm_lock. But we can easily recover this error within the mmap call,
so this seems unnecessary. See below.

>
>In my opinion, failed shm_lock() in shm_open() should lead to returning
>error from shm_mmap(). And there's no need in shm_close() hackery.
>My patch tries to implement this.
>
>>
>> >	That's look fragile to me. We would silently miss some other
>> >	broken open/close pattern.
>>
>> Such cases, if any, should be fixed and handled appropriately, not hide
>> it under the rung, methinks.
>
>But, don't you think you *do* hide such cases? With you patch pattern like
>shm_open()-shm_close()-shm_close() will not trigger any visible effect.
>
>> >>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.
>>
>> Yes, you WARN() in shm_close, but you still make it a no-op...
>
>We can crash kernel with BUG_ON() there, but would it help anyone?

So a failed shm_lock() used to always be a BUG_ON, but I don't think
we want to go back to that. Ultimately, a busted ipc id is not a reason
to halt the kernel.

>The WARN() is just to make broken open/close visible.

I really don't like that you have two different logic for shm_open and close
(more below).

>
>> >>>	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().
>>
>> Yeah that's just nasty.
>
>I don't see why: we successfully opened the segment, but f_op->mmap
>failed -- let's close the segment. It's normal error path.

I was referring to the fact that I hate having to prematurely call shm_open()
just for this case, and then have to backout, ie for nattach. Similarly, I
dislike that you make shm_close behave one way and _shm_open another, looks
hacky.

That said, I do agree that we should inform EIDRM back to the shm_mmap
caller. My immediate thought would be to recheck right after shm_open returns.
I realize this is also hacky as we run into similar inconsistencies that I
mentioned above. But that's a caller (and the only one), not the whole
shm_open/close. Also, just like we are concerned about EIDRM, should we also
care about EINVAL -- where we race with explicit user shmctl(RMID) calls but
we hold reference to nattach?? I mean, why bother doing mmap if the segment is
marked for deletion and ipc won't touch it again anyway (failed idr lookups).
The downside to that is the extra lookup overhead, so perhaps your approach
is better. But looks like the right thing to do conceptually. Something like so?

shm_mmap()
{
	err = shm_check_vma_validity()
	if (err)

	->mmap()

	shm_open()
	err = shm_check_vma_validity()
	if (err)
	   return err; /* shm_open was a nop, return the corresponding error */

	return 0;
}

So considering EINVAL, even your approach to bumping up nattach by calling
_shm_open earlier isn't enough. Races exposed to user called rmid can still
occur between dropping the lock and doing ->mmap(). Ultimately this leads to
all ipc_valid_object() checks, as we totally ignore SHM_DEST segments nowadays
since we forbid mapping previously removed segments.

I think this is the first thing we must decide before going forward with this
mess. ipc currently defines invalid objects by merely checking the deleted flag.

Manfred, any thoughts?

Thanks,
Davidlohr

Re: [PATCH, RESEND] ipc/shm: handle removed segments gracefully in shm_mmap()

[Davidlohr Bueso]

On Fri, 13 Nov 2015, Kirill A. Shutemov wrote:

>On Thu, Nov 12, 2015 at 09:31:37PM -0800, Davidlohr Bueso wrote:
>> On Wed, 11 Nov 2015, Kirill A. Shutemov wrote:
>> >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.
>>
>> Both open and close are no-ops in the case the segment has been removed,
>
>The part I disagree is that shm_open() shouldn't be allowed for removed
>segment. Basically, I prefer to keep the policy we have now.
>
>> that's the symmetrical, and I'm not sure I follow -- we don't ignore locking
>> failure in shm_open _at all_. Just like your approach, all I do is return if
>> there's an error...
>
>As you wrote in the comment, shm_check_vma_validity() check is racy. It's
>just speculative check which doesn't guarantee that shm_lock() in
>shm_open() will succeed. If this race happen, you just ignore this locking
>failure and proceed. You compensate this, essentially failed shm_open(),
>by no-op in shm_close().

With the exception of the call in shm_mmap, we handle shm_open and shm_close
the same way, we both consider them no-ops, just that you return the error
code from shm_lock. But we can easily recover this error within the mmap call,
so this seems unnecessary. See below.

>
>In my opinion, failed shm_lock() in shm_open() should lead to returning
>error from shm_mmap(). And there's no need in shm_close() hackery.
>My patch tries to implement this.
>
>>
>> >	That's look fragile to me. We would silently miss some other
>> >	broken open/close pattern.
>>
>> Such cases, if any, should be fixed and handled appropriately, not hide
>> it under the rung, methinks.
>
>But, don't you think you *do* hide such cases? With you patch pattern like
>shm_open()-shm_close()-shm_close() will not trigger any visible effect.
>
>> >>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.
>>
>> Yes, you WARN() in shm_close, but you still make it a no-op...
>
>We can crash kernel with BUG_ON() there, but would it help anyone?

So a failed shm_lock() used to always be a BUG_ON, but I don't think
we want to go back to that. Ultimately, a busted ipc id is not a reason
to halt the kernel.

>The WARN() is just to make broken open/close visible.

I really don't like that you have two different logic for shm_open and close
(more below).

>
>> >>>	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().
>>
>> Yeah that's just nasty.
>
>I don't see why: we successfully opened the segment, but f_op->mmap
>failed -- let's close the segment. It's normal error path.

I was referring to the fact that I hate having to prematurely call shm_open()
just for this case, and then have to backout, ie for nattach. Similarly, I
dislike that you make shm_close behave one way and _shm_open another, looks
hacky.

That said, I do agree that we should inform EIDRM back to the shm_mmap
caller. My immediate thought would be to recheck right after shm_open returns.
I realize this is also hacky as we run into similar inconsistencies that I
mentioned above. But that's a caller (and the only one), not the whole
shm_open/close. Also, just like we are concerned about EIDRM, should we also
care about EINVAL -- where we race with explicit user shmctl(RMID) calls but
we hold reference to nattach?? I mean, why bother doing mmap if the segment is
marked for deletion and ipc won't touch it again anyway (failed idr lookups).
The downside to that is the extra lookup overhead, so perhaps your approach
is better. But looks like the right thing to do conceptually. Something like so?

shm_mmap()
{
	err = shm_check_vma_validity()
	if (err)

	->mmap()

	shm_open()
	err = shm_check_vma_validity()
	if (err)
	   return err; /* shm_open was a nop, return the corresponding error */

	return 0;
}

So considering EINVAL, even your approach to bumping up nattach by calling
_shm_open earlier isn't enough. Races exposed to user called rmid can still
occur between dropping the lock and doing ->mmap(). Ultimately this leads to
all ipc_valid_object() checks, as we totally ignore SHM_DEST segments nowadays
since we forbid mapping previously removed segments.

I think this is the first thing we must decide before going forward with this
mess. ipc currently defines invalid objects by merely checking the deleted flag.

Manfred, any thoughts?

Thanks,
Davidlohr

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH, RESEND] ipc/shm: handle removed segments gracefully in shm_mmap()

[Davidlohr Bueso]

On Fri, 13 Nov 2015, Bueso wrote:


>So considering EINVAL, even your approach to bumping up nattach by calling
>_shm_open earlier isn't enough. Races exposed to user called rmid can still
>occur between dropping the lock and doing ->mmap(). Ultimately this leads to
>all ipc_valid_object() checks, as we totally ignore SHM_DEST segments nowadays
>since we forbid mapping previously removed segments.
>
>I think this is the first thing we must decide before going forward with this
>mess. ipc currently defines invalid objects by merely checking the deleted flag.

Particularly something like this, which we could then add to the vma validity
check, thus saving the lookup as well.

diff --git a/ipc/shm.c b/ipc/shm.c
index 4178727..d9b2fb1 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -64,6 +64,20 @@ static const struct vm_operations_struct shm_vm_ops;
  #define shm_unlock(shp)			\
  	ipc_unlock(&(shp)->shm_perm)
  
+/*
+ * shm object validity is different than the rest of ipc
+ * as shm needs to deal with segments previously marked
+ * for deletion, which can occur at any time via user calls.
+ */
+static inline int shm_invalid_object(struct kern_ipc_perm *perm)
+{
+	if (perm->mode & SHM_DEST)
+		return -EINVAL;
+	if (ipc_valid_object(perm))
+		return -EIDRM;
+	return 0; /* yay */
+}
+
  static int newseg(struct ipc_namespace *, struct ipc_params *);
  static void shm_open(struct vm_area_struct *vma);
  static void shm_close(struct vm_area_struct *vma);
@@ -985,11 +999,9 @@ SYSCALL_DEFINE3(shmctl, int, shmid, int, cmd, struct shmid_ds __user *, buf)
  
  		ipc_lock_object(&shp->shm_perm);
  
-		/* check if shm_destroy() is tearing down shp */
-		if (!ipc_valid_object(&shp->shm_perm)) {
-			err = -EIDRM;
+		err = shm_invalid_object(&shp->shm_perm);
+		if (err)
  			goto out_unlock0;
-		}
  
  		if (!ns_capable(ns->user_ns, CAP_IPC_LOCK)) {
  			kuid_t euid = current_euid();
@@ -1124,10 +1136,9 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
  
  	ipc_lock_object(&shp->shm_perm);
  
-	/* check if shm_destroy() is tearing down shp */
-	if (!ipc_valid_object(&shp->shm_perm)) {
+	err = shm_invalid_object(&shp->shm_perm);
+	if (err) {
  		ipc_unlock_object(&shp->shm_perm);
-		err = -EIDRM;
  		goto out_unlock;
  	}
  

Re: [PATCH, RESEND] ipc/shm: handle removed segments gracefully in shm_mmap()

[Davidlohr Bueso]

On Fri, 13 Nov 2015, Bueso wrote:


>So considering EINVAL, even your approach to bumping up nattach by calling
>_shm_open earlier isn't enough. Races exposed to user called rmid can still
>occur between dropping the lock and doing ->mmap(). Ultimately this leads to
>all ipc_valid_object() checks, as we totally ignore SHM_DEST segments nowadays
>since we forbid mapping previously removed segments.
>
>I think this is the first thing we must decide before going forward with this
>mess. ipc currently defines invalid objects by merely checking the deleted flag.

Particularly something like this, which we could then add to the vma validity
check, thus saving the lookup as well.

diff --git a/ipc/shm.c b/ipc/shm.c
index 4178727..d9b2fb1 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -64,6 +64,20 @@ static const struct vm_operations_struct shm_vm_ops;
  #define shm_unlock(shp)			\
  	ipc_unlock(&(shp)->shm_perm)
  
+/*
+ * shm object validity is different than the rest of ipc
+ * as shm needs to deal with segments previously marked
+ * for deletion, which can occur at any time via user calls.
+ */
+static inline int shm_invalid_object(struct kern_ipc_perm *perm)
+{
+	if (perm->mode & SHM_DEST)
+		return -EINVAL;
+	if (ipc_valid_object(perm))
+		return -EIDRM;
+	return 0; /* yay */
+}
+
  static int newseg(struct ipc_namespace *, struct ipc_params *);
  static void shm_open(struct vm_area_struct *vma);
  static void shm_close(struct vm_area_struct *vma);
@@ -985,11 +999,9 @@ SYSCALL_DEFINE3(shmctl, int, shmid, int, cmd, struct shmid_ds __user *, buf)
  
  		ipc_lock_object(&shp->shm_perm);
  
-		/* check if shm_destroy() is tearing down shp */
-		if (!ipc_valid_object(&shp->shm_perm)) {
-			err = -EIDRM;
+		err = shm_invalid_object(&shp->shm_perm);
+		if (err)
  			goto out_unlock0;
-		}
  
  		if (!ns_capable(ns->user_ns, CAP_IPC_LOCK)) {
  			kuid_t euid = current_euid();
@@ -1124,10 +1136,9 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
  
  	ipc_lock_object(&shp->shm_perm);
  
-	/* check if shm_destroy() is tearing down shp */
-	if (!ipc_valid_object(&shp->shm_perm)) {
+	err = shm_invalid_object(&shp->shm_perm);
+	if (err) {
  		ipc_unlock_object(&shp->shm_perm);
-		err = -EIDRM;
  		goto out_unlock;
  	}
  

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH, RESEND] ipc/shm: handle removed segments gracefully in shm_mmap()

[Kirill A. Shutemov]

On Fri, Nov 13, 2015 at 11:23:10AM -0800, Davidlohr Bueso wrote:
> On Fri, 13 Nov 2015, Kirill A. Shutemov wrote:
> 
> >On Thu, Nov 12, 2015 at 09:31:37PM -0800, Davidlohr Bueso wrote:
> >>On Wed, 11 Nov 2015, Kirill A. Shutemov wrote:
> >>>>>	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().
> >>
> >>Yeah that's just nasty.
> >
> >I don't see why: we successfully opened the segment, but f_op->mmap
> >failed -- let's close the segment. It's normal error path.
> 
> I was referring to the fact that I hate having to prematurely call shm_open()
> just for this case, and then have to backout, ie for nattach. Similarly, I
> dislike that you make shm_close behave one way and _shm_open another, looks
> hacky.
> 
> That said, I do agree that we should inform EIDRM back to the shm_mmap
> caller. My immediate thought would be to recheck right after shm_open returns.
> I realize this is also hacky as we run into similar inconsistencies that I
> mentioned above. But that's a caller (and the only one), not the whole
> shm_open/close. Also, just like we are concerned about EIDRM, should we also
> care about EINVAL -- where we race with explicit user shmctl(RMID) calls but
> we hold reference to nattach?? I mean, why bother doing mmap if the segment is
> marked for deletion and ipc won't touch it again anyway (failed idr lookups).
> The downside to that is the extra lookup overhead, so perhaps your approach
> is better. But looks like the right thing to do conceptually. Something like so?
> 
> shm_mmap()
> {
> 	err = shm_check_vma_validity()
> 	if (err)
> 
> 	->mmap()
> 
> 	shm_open()
> 	err = shm_check_vma_validity()
> 	if (err)
> 	   return err; /* shm_open was a nop, return the corresponding error */
> 
> 	return 0;
> }

The problem I have with this approach is that it assumes that there's
nothing to undo from ->mmap in case of shm_check_validity() failed in the
second call. That seems true at the moment, but I'm not sure if we can
assume this in general and if it's future-proof.

> So considering EINVAL, even your approach to bumping up nattach by calling
> _shm_open earlier isn't enough. Races exposed to user called rmid can still
> occur between dropping the lock and doing ->mmap().

Ugh.. I see. That's a problem.

Looks like a problem we solved for mm_struct by separation of mm_count
from mm_users. Should we have two counters instead of shm_nattch?

> Ultimately this leads to all ipc_valid_object() checks, as we totally
> ignore SHM_DEST segments nowadays since we forbid mapping previously
> removed segments.
> 
> I think this is the first thing we must decide before going forward with this
> mess. ipc currently defines invalid objects by merely checking the deleted flag.

To me all these flags mess should be replaced by proper refcounting.
Although, I admit, I don't understand SysV IPC API good enough to say for
sure if it's possible.

-- 
 Kirill A. Shutemov

Re: [PATCH, RESEND] ipc/shm: handle removed segments gracefully in shm_mmap()

[Kirill A. Shutemov]

On Fri, Nov 13, 2015 at 11:23:10AM -0800, Davidlohr Bueso wrote:
> On Fri, 13 Nov 2015, Kirill A. Shutemov wrote:
> 
> >On Thu, Nov 12, 2015 at 09:31:37PM -0800, Davidlohr Bueso wrote:
> >>On Wed, 11 Nov 2015, Kirill A. Shutemov wrote:
> >>>>>	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().
> >>
> >>Yeah that's just nasty.
> >
> >I don't see why: we successfully opened the segment, but f_op->mmap
> >failed -- let's close the segment. It's normal error path.
> 
> I was referring to the fact that I hate having to prematurely call shm_open()
> just for this case, and then have to backout, ie for nattach. Similarly, I
> dislike that you make shm_close behave one way and _shm_open another, looks
> hacky.
> 
> That said, I do agree that we should inform EIDRM back to the shm_mmap
> caller. My immediate thought would be to recheck right after shm_open returns.
> I realize this is also hacky as we run into similar inconsistencies that I
> mentioned above. But that's a caller (and the only one), not the whole
> shm_open/close. Also, just like we are concerned about EIDRM, should we also
> care about EINVAL -- where we race with explicit user shmctl(RMID) calls but
> we hold reference to nattach?? I mean, why bother doing mmap if the segment is
> marked for deletion and ipc won't touch it again anyway (failed idr lookups).
> The downside to that is the extra lookup overhead, so perhaps your approach
> is better. But looks like the right thing to do conceptually. Something like so?
> 
> shm_mmap()
> {
> 	err = shm_check_vma_validity()
> 	if (err)
> 
> 	->mmap()
> 
> 	shm_open()
> 	err = shm_check_vma_validity()
> 	if (err)
> 	   return err; /* shm_open was a nop, return the corresponding error */
> 
> 	return 0;
> }

The problem I have with this approach is that it assumes that there's
nothing to undo from ->mmap in case of shm_check_validity() failed in the
second call. That seems true at the moment, but I'm not sure if we can
assume this in general and if it's future-proof.

> So considering EINVAL, even your approach to bumping up nattach by calling
> _shm_open earlier isn't enough. Races exposed to user called rmid can still
> occur between dropping the lock and doing ->mmap().

Ugh.. I see. That's a problem.

Looks like a problem we solved for mm_struct by separation of mm_count
from mm_users. Should we have two counters instead of shm_nattch?

> Ultimately this leads to all ipc_valid_object() checks, as we totally
> ignore SHM_DEST segments nowadays since we forbid mapping previously
> removed segments.
> 
> I think this is the first thing we must decide before going forward with this
> mess. ipc currently defines invalid objects by merely checking the deleted flag.

To me all these flags mess should be replaced by proper refcounting.
Although, I admit, I don't understand SysV IPC API good enough to say for
sure if it's possible.

-- 
 Kirill A. Shutemov

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH, RESEND] ipc/shm: handle removed segments gracefully in shm_mmap()

[Manfred Spraul]

On 11/13/2015 08:23 PM, Davidlohr Bueso wrote:
>
> So considering EINVAL, even your approach to bumping up nattach by 
> calling
> _shm_open earlier isn't enough. Races exposed to user called rmid can 
> still
> occur between dropping the lock and doing ->mmap(). Ultimately this 
> leads to
> all ipc_valid_object() checks, as we totally ignore SHM_DEST segments 
> nowadays
> since we forbid mapping previously removed segments.
>
> I think this is the first thing we must decide before going forward 
> with this
> mess. ipc currently defines invalid objects by merely checking the 
> deleted flag.
>
> Manfred, any thoughts?
>
With regards to locking: Sorry, shm is too different to msg/sem/mqueue.

With regards to EIDRM / EINVAL:
When all kernel memory was released, then the kernel cannot find out if 
the ID was valid at one time or not.
Thus EIDRM can only be a hint, the OS (kernel/libc) cannot guarantee 
that user space will never see something else.
(trivial example: user space sleeps just before the syscall)

So I would not create special code to optimize EIDRM handling for races. 
If we sometimes report EINVAL, it would be probably ok as well.

--
     Manfred

Re: [PATCH, RESEND] ipc/shm: handle removed segments gracefully in shm_mmap()

[Manfred Spraul]

On 11/13/2015 08:23 PM, Davidlohr Bueso wrote:
>
> So considering EINVAL, even your approach to bumping up nattach by 
> calling
> _shm_open earlier isn't enough. Races exposed to user called rmid can 
> still
> occur between dropping the lock and doing ->mmap(). Ultimately this 
> leads to
> all ipc_valid_object() checks, as we totally ignore SHM_DEST segments 
> nowadays
> since we forbid mapping previously removed segments.
>
> I think this is the first thing we must decide before going forward 
> with this
> mess. ipc currently defines invalid objects by merely checking the 
> deleted flag.
>
> Manfred, any thoughts?
>
With regards to locking: Sorry, shm is too different to msg/sem/mqueue.

With regards to EIDRM / EINVAL:
When all kernel memory was released, then the kernel cannot find out if 
the ID was valid at one time or not.
Thus EIDRM can only be a hint, the OS (kernel/libc) cannot guarantee 
that user space will never see something else.
(trivial example: user space sleeps just before the syscall)

So I would not create special code to optimize EIDRM handling for races. 
If we sometimes report EINVAL, it would be probably ok as well.

--
     Manfred

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH, RESEND] ipc/shm: handle removed segments gracefully in shm_mmap()

[Kirill A. Shutemov]

On Sat, Jan 02, 2016 at 12:45:07PM +0100, Manfred Spraul wrote:
> On 11/13/2015 08:23 PM, Davidlohr Bueso wrote:
> >
> >So considering EINVAL, even your approach to bumping up nattach by calling
> >_shm_open earlier isn't enough. Races exposed to user called rmid can
> >still
> >occur between dropping the lock and doing ->mmap(). Ultimately this leads
> >to
> >all ipc_valid_object() checks, as we totally ignore SHM_DEST segments
> >nowadays
> >since we forbid mapping previously removed segments.
> >
> >I think this is the first thing we must decide before going forward with
> >this
> >mess. ipc currently defines invalid objects by merely checking the deleted
> >flag.
> >
> >Manfred, any thoughts?
> >
> With regards to locking: Sorry, shm is too different to msg/sem/mqueue.
> 
> With regards to EIDRM / EINVAL:
> When all kernel memory was released, then the kernel cannot find out if the
> ID was valid at one time or not.
> Thus EIDRM can only be a hint, the OS (kernel/libc) cannot guarantee that
> user space will never see something else.
> (trivial example: user space sleeps just before the syscall)
> 
> So I would not create special code to optimize EIDRM handling for races. If
> we sometimes report EINVAL, it would be probably ok as well.

Guys, here's yet another attempt to fix the issue.

The key idea this time is to use shm_ids(ns).rwsem taken for read in shm_mmap()
to prevent rmid under us.

Any problem with this?

diff --git a/ipc/shm.c b/ipc/shm.c
index ed3027d0f277..b306fb3d9586 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -156,11 +156,12 @@ static inline struct shmid_kernel *shm_lock(struct ipc_namespace *ns, int id)
 	struct kern_ipc_perm *ipcp = ipc_lock(&shm_ids(ns), id);
 
 	/*
-	 * We raced in the idr lookup or with shm_destroy().  Either way, the
-	 * ID is busted.
+	 * Callers of shm_lock() must validate the status of the returned ipc
+	 * object pointer (as returned by ipc_lock()), and error out as
+	 * appropriate.
 	 */
-	WARN_ON(IS_ERR(ipcp));
-
+	if (IS_ERR(ipcp))
+		return (void *)ipcp;
 	return container_of(ipcp, struct shmid_kernel, shm_perm);
 }
 
@@ -194,6 +195,14 @@ static void shm_open(struct vm_area_struct *vma)
 	struct shmid_kernel *shp;
 
 	shp = shm_lock(sfd->ns, sfd->id);
+
+	/*
+	 * We raced in the idr lookup or with shm_destroy().
+	 * Either way, the ID is busted.
+	 */
+	if (WARN_ON(IS_ERR(shp)))
+		return ;
+
 	shp->shm_atim = get_seconds();
 	shp->shm_lprid = task_tgid_vnr(current);
 	shp->shm_nattch++;
@@ -386,18 +395,34 @@ static struct mempolicy *shm_get_policy(struct vm_area_struct *vma,
 static int shm_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	struct shm_file_data *sfd = shm_file_data(file);
+	struct shmid_kernel *shp;
 	int ret;
 
+	/* Prevent rmid under us */
+	down_read(&shm_ids(sfd->ns).rwsem);
+
+	/* Check if we can map the segment */
+	shp = shm_lock(sfd->ns, sfd->id);
+	if (IS_ERR(shp)) {
+		ret = PTR_ERR(shp);
+		goto out;
+	}
+	ret = shp->shm_perm.mode & SHM_DEST ? -EINVAL : 0;
+	shm_unlock(shp);
+	if (ret)
+		goto out;
+
 	ret = sfd->file->f_op->mmap(sfd->file, vma);
 	if (ret != 0)
-		return ret;
+		goto out;
 	sfd->vm_ops = vma->vm_ops;
 #ifdef CONFIG_MMU
 	WARN_ON(!sfd->vm_ops->fault);
 #endif
 	vma->vm_ops = &shm_vm_ops;
 	shm_open(vma);
-
+out:
+	up_read(&shm_ids(sfd->ns).rwsem);
 	return ret;
 }
 
-- 
 Kirill A. Shutemov

Re: [PATCH, RESEND] ipc/shm: handle removed segments gracefully in shm_mmap()

[Kirill A. Shutemov]

On Sat, Jan 02, 2016 at 12:45:07PM +0100, Manfred Spraul wrote:
> On 11/13/2015 08:23 PM, Davidlohr Bueso wrote:
> >
> >So considering EINVAL, even your approach to bumping up nattach by calling
> >_shm_open earlier isn't enough. Races exposed to user called rmid can
> >still
> >occur between dropping the lock and doing ->mmap(). Ultimately this leads
> >to
> >all ipc_valid_object() checks, as we totally ignore SHM_DEST segments
> >nowadays
> >since we forbid mapping previously removed segments.
> >
> >I think this is the first thing we must decide before going forward with
> >this
> >mess. ipc currently defines invalid objects by merely checking the deleted
> >flag.
> >
> >Manfred, any thoughts?
> >
> With regards to locking: Sorry, shm is too different to msg/sem/mqueue.
> 
> With regards to EIDRM / EINVAL:
> When all kernel memory was released, then the kernel cannot find out if the
> ID was valid at one time or not.
> Thus EIDRM can only be a hint, the OS (kernel/libc) cannot guarantee that
> user space will never see something else.
> (trivial example: user space sleeps just before the syscall)
> 
> So I would not create special code to optimize EIDRM handling for races. If
> we sometimes report EINVAL, it would be probably ok as well.

Guys, here's yet another attempt to fix the issue.

The key idea this time is to use shm_ids(ns).rwsem taken for read in shm_mmap()
to prevent rmid under us.

Any problem with this?

diff --git a/ipc/shm.c b/ipc/shm.c
index ed3027d0f277..b306fb3d9586 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -156,11 +156,12 @@ static inline struct shmid_kernel *shm_lock(struct ipc_namespace *ns, int id)
 	struct kern_ipc_perm *ipcp = ipc_lock(&shm_ids(ns), id);
 
 	/*
-	 * We raced in the idr lookup or with shm_destroy().  Either way, the
-	 * ID is busted.
+	 * Callers of shm_lock() must validate the status of the returned ipc
+	 * object pointer (as returned by ipc_lock()), and error out as
+	 * appropriate.
 	 */
-	WARN_ON(IS_ERR(ipcp));
-
+	if (IS_ERR(ipcp))
+		return (void *)ipcp;
 	return container_of(ipcp, struct shmid_kernel, shm_perm);
 }
 
@@ -194,6 +195,14 @@ static void shm_open(struct vm_area_struct *vma)
 	struct shmid_kernel *shp;
 
 	shp = shm_lock(sfd->ns, sfd->id);
+
+	/*
+	 * We raced in the idr lookup or with shm_destroy().
+	 * Either way, the ID is busted.
+	 */
+	if (WARN_ON(IS_ERR(shp)))
+		return ;
+
 	shp->shm_atim = get_seconds();
 	shp->shm_lprid = task_tgid_vnr(current);
 	shp->shm_nattch++;
@@ -386,18 +395,34 @@ static struct mempolicy *shm_get_policy(struct vm_area_struct *vma,
 static int shm_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	struct shm_file_data *sfd = shm_file_data(file);
+	struct shmid_kernel *shp;
 	int ret;
 
+	/* Prevent rmid under us */
+	down_read(&shm_ids(sfd->ns).rwsem);
+
+	/* Check if we can map the segment */
+	shp = shm_lock(sfd->ns, sfd->id);
+	if (IS_ERR(shp)) {
+		ret = PTR_ERR(shp);
+		goto out;
+	}
+	ret = shp->shm_perm.mode & SHM_DEST ? -EINVAL : 0;
+	shm_unlock(shp);
+	if (ret)
+		goto out;
+
 	ret = sfd->file->f_op->mmap(sfd->file, vma);
 	if (ret != 0)
-		return ret;
+		goto out;
 	sfd->vm_ops = vma->vm_ops;
 #ifdef CONFIG_MMU
 	WARN_ON(!sfd->vm_ops->fault);
 #endif
 	vma->vm_ops = &shm_vm_ops;
 	shm_open(vma);
-
+out:
+	up_read(&shm_ids(sfd->ns).rwsem);
 	return ret;
 }
 
-- 
 Kirill A. Shutemov

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>