Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1440101 > unrolled thread

Re: [PATCHv2 3/6] x86/arch_prctl/vdso: add ARCH_MAP_VDSO_*

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCHv2 3/6] x86/arch_prctl/vdso: add ARCH_MAP_VDSO_* Andy Lutomirski <luto@amacapital.net> - 2016-07-10 14:50 +0200
    Re: [PATCHv2 3/6] x86/arch_prctl/vdso: add ARCH_MAP_VDSO_* Oleg Nesterov <oleg@redhat.com> - 2016-07-11 20:30 +0200
      Re: [PATCHv2 3/6] x86/arch_prctl/vdso: add ARCH_MAP_VDSO_* Andy Lutomirski <luto@amacapital.net> - 2016-07-11 20:30 +0200
        Re: [PATCHv2 3/6] x86/arch_prctl/vdso: add ARCH_MAP_VDSO_* Oleg Nesterov <oleg@redhat.com> - 2016-07-12 16:20 +0200

#1440101 — Re: [PATCHv2 3/6] x86/arch_prctl/vdso: add ARCH_MAP_VDSO_*

On Thu, Jul 7, 2016 at 4:11 AM, Dmitry Safonov <dsafonov@virtuozzo.com> wrote:
> On 07/06/2016 05:30 PM, Andy Lutomirski wrote:
>>
>> On Wed, Jun 29, 2016 at 3:57 AM, Dmitry Safonov <dsafonov@virtuozzo.com>
>> wrote:
>>>
>>> Add API to change vdso blob type with arch_prctl.
>>> As this is usefull only by needs of CRIU, expose
>>> this interface under CONFIG_CHECKPOINT_RESTORE.
>>
>>
>>> +#ifdef CONFIG_CHECKPOINT_RESTORE
>>> +       case ARCH_MAP_VDSO_X32:
>>> +               return do_map_vdso(VDSO_X32, addr, false);
>>> +       case ARCH_MAP_VDSO_32:
>>> +               return do_map_vdso(VDSO_32, addr, false);
>>> +       case ARCH_MAP_VDSO_64:
>>> +               return do_map_vdso(VDSO_64, addr, false);
>>> +#endif
>>> +
>>
>>
>> This will have an odd side effect: if the old mapping is still around,
>> its .fault will start behaving erratically.  I wonder if we can either
>> reliably zap the old vma (or check that it's not there any more)
>> before mapping a new one or whether we can associate the vdso image
>> with the vma (possibly by having a separate vm_special_mapping for
>> each vdso_image.  The latter is quite easy: change vdso_image to embed
>> vm_special_mapping and use container_of in vdso_fault to fish the
>> vdso_image back out.  But we'd have to embed another
>> vm_special_mapping for the vvar mapping as well for the same reason.
>>
>> I'm also a bit concerned that __install_special_mapping might not get
>> all the cgroup and rlimit stuff right.  If we ensure that any old
>> mappings are gone, then the damage is bounded, but otherwise someone
>> might call this in a loop and fill their address space with arbitrary
>> numbers of special mappings.
>
>
> Well, I have deleted code that unmaps old vdso because I didn't saw
> a reason why it's bad and wanted to reduce code. But well, now I do see
> reasons, thanks.
>
> Hmm, what do you think if I do it a little different way then embedding
> vm_special_mapping: just that old hack with vma_ops. If I add a close()
> hook there and make there context.vdso = NULL pointer, then I can test
> it on remap. This can also have nice feature as restricting partial
> munmap of vdso blob. Is this sounds sane?

I think so, as long as you do something to make sure that vvar gets
unmapped as well.

Oleg, want to sanity-check us?  Do you believe that if .mremap ensures
that only entire vma can be remapped and .close ensures that only the
whole vma can be unmapped, are we okay?  Or will we have issues with
mprotect?


-- 
Andy Lutomirski
AMA Capital Management, LLC

[toc] | [next] | [standalone]

#1440809

On 07/10, Andy Lutomirski wrote:
>
> On Thu, Jul 7, 2016 at 4:11 AM, Dmitry Safonov <dsafonov@virtuozzo.com> wrote:
> > On 07/06/2016 05:30 PM, Andy Lutomirski wrote:
> >>
> >> On Wed, Jun 29, 2016 at 3:57 AM, Dmitry Safonov <dsafonov@virtuozzo.com>
> >> wrote:
> >>>
> >>> Add API to change vdso blob type with arch_prctl.
> >>> As this is usefull only by needs of CRIU, expose
> >>> this interface under CONFIG_CHECKPOINT_RESTORE.
> >>
> >>
> >>> +#ifdef CONFIG_CHECKPOINT_RESTORE
> >>> +       case ARCH_MAP_VDSO_X32:
> >>> +               return do_map_vdso(VDSO_X32, addr, false);
> >>> +       case ARCH_MAP_VDSO_32:
> >>> +               return do_map_vdso(VDSO_32, addr, false);
> >>> +       case ARCH_MAP_VDSO_64:
> >>> +               return do_map_vdso(VDSO_64, addr, false);
> >>> +#endif
> >>> +
> >>
> >>
> >> This will have an odd side effect: if the old mapping is still around,
> >> its .fault will start behaving erratically.

Yes but I am not sure I fully understand your concerns, so let me ask...

Do we really care? I mean, the kernel can't crash or something like this,
just the old vdso mapping can faultin the "wrong" page from the new
vdso_image, right?

The user of prctl(ARCH_MAP_VDSO) should understand what it does and unmap
the old vdso anyway.

> >> I wonder if we can either
> >> reliably zap the old vma (or check that it's not there any more)
> >> before mapping a new one

However, I think this is right anyway, please see below...

> >> or whether we can associate the vdso image
> >> with the vma (possibly by having a separate vm_special_mapping for
> >> each vdso_image.

Yes, I too thought it would be nice to do this, regardless.

But as you said we probably want to limit the numbet of special mappings
an application can create:

> >> I'm also a bit concerned that __install_special_mapping might not get
> >> all the cgroup and rlimit stuff right.  If we ensure that any old
> >> mappings are gone, then the damage is bounded, but otherwise someone
> >> might call this in a loop and fill their address space with arbitrary
> >> numbers of special mappings.

I think you are right, we should not allow user-space to abuse the special
mappings. Even if iiuc in this case only RLIMIT_AS does matter...

> Oleg, want to sanity-check us?  Do you believe that if .mremap ensures
> that only entire vma can be remapped

Yes I think this makes sense. And damn we should kill arch_remap() ;)

> and .close ensures that only the
> whole vma can be unmapped,

How? It can't return the error.

And do_munmap() doesn't necessarily call ->close(),

> Or will we have issues with
> mprotect?

Yes, __split_vma() doesn't call ->close() too. ->open() can't help...

So it seems that we should do this by hand somehow. But in fact, what
I actually think right now is that I am totally confused and got lost ;)

Oleg.

[toc] | [prev] | [next] | [standalone]

#1440810

On Mon, Jul 11, 2016 at 11:26 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 07/10, Andy Lutomirski wrote:
>>
>> On Thu, Jul 7, 2016 at 4:11 AM, Dmitry Safonov <dsafonov@virtuozzo.com> wrote:
>> > On 07/06/2016 05:30 PM, Andy Lutomirski wrote:
>> >>
>> >> On Wed, Jun 29, 2016 at 3:57 AM, Dmitry Safonov <dsafonov@virtuozzo.com>
>> >> wrote:
>> >>>
>> >>> Add API to change vdso blob type with arch_prctl.
>> >>> As this is usefull only by needs of CRIU, expose
>> >>> this interface under CONFIG_CHECKPOINT_RESTORE.
>> >>
>> >>
>> >>> +#ifdef CONFIG_CHECKPOINT_RESTORE
>> >>> +       case ARCH_MAP_VDSO_X32:
>> >>> +               return do_map_vdso(VDSO_X32, addr, false);
>> >>> +       case ARCH_MAP_VDSO_32:
>> >>> +               return do_map_vdso(VDSO_32, addr, false);
>> >>> +       case ARCH_MAP_VDSO_64:
>> >>> +               return do_map_vdso(VDSO_64, addr, false);
>> >>> +#endif
>> >>> +
>> >>
>> >>
>> >> This will have an odd side effect: if the old mapping is still around,
>> >> its .fault will start behaving erratically.
>
> Yes but I am not sure I fully understand your concerns, so let me ask...
>
> Do we really care? I mean, the kernel can't crash or something like this,
> just the old vdso mapping can faultin the "wrong" page from the new
> vdso_image, right?

That makes me nervous.  IMO a mapping should have well-defined
semantics.  If nothing else, could be really messy if the list of
pages were wrong.

My real concern is DoS: I doubt that __install_special_mapping gets
all the accounting right.

>
> The user of prctl(ARCH_MAP_VDSO) should understand what it does and unmap
> the old vdso anyway.
>
>> >> I wonder if we can either
>> >> reliably zap the old vma (or check that it's not there any more)
>> >> before mapping a new one
>
> However, I think this is right anyway, please see below...
>
>> >> or whether we can associate the vdso image
>> >> with the vma (possibly by having a separate vm_special_mapping for
>> >> each vdso_image.
>
> Yes, I too thought it would be nice to do this, regardless.
>
> But as you said we probably want to limit the numbet of special mappings
> an application can create:
>
>> >> I'm also a bit concerned that __install_special_mapping might not get
>> >> all the cgroup and rlimit stuff right.  If we ensure that any old
>> >> mappings are gone, then the damage is bounded, but otherwise someone
>> >> might call this in a loop and fill their address space with arbitrary
>> >> numbers of special mappings.
>
> I think you are right, we should not allow user-space to abuse the special
> mappings. Even if iiuc in this case only RLIMIT_AS does matter...
>
>> Oleg, want to sanity-check us?  Do you believe that if .mremap ensures
>> that only entire vma can be remapped
>
> Yes I think this makes sense. And damn we should kill arch_remap() ;)
>
>> and .close ensures that only the
>> whole vma can be unmapped,
>
> How? It can't return the error.
>
> And do_munmap() doesn't necessarily call ->close(),
>
>> Or will we have issues with
>> mprotect?
>
> Yes, __split_vma() doesn't call ->close() too. ->open() can't help...
>
> So it seems that we should do this by hand somehow. But in fact, what
> I actually think right now is that I am totally confused and got lost ;)

I'm starting to wonder if we should finally suck it up and give
special mappings a non-NULL vm_file so we can track them properly.
Oleg, weren't you thinking of doing that for some other reason?

--Andy

[toc] | [prev] | [next] | [standalone]

#1441392

On 07/11, Andy Lutomirski wrote:
>
> On Mon, Jul 11, 2016 at 11:26 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > Do we really care? I mean, the kernel can't crash or something like this,
> > just the old vdso mapping can faultin the "wrong" page from the new
> > vdso_image, right?
>
> That makes me nervous.  IMO a mapping should have well-defined
> semantics.

Perhaps. but map_vdso() will be special anyway, it also changes ->vdso.

For example, if a 32-bit application calls prctl(ARCH_MAP_VDSO) from a
signal handler and we unmap the old vdso mapping, it will crash later
trying to call the (unmapped) restorer == kernel_rt_sigreturn.

> If nothing else, could be really messy if the list of
> pages were wrong.

I do not see anything really wrong, but I can easily miss something.

And don't get me wrong, I agree that any cleanup (say, associate vdso
image with vma) makes sense.

> My real concern is DoS: I doubt that __install_special_mapping gets
> all the accounting right.

Yes, and if it was not clear I fully agree. Even if we forget about the
accounting, I feel that special mappings must not be abused by userspace.

> > So it seems that we should do this by hand somehow. But in fact, what
> > I actually think right now is that I am totally confused and got lost ;)
>
> I'm starting to wonder if we should finally suck it up and give
> special mappings a non-NULL vm_file so we can track them properly.
> Oleg, weren't you thinking of doing that for some other reason?

Yes, uprobes. Currently we can't probe vdso page(s).

Oleg.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web