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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/