perf_event_mmap(vma) && !vma->vm_mm

perf_event_mmap(vma) && !vma->vm_mm

[Oleg Nesterov]

Hi,

I am fighting with uprobe bug, and perf_event_mmap() complicates
the problem, it is the only reason (afaics) why the forking task
can not do install_special_mapping(new_child_mm). This means that
the child should do this itself, say, from task_work_run() but
this way it can't handle the error if get_xol_area() fails, too
late to abort the already finished copy_process().

But please ignore, the only question is that I can't understand
this

	if (!vma->vm_mm) {
		name = strncpy(tmp, "[vdso]", sizeof(tmp));
		goto got_name;
	}

code in perf_event_mmap_event() and I am just curious. How it is
possible that vma->vm_mm == NULL ? perf_event_mmap(vma) is never
called with, say, vma == gate_vma. And even if it was possible
arch_vma_name() should handle this case?

Thanks,

Oleg.

Re: perf_event_mmap(vma) && !vma->vm_mm

[Peter Zijlstra]

On Sat, Oct 12, 2013 at 09:22:03PM +0200, Oleg Nesterov wrote:
> Hi,
> 
> I am fighting with uprobe bug, and perf_event_mmap() complicates
> the problem, it is the only reason (afaics) why the forking task
> can not do install_special_mapping(new_child_mm). This means that
> the child should do this itself, say, from task_work_run() but
> this way it can't handle the error if get_xol_area() fails, too
> late to abort the already finished copy_process().
> 
> But please ignore, the only question is that I can't understand
> this
> 
> 	if (!vma->vm_mm) {
> 		name = strncpy(tmp, "[vdso]", sizeof(tmp));
> 		goto got_name;
> 	}
> 
> code in perf_event_mmap_event() and I am just curious. How it is
> possible that vma->vm_mm == NULL ? perf_event_mmap(vma) is never
> called with, say, vma == gate_vma. And even if it was possible
> arch_vma_name() should handle this case?

Uuuhhhh... I wrote that didn't I ;-)

So I think that was due to the x86_32 gate_vma, but yes I don't think
we'd ever call perf_event_mmap() (perf_counter_mmap at the time) on it.

Also, the x86_32 arch_vma_name() didn't deal with the gate_vma (it still
doesn't appear to do so) as opposed to x86_64 which does.

But the main reason I added it was because task_mmu.c:show_map_vma() did
so too; I just wanted to be extra careful.

[PATCH 0/2] (Was: perf_event_mmap(vma) && !vma->vm_mm)

[Oleg Nesterov]

Peter,

you know, I was going to forget about this, but then I looked at
perf_event_mmap_event() again and I am even more puzzled.

I believe it asks for cleanups.

On 10/14, Peter Zijlstra wrote:
>
> On Sat, Oct 12, 2013 at 09:22:03PM +0200, Oleg Nesterov wrote:
> >
> > But please ignore, the only question is that I can't understand
> > this
> >
> > 	if (!vma->vm_mm) {
> > 		name = strncpy(tmp, "[vdso]", sizeof(tmp));
> > 		goto got_name;
> > 	}
> >
> > code in perf_event_mmap_event() and I am just curious. How it is
> > possible that vma->vm_mm == NULL ? perf_event_mmap(vma) is never
> > called with, say, vma == gate_vma. And even if it was possible
> > arch_vma_name() should handle this case?
>
> Uuuhhhh... I wrote that didn't I ;-)
>
> So I think that was due to the x86_32 gate_vma, but yes I don't think
> we'd ever call perf_event_mmap() (perf_counter_mmap at the time) on it.

OK, so I think this code should die, it only adds the confusion.

Note also that at least on x86_64 "[vdso]" is not correct (if !vma_mm
was possible), this adds more confusion.

> Also, the x86_32 arch_vma_name() didn't deal with the gate_vma (it still
> doesn't appear to do so) as opposed to x86_64 which does.

Hmm... I am looking into arch/x86/vdso/vdso32-setup.c, it seems it does.
But probably I missed something, this doesn't matter.

> But the main reason I added it was because task_mmu.c:show_map_vma() did
> so too; I just wanted to be extra careful.

Yes, but this code can actually hit gate_vma. (and I'd say that if some
arch/ has the global gate_vma-like vma's, it should implement arch_vma_name
correctly, but this is off-topic).

---------------------------------------------------------------------------
Please look at these 2 simple patches. Initially I was going to send
the 3rd patch, but I simply can't understand the "align" logic.

First of all, we surely do not need __GFP_ZERO for kzalloc(PATH_MAX),
even if we need to nullify the alignment. So I am going to send another
patch in any case.

But do we really need to nullify the extra bytes after strlen()? If yes,
for what? If no, we can simply do s/kzalloc/kmalloc/ and kill that
memset(tmp, 0, sizeof(tmp)) at the start.

Otoh. Why do we need the temporary string buffer (char tmp[16]) at all?
We either use the result from d_path() (which has a room), or we use a
string literal (may be returned by arch_vma_name), in the latter case
it is safe to assume we can read the extra 7 bytes from .data?

IOW. Could you explain why the patch below (on top of 1-2) is wrong?

Thanks,

Oleg.

--- x/kernel/events/core.c
+++ x/kernel/events/core.c
@@ -5098,19 +5098,16 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 	int maj = 0, min = 0;
 	u64 ino = 0, gen = 0;
 	unsigned int size;
-	char tmp[16];
 	char *buf = NULL;
 	const char *name;
 
-	memset(tmp, 0, sizeof(tmp));
-
 	if (file) {
 		struct inode *inode;
 		dev_t dev;
 
-		buf = kzalloc(PATH_MAX, GFP_KERNEL);
+		buf = kmalloc(PATH_MAX, GFP_KERNEL);
 		if (!buf) {
-			name = strncpy(tmp, "//enomem", sizeof(tmp));
+			name = "//enomem";
 			goto got_name;
 		}
 		/*
@@ -5120,7 +5117,7 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 		 */
 		name = d_path(&file->f_path, buf, PATH_MAX - sizeof(u64));
 		if (IS_ERR(name)) {
-			name = strncpy(tmp, "//toolong", sizeof(tmp));
+			name = "//toolong";
 			goto got_name;
 		}
 		inode = file_inode(vma->vm_file);
@@ -5133,23 +5130,21 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 	} else {
 		name = arch_vma_name(vma);
 		if (name) {
-			name = strncpy(tmp, name, sizeof(tmp) - 1);
-			tmp[sizeof(tmp) - 1] = '\0';
 			goto got_name;
 		}
 
 		if (vma->vm_start <= vma->vm_mm->start_brk &&
 				vma->vm_end >= vma->vm_mm->brk) {
-			name = strncpy(tmp, "[heap]", sizeof(tmp));
+			name = "[heap]";
 			goto got_name;
 		}
 		if (vma->vm_start <= vma->vm_mm->start_stack &&
 				vma->vm_end >= vma->vm_mm->start_stack) {
-			name = strncpy(tmp, "[stack]", sizeof(tmp));
+			name = "[stack]";
 			goto got_name;
 		}
 
-		name = strncpy(tmp, "//anon", sizeof(tmp));
+		name = "//anon";
 		goto got_name;
 	}
 

[PATCH 1/2] perf: Kill the dead !vma->vm_mm code in perf_event_mmap_event()

[Oleg Nesterov]

1. perf_event_mmap(vma) is never called with a gate_vma-like arg,
   remove the "if (!vma->vm_mm)" code.

2. arch_vma_name() can use the chached value of mmap_event->vma.

3. Change the code to not call arch_vma_name() twice.

4. Purely cosmetic, but since we use "goto got_name" all the time
   remove "else" from "[stack]" branch just for symmetry.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/core.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index cb4238e..e27d8f8 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -5130,21 +5130,19 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 		min = MINOR(dev);
 
 	} else {
-		if (arch_vma_name(mmap_event->vma)) {
-			name = strncpy(tmp, arch_vma_name(mmap_event->vma),
-				       sizeof(tmp) - 1);
+		name = arch_vma_name(vma);
+		if (name) {
+			name = strncpy(tmp, name, sizeof(tmp) - 1);
 			tmp[sizeof(tmp) - 1] = '\0';
 			goto got_name;
 		}
 
-		if (!vma->vm_mm) {
-			name = strncpy(tmp, "[vdso]", sizeof(tmp));
-			goto got_name;
-		} else if (vma->vm_start <= vma->vm_mm->start_brk &&
+		if (vma->vm_start <= vma->vm_mm->start_brk &&
 				vma->vm_end >= vma->vm_mm->brk) {
 			name = strncpy(tmp, "[heap]", sizeof(tmp));
 			goto got_name;
-		} else if (vma->vm_start <= vma->vm_mm->start_stack &&
+		}
+		if (vma->vm_start <= vma->vm_mm->start_stack &&
 				vma->vm_end >= vma->vm_mm->start_stack) {
 			name = strncpy(tmp, "[stack]", sizeof(tmp));
 			goto got_name;
-- 
1.5.5.1

[tip:perf/core] perf: Kill the dead !vma-> vm_mm code in perf_event_mmap_event()

[tip-bot for Oleg Nesterov]

Commit-ID:  32c5fb7e7d18b4fd37c5e29dea731151e9d66866
Gitweb:     http://git.kernel.org/tip/32c5fb7e7d18b4fd37c5e29dea731151e9d66866
Author:     Oleg Nesterov <oleg@redhat.com>
AuthorDate: Wed, 16 Oct 2013 22:09:45 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 29 Oct 2013 12:02:51 +0100

perf: Kill the dead !vma->vm_mm code in perf_event_mmap_event()

1. perf_event_mmap(vma) is never called with a gate_vma-like arg,
   remove the "if (!vma->vm_mm)" code.

2. arch_vma_name() can use the chached value of mmap_event->vma.

3. Change the code to not call arch_vma_name() twice.

4. Purely cosmetic, but since we use "goto got_name" all the time
   remove "else" from "[stack]" branch just for symmetry.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20131016200945.GB23214@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/events/core.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 028dad9..3ea5605 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -5136,21 +5136,19 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 		min = MINOR(dev);
 
 	} else {
-		if (arch_vma_name(mmap_event->vma)) {
-			name = strncpy(tmp, arch_vma_name(mmap_event->vma),
-				       sizeof(tmp) - 1);
+		name = arch_vma_name(vma);
+		if (name) {
+			name = strncpy(tmp, name, sizeof(tmp) - 1);
 			tmp[sizeof(tmp) - 1] = '\0';
 			goto got_name;
 		}
 
-		if (!vma->vm_mm) {
-			name = strncpy(tmp, "[vdso]", sizeof(tmp));
-			goto got_name;
-		} else if (vma->vm_start <= vma->vm_mm->start_brk &&
+		if (vma->vm_start <= vma->vm_mm->start_brk &&
 				vma->vm_end >= vma->vm_mm->brk) {
 			name = strncpy(tmp, "[heap]", sizeof(tmp));
 			goto got_name;
-		} else if (vma->vm_start <= vma->vm_mm->start_stack &&
+		}
+		if (vma->vm_start <= vma->vm_mm->start_stack &&
 				vma->vm_end >= vma->vm_mm->start_stack) {
 			name = strncpy(tmp, "[stack]", sizeof(tmp));
 			goto got_name;

[PATCH 2/2] perf: Do not waste PAGE_SIZE bytes for ALIGN(8) in perf_event_mmap_event()

[Oleg Nesterov]

perf_event_mmap_event() does kzalloc(PATH_MAX + sizeof(u64)) to
ensure we can align the size later. However this means that we
actually allocate PAGE_SIZE * 2 buffer, seems too much.

Change this code to allocate PATH_MAX==PAGE_SIZE bytes, but tell
d_path() to not use the last sizeof(u64) bytes.

Note: it is not clear why do we need __GFP_ZERO, see the next patch.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/core.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index e27d8f8..4a1e7b8 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -5107,17 +5107,18 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 	if (file) {
 		struct inode *inode;
 		dev_t dev;
+
+		buf = kzalloc(PATH_MAX, GFP_KERNEL);
+		if (!buf) {
+			name = strncpy(tmp, "//enomem", sizeof(tmp));
+			goto got_name;
+		}
 		/*
 		 * d_path works from the end of the rb backwards, so we
 		 * need to add enough zero bytes after the string to handle
 		 * the 64bit alignment we do later.
 		 */
-		buf = kzalloc(PATH_MAX + sizeof(u64), GFP_KERNEL);
-		if (!buf) {
-			name = strncpy(tmp, "//enomem", sizeof(tmp));
-			goto got_name;
-		}
-		name = d_path(&file->f_path, buf, PATH_MAX);
+		name = d_path(&file->f_path, buf, PATH_MAX - sizeof(u64));
 		if (IS_ERR(name)) {
 			name = strncpy(tmp, "//toolong", sizeof(tmp));
 			goto got_name;
-- 
1.5.5.1

[tip:perf/core] perf: Do not waste PAGE_SIZE bytes for ALIGN(8) in perf_event_mmap_event()

[tip-bot for Oleg Nesterov]

Commit-ID:  3ea2f2b96f9e636f49eb10962e96db3e19cab157
Gitweb:     http://git.kernel.org/tip/3ea2f2b96f9e636f49eb10962e96db3e19cab157
Author:     Oleg Nesterov <oleg@redhat.com>
AuthorDate: Wed, 16 Oct 2013 22:10:04 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 29 Oct 2013 12:02:52 +0100

perf: Do not waste PAGE_SIZE bytes for ALIGN(8) in perf_event_mmap_event()

perf_event_mmap_event() does kzalloc(PATH_MAX + sizeof(u64)) to
ensure we can align the size later. However this means that we
actually allocate PAGE_SIZE * 2 buffer, seems too much.

Change this code to allocate PATH_MAX==PAGE_SIZE bytes, but tell
d_path() to not use the last sizeof(u64) bytes.

Note: it is not clear why do we need __GFP_ZERO, see the next patch.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20131016201004.GC23214@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/events/core.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 3ea5605..b409e75 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -5113,17 +5113,18 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 	if (file) {
 		struct inode *inode;
 		dev_t dev;
-		/*
-		 * d_path works from the end of the rb backwards, so we
-		 * need to add enough zero bytes after the string to handle
-		 * the 64bit alignment we do later.
-		 */
-		buf = kzalloc(PATH_MAX + sizeof(u64), GFP_KERNEL);
+
+		buf = kzalloc(PATH_MAX, GFP_KERNEL);
 		if (!buf) {
 			name = strncpy(tmp, "//enomem", sizeof(tmp));
 			goto got_name;
 		}
-		name = d_path(&file->f_path, buf, PATH_MAX);
+		/*
+		 * d_path() works from the end of the rb backwards, so we
+		 * need to add enough zero bytes after the string to handle
+		 * the 64bit alignment we do later.
+		 */
+		name = d_path(&file->f_path, buf, PATH_MAX - sizeof(u64));
 		if (IS_ERR(name)) {
 			name = strncpy(tmp, "//toolong", sizeof(tmp));
 			goto got_name;

Re: [PATCH 0/2] (Was: perf_event_mmap(vma) && !vma->vm_mm)

[Peter Zijlstra]

On Wed, Oct 16, 2013 at 10:09:24PM +0200, Oleg Nesterov wrote:
> OK, so I think this code should die, it only adds the confusion.
> 
> Note also that at least on x86_64 "[vdso]" is not correct (if !vma_mm
> was possible), this adds more confusion.

Right, but x86_64 will return [vsyscall] and we'll never get there.

> > Also, the x86_32 arch_vma_name() didn't deal with the gate_vma (it still
> > doesn't appear to do so) as opposed to x86_64 which does.
> 
> Hmm... I am looking into arch/x86/vdso/vdso32-setup.c, it seems it does.
> But probably I missed something, this doesn't matter.

Right, vdso32-setup.c:arch_vma_name() never checks for vma ==
get_gate_vma(). So it would return NULL for the gate vma, not a proper
name like you argue it should.

> > But the main reason I added it was because task_mmu.c:show_map_vma() did
> > so too; I just wanted to be extra careful.
> 
> Yes, but this code can actually hit gate_vma. (and I'd say that if some
> arch/ has the global gate_vma-like vma's, it should implement arch_vma_name
> correctly, but this is off-topic).

I'd tend to agree with you there, but clearly this isn't/wasn't the case
and the generic code grew a fallback or so.

> ---------------------------------------------------------------------------
> Please look at these 2 simple patches. Initially I was going to send
> the 3rd patch, but I simply can't understand the "align" logic.
> 
> First of all, we surely do not need __GFP_ZERO for kzalloc(PATH_MAX),
> even if we need to nullify the alignment. So I am going to send another
> patch in any case.
> 
> But do we really need to nullify the extra bytes after strlen()? If yes,
> for what? If no, we can simply do s/kzalloc/kmalloc/ and kill that
> memset(tmp, 0, sizeof(tmp)) at the start.
> 
> Otoh. Why do we need the temporary string buffer (char tmp[16]) at all?
> We either use the result from d_path() (which has a room), or we use a
> string literal (may be returned by arch_vma_name), in the latter case
> it is safe to assume we can read the extra 7 bytes from .data?
> 
> IOW. Could you explain why the patch below (on top of 1-2) is wrong?

The perf buffer works in multiples of u64 (8 bytes), your proposed patch
gives a string shorter than size; remember:

	size = ALIGN(strlen(name)+1, sizeof(u64));

And therefore the copy into the buffer will access beyond the end of
string, copying god knows what into userspace.

So yes, we could go make sure all stings are proper multiples of 8 bytes
and pad with '\0' at the end, but the zalloc + strcpy was by far the
easiest way to not get it wrong and leak crap to userspace.

Re: [PATCH 0/2] (Was: perf_event_mmap(vma) && !vma->vm_mm)

[Oleg Nesterov]

On 10/16, Peter Zijlstra wrote:
>
> On Wed, Oct 16, 2013 at 10:09:24PM +0200, Oleg Nesterov wrote:
> > OK, so I think this code should die, it only adds the confusion.
> >
> > Note also that at least on x86_64 "[vdso]" is not correct (if !vma_mm
> > was possible), this adds more confusion.
>
> Right, but x86_64 will return [vsyscall] and we'll never get there.

Yes, this is what I meant. But again, this doesn't really matter because
perf_event_mmap() is always called with the "real" vma.

> > > Also, the x86_32 arch_vma_name() didn't deal with the gate_vma (it still
> > > doesn't appear to do so) as opposed to x86_64 which does.
> >
> > Hmm... I am looking into arch/x86/vdso/vdso32-setup.c, it seems it does.
> > But probably I missed something, this doesn't matter.
>
> Right, vdso32-setup.c:arch_vma_name() never checks for vma ==
> get_gate_vma().

But it should not? It checks vm_start/context.vdso and
VDSO_HIGH_BASE == FIXADDR_USER_START in this case?

> So it would return NULL for the gate vma, not a proper
> name like you argue it should.

See above... but again, this doesn't really matter even if I am (likely)
wrong.

> > > But the main reason I added it was because task_mmu.c:show_map_vma() did
> > > so too; I just wanted to be extra careful.
> >
> > Yes, but this code can actually hit gate_vma. (and I'd say that if some
> > arch/ has the global gate_vma-like vma's, it should implement arch_vma_name
> > correctly, but this is off-topic).
>
> I'd tend to agree with you there, but clearly this isn't/wasn't the case
> and the generic code grew a fallback or so.

Agreed, this was just the "off-topic" note.

> > ---------------------------------------------------------------------------
> > Please look at these 2 simple patches. Initially I was going to send
> > the 3rd patch, but I simply can't understand the "align" logic.
> >
> > First of all, we surely do not need __GFP_ZERO for kzalloc(PATH_MAX),
> > even if we need to nullify the alignment. So I am going to send another
> > patch in any case.
> >
> > But do we really need to nullify the extra bytes after strlen()? If yes,
> > for what? If no, we can simply do s/kzalloc/kmalloc/ and kill that
> > memset(tmp, 0, sizeof(tmp)) at the start.
> >
> > Otoh. Why do we need the temporary string buffer (char tmp[16]) at all?
> > We either use the result from d_path() (which has a room), or we use a
> > string literal (may be returned by arch_vma_name), in the latter case
> > it is safe to assume we can read the extra 7 bytes from .data?
> >
> > IOW. Could you explain why the patch below (on top of 1-2) is wrong?
>
> The perf buffer works in multiples of u64 (8 bytes),

Yes, yes, I see.

> your proposed patch
> gives a string shorter than size; remember:
>
> 	size = ALIGN(strlen(name)+1, sizeof(u64));
>
> And therefore the copy into the buffer will access beyond the end of
> string,

Yes. But this can only happen if this sting lives in .data, and

> copying god knows what into userspace.

This is what I can not understand. At all.

OK. To simplify the discussion, suppose that it is only called with
->vm_file != NULL and d_path() never fails.

Why do we need to nullify the extra bytes after the end of string? IOW,
why do we need kzalloc?

Yes, we can copy the garbage to the userspace. So what? This should not
matter at all. Exactly because userspace should never used the data after
the end of string, no?

Peter, just in case. I understand that this all is minor. Just I am
confused. And in any case I do think we do not need __GFP_ZERO when
it comes to PAGE_SIZE allocation.

Oleg.

Re: [PATCH 0/2] (Was: perf_event_mmap(vma) && !vma->vm_mm)

[Peter Zijlstra]

On Wed, Oct 16, 2013 at 10:43:48PM +0200, Oleg Nesterov wrote:
> Yes, we can copy the garbage to the userspace. So what? This should not
> matter at all. Exactly because userspace should never used the data after
> the end of string, no?

Security people tell me to _never_ leak anything. I'm not near creative
enough to exploit such a thing; but I am amazed at the techniques I do
read about at times. So its really a better safe than sorry thing.

Re: [PATCH 0/2] (Was: perf_event_mmap(vma) && !vma->vm_mm)

[Oleg Nesterov]

On 10/16, Peter Zijlstra wrote:
>
> On Wed, Oct 16, 2013 at 10:43:48PM +0200, Oleg Nesterov wrote:
> > Yes, we can copy the garbage to the userspace. So what? This should not
> > matter at all. Exactly because userspace should never used the data after
> > the end of string, no?
>
> Security people tell me to _never_ leak anything. I'm not near creative
> enough to exploit such a thing; but I am amazed at the techniques I do
> read about at times. So its really a better safe than sorry thing.

OK.

Still we can cleanup/simplify this. see the next email...

Oleg.

Re: [PATCH 0/2] (Was: perf_event_mmap(vma) && !vma->vm_mm)

[Peter Zijlstra]

On Wed, Oct 16, 2013 at 10:43:48PM +0200, Oleg Nesterov wrote:
> Peter, just in case. I understand that this all is minor. Just I am
> confused. And in any case I do think we do not need __GFP_ZERO when
> it comes to PAGE_SIZE allocation.

Yeah; we could probably avoid it; if only the strcpy functions would
return how many bytes they copied :/ Then we could create a nice little
helper to \0 the tail end for alignment.

Anyway, as long as the resulting code is obvious and makes it hard to
leak something I'm fine.

Re: [PATCH 0/2] (Was: perf_event_mmap(vma) && !vma->vm_mm)

[Oleg Nesterov]

On 10/16, Peter Zijlstra wrote:
>
> On Wed, Oct 16, 2013 at 10:43:48PM +0200, Oleg Nesterov wrote:
> > Peter, just in case. I understand that this all is minor. Just I am
> > confused. And in any case I do think we do not need __GFP_ZERO when
> > it comes to PAGE_SIZE allocation.
>
> Yeah; we could probably avoid it; if only the strcpy functions would
> return how many bytes they copied :/ Then we could create a nice little
> helper to \0 the tail end for alignment.
>
> Anyway, as long as the resulting code is obvious and makes it hard to
> leak something I'm fine.

OK. I'll wait for your review on this series, then send the next patch.

d_path() case can clear the extra bytes, everything else can use a single

	if (name != buf)
		name = str*copy(tmp, buf);

to avoid the leak.

Oleg.

Re: [PATCH 0/2] (Was: perf_event_mmap(vma) && !vma->vm_mm)

[Peter Zijlstra]

On Wed, Oct 16, 2013 at 10:58:00PM +0200, Oleg Nesterov wrote:
> OK. I'll wait for your review on this series, then send the next patch.
> 

Those two patches look good; thanks. How about something like so on top?

---
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -5103,18 +5103,16 @@ static void perf_event_mmap_event(struct
 	struct file *file = vma->vm_file;
 	int maj = 0, min = 0;
 	u64 ino = 0, gen = 0;
-	unsigned int size;
+	unsigned int size, len;
 	char tmp[16];
 	char *buf = NULL;
 	const char *name;
 
-	memset(tmp, 0, sizeof(tmp));
-
 	if (file) {
 		struct inode *inode;
 		dev_t dev;
 
-		buf = kzalloc(PATH_MAX, GFP_KERNEL);
+		buf = kmalloc(PATH_MAX, GFP_KERNEL);
 		if (!buf) {
 			name = strncpy(tmp, "//enomem", sizeof(tmp));
 			goto got_name;
@@ -5160,7 +5158,15 @@ static void perf_event_mmap_event(struct
 	}
 
 got_name:
-	size = ALIGN(strlen(name)+1, sizeof(u64));
+	/*
+	 * Since our buffer works in 8 byte units we need to align our string
+	 * size to a multiple of 8. However, we must guarantee the tail end is
+	 * zero'd out to avoid leaking random bits to userspace.
+	 */
+	len = strlen(name)+1;
+	size = ALIGN(len, sizeof(u64));
+	for (; len < size; len++)
+		name[len] = '\0';
 
 	mmap_event->file_name = name;
 	mmap_event->file_size = size;

Re: [PATCH 0/2] (Was: perf_event_mmap(vma) && !vma->vm_mm)

[Oleg Nesterov]

On 10/16, Peter Zijlstra wrote:
>
> On Wed, Oct 16, 2013 at 10:58:00PM +0200, Oleg Nesterov wrote:
> > OK. I'll wait for your review on this series, then send the next patch.
> >
>
> Those two patches look good; thanks.

Thanks, can I have your acks for Ingo ?

> How about something like so on top?
>
> ---
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -5103,18 +5103,16 @@ static void perf_event_mmap_event(struct
>  	struct file *file = vma->vm_file;
>  	int maj = 0, min = 0;
>  	u64 ino = 0, gen = 0;
> -	unsigned int size;
> +	unsigned int size, len;
>  	char tmp[16];
>  	char *buf = NULL;
>  	const char *name;
>  
> -	memset(tmp, 0, sizeof(tmp));
> -
>  	if (file) {
>  		struct inode *inode;
>  		dev_t dev;
>  
> -		buf = kzalloc(PATH_MAX, GFP_KERNEL);
> +		buf = kmalloc(PATH_MAX, GFP_KERNEL);
>  		if (!buf) {
>  			name = strncpy(tmp, "//enomem", sizeof(tmp));
>  			goto got_name;
> @@ -5160,7 +5158,15 @@ static void perf_event_mmap_event(struct
>  	}
>  
>  got_name:
> -	size = ALIGN(strlen(name)+1, sizeof(u64));
> +	/*
> +	 * Since our buffer works in 8 byte units we need to align our string
> +	 * size to a multiple of 8. However, we must guarantee the tail end is
> +	 * zero'd out to avoid leaking random bits to userspace.
> +	 */
> +	len = strlen(name)+1;
> +	size = ALIGN(len, sizeof(u64));
> +	for (; len < size; len++)
> +		name[len] = '\0';

Yes, this is almost what I meant, but:

	- name is "const char *", we need another variable

	- we do not really need "len", we can simply do

		size = strlen(name) + 1;
		while (size % sizeof(u64))
			name[size++] = '\0';

	  although I won't argue if you dislike "size & 7" in while().

	- we can factor out strncpy(tmp, name).

Could you look at 3/2 I'll send in a minute?

Oleg.

Re: [PATCH 0/2] (Was: perf_event_mmap(vma) && !vma->vm_mm)

[Oleg Nesterov]

On 10/17, Oleg Nesterov wrote:
>
> 	- we do not really need "len", we can simply do
>
> 		size = strlen(name) + 1;
> 		while (size % sizeof(u64))
> 			name[size++] = '\0';
>
> 	  although I won't argue if you dislike "size & 7" in while().

Or, perhaps,

	while (!IS_ALIGNED(size, sizeof(u64)))
		name[size++] = '\0';

to make it self-explanatory.

Oleg.

Re: [PATCH 0/2] (Was: perf_event_mmap(vma) && !vma->vm_mm)

[Peter Zijlstra]

On Thu, Oct 17, 2013 at 05:27:17PM +0200, Oleg Nesterov wrote:
> On 10/17, Oleg Nesterov wrote:
> >
> > 	- we do not really need "len", we can simply do
> >
> > 		size = strlen(name) + 1;
> > 		while (size % sizeof(u64))
> > 			name[size++] = '\0';
> >
> > 	  although I won't argue if you dislike "size & 7" in while().
> 
> Or, perhaps,
> 
> 	while (!IS_ALIGNED(size, sizeof(u64)))
> 		name[size++] = '\0';
> 

---
Subject: perf: Change zero-padding of strings in perf_event_mmap_event()
From: Peter Zijlstra <peterz@infradead.org>
Date: Thu Oct 17 00:06:46 CEST 2013

Oleg complained about the excessive 0-ing in perf_event_mmap_event(),
so try and be smarter about it while keeping it fairly fool proof and
avoid leaking random bits out to userspace.

Suggested-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/n/tip-8jirlm99m6if2z13wd6rbyu6@git.kernel.org
---
 kernel/events/core.c |   17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -5106,15 +5106,13 @@ static void perf_event_mmap_event(struct
 	unsigned int size;
 	char tmp[16];
 	char *buf = NULL;
-	const char *name;
-
-	memset(tmp, 0, sizeof(tmp));
+	char *name;
 
 	if (file) {
 		struct inode *inode;
 		dev_t dev;
 
-		buf = kzalloc(PATH_MAX, GFP_KERNEL);
+		buf = kmalloc(PATH_MAX, GFP_KERNEL);
 		if (!buf) {
 			name = strncpy(tmp, "//enomem", sizeof(tmp));
 			goto got_name;
@@ -5137,7 +5135,7 @@ static void perf_event_mmap_event(struct
 		min = MINOR(dev);
 
 	} else {
-		name = arch_vma_name(vma);
+		name = (char *)arch_vma_name(vma);
 		if (name) {
 			name = strncpy(tmp, name, sizeof(tmp) - 1);
 			tmp[sizeof(tmp) - 1] = '\0';
@@ -5160,7 +5158,14 @@ static void perf_event_mmap_event(struct
 	}
 
 got_name:
-	size = ALIGN(strlen(name)+1, sizeof(u64));
+	/*
+	 * Since our buffer works in 8 byte units we need to align our string
+	 * size to a multiple of 8. However, we must guarantee the tail end is
+	 * zero'd out to avoid leaking random bits to userspace.
+	 */
+	size = strlen(name)+1;
+	while (!IS_ALIGNED(size, sizeof(u64)))
+		name[size++] = '\0';
 
 	mmap_event->file_name = name;
 	mmap_event->file_size = size;

Re: [PATCH 0/2] (Was: perf_event_mmap(vma) && !vma->vm_mm)

[Oleg Nesterov]

On 10/17, Peter Zijlstra wrote:
>
> -		name = arch_vma_name(vma);
> +		name = (char *)arch_vma_name(vma);

Yes, this way we do not need another non-const ptr.

OK, please consider another cleanup on top of this change.

Just to "complete" the discussion, do not even bother to reply
if you think it doesn't make sense ;)

---
Subject: [PATCH] perf: Factor out strncpy() in perf_event_mmap_event()
From: Oleg Nesterov <oleg@redhat.com>
Date: Thu, 17 Oct 2013 20:04:17 +0200

While this is really minor, but strncpy() does the unnecessary
zero-padding till the end of tmp[16] and it is called every time
we are going to use the string literal.

Turn these strncpy()'s into the single strlcpy() under the new
label, saves 72 bytes.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/core.c |   32 ++++++++++++++++----------------
 1 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index e7ace78..93ddf3d 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -5108,8 +5108,8 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 
 		buf = kmalloc(PATH_MAX, GFP_KERNEL);
 		if (!buf) {
-			name = strncpy(tmp, "//enomem", sizeof(tmp));
-			goto got_name;
+			name = "//enomem";
+			goto cpy_name;
 		}
 		/*
 		 * d_path works from the end of the rb backwards, so we
@@ -5118,8 +5118,8 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 		 */
 		name = d_path(&file->f_path, buf, PATH_MAX - sizeof(u64));
 		if (IS_ERR(name)) {
-			name = strncpy(tmp, "//toolong", sizeof(tmp));
-			goto got_name;
+			name = "//toolong";
+			goto cpy_name;
 		}
 		inode = file_inode(vma->vm_file);
 		dev = inode->i_sb->s_dev;
@@ -5127,30 +5127,30 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 		gen = inode->i_generation;
 		maj = MAJOR(dev);
 		min = MINOR(dev);
-
+		goto got_name;
 	} else {
 		name = (char *)arch_vma_name(vma);
-		if (name) {
-			name = strncpy(tmp, name, sizeof(tmp) - 1);
-			tmp[sizeof(tmp) - 1] = '\0';
-			goto got_name;
-		}
+		if (name)
+			goto cpy_name;
 
 		if (vma->vm_start <= vma->vm_mm->start_brk &&
 				vma->vm_end >= vma->vm_mm->brk) {
-			name = strncpy(tmp, "[heap]", sizeof(tmp));
-			goto got_name;
+			name = "[heap]";
+			goto cpy_name;
 		}
 		if (vma->vm_start <= vma->vm_mm->start_stack &&
 				vma->vm_end >= vma->vm_mm->start_stack) {
-			name = strncpy(tmp, "[stack]", sizeof(tmp));
-			goto got_name;
+			name = "[stack]";
+			goto cpy_name;
 		}
 
-		name = strncpy(tmp, "//anon", sizeof(tmp));
-		goto got_name;
+		name = "//anon";
+		goto cpy_name;
 	}
 
+cpy_name:
+	strlcpy(tmp, name, sizeof(tmp));
+	name = tmp;
 got_name:
 	/*
 	 * Since our buffer works in 8 byte units we need to align our string
-- 
1.5.5.1

Re: [PATCH 0/2] (Was: perf_event_mmap(vma) && !vma->vm_mm)

[Peter Zijlstra]

On Thu, Oct 17, 2013 at 08:24:17PM +0200, Oleg Nesterov wrote:
> Subject: [PATCH] perf: Factor out strncpy() in perf_event_mmap_event()
> From: Oleg Nesterov <oleg@redhat.com>
> Date: Thu, 17 Oct 2013 20:04:17 +0200
> 
> While this is really minor, but strncpy() does the unnecessary
> zero-padding till the end of tmp[16] and it is called every time
> we are going to use the string literal.
> 
> Turn these strncpy()'s into the single strlcpy() under the new
> label, saves 72 bytes.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Sure why not.. applied, thanks!

[tip:perf/core] perf: Factor out strncpy() in perf_event_mmap_event()

[tip-bot for Oleg Nesterov]

Commit-ID:  c7e548b45ce85f765f6262149dd60d9956a31d60
Gitweb:     http://git.kernel.org/tip/c7e548b45ce85f765f6262149dd60d9956a31d60
Author:     Oleg Nesterov <oleg@redhat.com>
AuthorDate: Thu, 17 Oct 2013 20:24:17 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 6 Nov 2013 12:34:28 +0100

perf: Factor out strncpy() in perf_event_mmap_event()

While this is really minor, but strncpy() does the unnecessary
zero-padding till the end of tmp[16] and it is called every time
we are going to use the string literal.

Turn these strncpy()'s into the single strlcpy() under the new
label, saves 72 bytes.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20131017182417.GA17753@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/events/core.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 17b3c6c..4dc078d 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -5144,8 +5144,8 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 
 		buf = kmalloc(PATH_MAX, GFP_KERNEL);
 		if (!buf) {
-			name = strncpy(tmp, "//enomem", sizeof(tmp));
-			goto got_name;
+			name = "//enomem";
+			goto cpy_name;
 		}
 		/*
 		 * d_path() works from the end of the rb backwards, so we
@@ -5154,8 +5154,8 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 		 */
 		name = d_path(&file->f_path, buf, PATH_MAX - sizeof(u64));
 		if (IS_ERR(name)) {
-			name = strncpy(tmp, "//toolong", sizeof(tmp));
-			goto got_name;
+			name = "//toolong";
+			goto cpy_name;
 		}
 		inode = file_inode(vma->vm_file);
 		dev = inode->i_sb->s_dev;
@@ -5163,30 +5163,30 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 		gen = inode->i_generation;
 		maj = MAJOR(dev);
 		min = MINOR(dev);
-
+		goto got_name;
 	} else {
 		name = (char *)arch_vma_name(vma);
-		if (name) {
-			name = strncpy(tmp, name, sizeof(tmp) - 1);
-			tmp[sizeof(tmp) - 1] = '\0';
-			goto got_name;
-		}
+		if (name)
+			goto cpy_name;
 
 		if (vma->vm_start <= vma->vm_mm->start_brk &&
 				vma->vm_end >= vma->vm_mm->brk) {
-			name = strncpy(tmp, "[heap]", sizeof(tmp));
-			goto got_name;
+			name = "[heap]";
+			goto cpy_name;
 		}
 		if (vma->vm_start <= vma->vm_mm->start_stack &&
 				vma->vm_end >= vma->vm_mm->start_stack) {
-			name = strncpy(tmp, "[stack]", sizeof(tmp));
-			goto got_name;
+			name = "[stack]";
+			goto cpy_name;
 		}
 
-		name = strncpy(tmp, "//anon", sizeof(tmp));
-		goto got_name;
+		name = "//anon";
+		goto cpy_name;
 	}
 
+cpy_name:
+	strlcpy(tmp, name, sizeof(tmp));
+	name = tmp;
 got_name:
 	/*
 	 * Since our buffer works in 8 byte units we need to align our string

Re: [PATCH 0/2] (Was: perf_event_mmap(vma) && !vma->vm_mm)

[Peter Zijlstra]

On Thu, Oct 17, 2013 at 05:20:27PM +0200, Oleg Nesterov wrote:
> On 10/16, Peter Zijlstra wrote:
> >
> > On Wed, Oct 16, 2013 at 10:58:00PM +0200, Oleg Nesterov wrote:
> > > OK. I'll wait for your review on this series, then send the next patch.
> > >
> >
> > Those two patches look good; thanks.
> 
> Thanks, can I have your acks for Ingo ?

Oh, I queued them :-)

> Yes, this is almost what I meant, but:
> 
> 	- name is "const char *", we need another variable

I cased that away ;-)

> 	- we do not really need "len", we can simply do
> 
> 		size = strlen(name) + 1;
> 		while (size % sizeof(u64))
> 			name[size++] = '\0';
> 
> 	  although I won't argue if you dislike "size & 7" in while().

Cute..

> 	- we can factor out strncpy(tmp, name).
> 
> Could you look at 3/2 I'll send in a minute?

I'll got have a look :-)

[PATCH 3/2] perf: Optimize the fill/align code in perf_event_mmap_event()

[Oleg Nesterov]

1. memset(tmp, 0) and especially kzalloc(PATH_MAX) are suboptimal,
   we only need to zero-fill the alignment.

   Remove this memset/__GFP_ZERO and fill the extra bytes by hand.

2. The usage of strncpy(tmp) is not optimal too, we can add the new
   label and do this in one place.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/core.c |   47 +++++++++++++++++++++++------------------------
 1 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 4a1e7b8..777a268 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -5098,20 +5098,17 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 	int maj = 0, min = 0;
 	u64 ino = 0, gen = 0;
 	unsigned int size;
-	char tmp[16];
-	char *buf = NULL;
-	const char *name;
-
-	memset(tmp, 0, sizeof(tmp));
+	char tmp[16], *name, *buf = NULL;
+	const char *str;
 
 	if (file) {
 		struct inode *inode;
 		dev_t dev;
 
-		buf = kzalloc(PATH_MAX, GFP_KERNEL);
+		buf = kmalloc(PATH_MAX, GFP_KERNEL);
 		if (!buf) {
-			name = strncpy(tmp, "//enomem", sizeof(tmp));
-			goto got_name;
+			str = "//enomem";
+			goto cpy_name;
 		}
 		/*
 		 * d_path works from the end of the rb backwards, so we
@@ -5120,8 +5117,8 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 		 */
 		name = d_path(&file->f_path, buf, PATH_MAX - sizeof(u64));
 		if (IS_ERR(name)) {
-			name = strncpy(tmp, "//toolong", sizeof(tmp));
-			goto got_name;
+			str = "//toolong";
+			goto cpy_name;
 		}
 		inode = file_inode(vma->vm_file);
 		dev = inode->i_sb->s_dev;
@@ -5129,32 +5126,34 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event)
 		gen = inode->i_generation;
 		maj = MAJOR(dev);
 		min = MINOR(dev);
-
+		goto got_name;
 	} else {
-		name = arch_vma_name(vma);
-		if (name) {
-			name = strncpy(tmp, name, sizeof(tmp) - 1);
-			tmp[sizeof(tmp) - 1] = '\0';
-			goto got_name;
-		}
+		str = arch_vma_name(vma);
+		if (str)
+			goto cpy_name;
 
 		if (vma->vm_start <= vma->vm_mm->start_brk &&
 				vma->vm_end >= vma->vm_mm->brk) {
-			name = strncpy(tmp, "[heap]", sizeof(tmp));
-			goto got_name;
+			str = "[heap]";
+			goto cpy_name;
 		}
 		if (vma->vm_start <= vma->vm_mm->start_stack &&
 				vma->vm_end >= vma->vm_mm->start_stack) {
-			name = strncpy(tmp, "[stack]", sizeof(tmp));
-			goto got_name;
+			str = "[stack]";
+			goto cpy_name;
 		}
 
-		name = strncpy(tmp, "//anon", sizeof(tmp));
-		goto got_name;
+		str = "//anon";
+		goto cpy_name;
 	}
 
+cpy_name:
+	strlcpy(tmp, str, sizeof(tmp));
+	name = tmp;
 got_name:
-	size = ALIGN(strlen(name)+1, sizeof(u64));
+	size = strlen(name) + 1;
+	while (size % sizeof(u64))
+		name[size++] = '\0';
 
 	mmap_event->file_name = name;
 	mmap_event->file_size = size;
-- 
1.5.5.1