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

Groups > linux.kernel > #1398286 > unrolled thread

Getting rid of dynamic TASK_SIZE (on x86, at least)

Back to article view | Back to linux.kernel

Contents

  Getting rid of dynamic TASK_SIZE (on x86, at least) Andy Lutomirski <luto@amacapital.net> - 2016-05-10 18:10 +0200
    Re: Getting rid of dynamic TASK_SIZE (on x86, at least) Cyrill Gorcunov <gorcunov@gmail.com> - 2016-05-10 18:40 +0200
      Re: Getting rid of dynamic TASK_SIZE (on x86, at least) Andy Lutomirski <luto@amacapital.net> - 2016-05-10 18:50 +0200
        Re: Getting rid of dynamic TASK_SIZE (on x86, at least) Cyrill Gorcunov <gorcunov@gmail.com> - 2016-05-10 19:10 +0200
          Re: Getting rid of dynamic TASK_SIZE (on x86, at least) Andy Lutomirski <luto@amacapital.net> - 2016-05-10 19:30 +0200
            Re: Getting rid of dynamic TASK_SIZE (on x86, at least) Cyrill Gorcunov <gorcunov@gmail.com> - 2016-05-10 19:50 +0200
              Re: Getting rid of dynamic TASK_SIZE (on x86, at least) Andy Lutomirski <luto@amacapital.net> - 2016-05-10 23:20 +0200
                Re: Getting rid of dynamic TASK_SIZE (on x86, at least) Cyrill Gorcunov <gorcunov@gmail.com> - 2016-05-11 08:10 +0200
    Re: Getting rid of dynamic TASK_SIZE (on x86, at least) Oleg Nesterov <oleg@redhat.com> - 2016-05-10 20:30 +0200
      Re: Getting rid of dynamic TASK_SIZE (on x86, at least) Andy Lutomirski <luto@amacapital.net> - 2016-05-10 22:40 +0200
        Re: Getting rid of dynamic TASK_SIZE (on x86, at least) Oleg Nesterov <oleg@redhat.com> - 2016-05-11 20:10 +0200

#1398286 — Getting rid of dynamic TASK_SIZE (on x86, at least)

Hi all-

I'm trying to get rid of x86's dynamic TASK_SIZE and just redefine it
to TASK_SIZE_MAX.  So far, these are the TASK_SIZE users that actually
seem to care about the task in question:

get_unmapped_area.  This is used by mmap, mremap, exec, uprobe XOL,
and maybe some other things.

 - mmap, mremap, etc: IMO this should check in_compat_syscall, not
TIF_ADDR32.  If a 64-bit task does an explicit 32-bit mmap (using int
$0x80, for example), it should get a 32-bit address back.

 - xol_add_vma: This one is weird: uprobes really is doing something
behind the task's back, and the addresses need to be consistent with
the address width.  I'm not quite sure what to do here.

 - exec.  This wants to set up mappings that are appropriate for the new task.

My inclination would be add a new 'limit' parameter to all the
get_unmapped_area variants and possible to vm_brk and friends and to
thus push the decision into the callers.  For the syscalls, we could
add:

static inline unsigned long this_syscall_addr_limit(void) { return TASK_SIZE; }

and override it on x86.

I'm not super excited to write that patch, though...

--Andy

[toc] | [next] | [standalone]

#1398311

On Tue, May 10, 2016 at 09:07:49AM -0700, Andy Lutomirski wrote:
> Hi all-
> 
> I'm trying to get rid of x86's dynamic TASK_SIZE and just redefine it
> to TASK_SIZE_MAX.  So far, these are the TASK_SIZE users that actually
> seem to care about the task in question:
> 
> get_unmapped_area.  This is used by mmap, mremap, exec, uprobe XOL,
> and maybe some other things.
> 
>  - mmap, mremap, etc: IMO this should check in_compat_syscall, not
> TIF_ADDR32.  If a 64-bit task does an explicit 32-bit mmap (using int
> $0x80, for example), it should get a 32-bit address back.
> 
>  - xol_add_vma: This one is weird: uprobes really is doing something
> behind the task's back, and the addresses need to be consistent with
> the address width.  I'm not quite sure what to do here.
> 
>  - exec.  This wants to set up mappings that are appropriate for the new task.
> 
> My inclination would be add a new 'limit' parameter to all the
> get_unmapped_area variants and possible to vm_brk and friends and to
> thus push the decision into the callers.  For the syscalls, we could
> add:
> 
> static inline unsigned long this_syscall_addr_limit(void) { return TASK_SIZE; }
> 
> and override it on x86.
> 
> I'm not super excited to write that patch, though...

Andy, could you please highlight what's wrong with TASK_SIZE helper
in first place? The idea behind is to clean up the code or there
some real problem?

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

#1398317

On Tue, May 10, 2016 at 9:30 AM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> On Tue, May 10, 2016 at 09:07:49AM -0700, Andy Lutomirski wrote:
>> Hi all-
>>
>> I'm trying to get rid of x86's dynamic TASK_SIZE and just redefine it
>> to TASK_SIZE_MAX.  So far, these are the TASK_SIZE users that actually
>> seem to care about the task in question:
>>
>> get_unmapped_area.  This is used by mmap, mremap, exec, uprobe XOL,
>> and maybe some other things.
>>
>>  - mmap, mremap, etc: IMO this should check in_compat_syscall, not
>> TIF_ADDR32.  If a 64-bit task does an explicit 32-bit mmap (using int
>> $0x80, for example), it should get a 32-bit address back.
>>
>>  - xol_add_vma: This one is weird: uprobes really is doing something
>> behind the task's back, and the addresses need to be consistent with
>> the address width.  I'm not quite sure what to do here.
>>
>>  - exec.  This wants to set up mappings that are appropriate for the new task.
>>
>> My inclination would be add a new 'limit' parameter to all the
>> get_unmapped_area variants and possible to vm_brk and friends and to
>> thus push the decision into the callers.  For the syscalls, we could
>> add:
>>
>> static inline unsigned long this_syscall_addr_limit(void) { return TASK_SIZE; }
>>
>> and override it on x86.
>>
>> I'm not super excited to write that patch, though...
>
> Andy, could you please highlight what's wrong with TASK_SIZE helper
> in first place? The idea behind is to clean up the code or there
> some real problem?

It's annoying and ugly.  It also makes the idea of doing 32-bit CRIU
restore by starting in 64-bit mode and switching to 32-bit more
complicated because it requires switching TASK_SIZE.

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC

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

#1398330

On Tue, May 10, 2016 at 09:45:34AM -0700, Andy Lutomirski wrote:
> On Tue, May 10, 2016 at 9:30 AM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> > On Tue, May 10, 2016 at 09:07:49AM -0700, Andy Lutomirski wrote:
> >> Hi all-
> >>
> >> I'm trying to get rid of x86's dynamic TASK_SIZE and just redefine it
> >> to TASK_SIZE_MAX.  So far, these are the TASK_SIZE users that actually
> >> seem to care about the task in question:
> >>
> >> get_unmapped_area.  This is used by mmap, mremap, exec, uprobe XOL,
> >> and maybe some other things.
> >>
> >>  - mmap, mremap, etc: IMO this should check in_compat_syscall, not
> >> TIF_ADDR32.  If a 64-bit task does an explicit 32-bit mmap (using int
> >> $0x80, for example), it should get a 32-bit address back.
> >>
> >>  - xol_add_vma: This one is weird: uprobes really is doing something
> >> behind the task's back, and the addresses need to be consistent with
> >> the address width.  I'm not quite sure what to do here.
> >>
> >>  - exec.  This wants to set up mappings that are appropriate for the new task.
> >>
> >> My inclination would be add a new 'limit' parameter to all the
> >> get_unmapped_area variants and possible to vm_brk and friends and to
> >> thus push the decision into the callers.  For the syscalls, we could
> >> add:
> >>
> >> static inline unsigned long this_syscall_addr_limit(void) { return TASK_SIZE; }
> >>
> >> and override it on x86.
> >>
> >> I'm not super excited to write that patch, though...
> >
> > Andy, could you please highlight what's wrong with TASK_SIZE helper
> > in first place? The idea behind is to clean up the code or there
> > some real problem?
> 
> It's annoying and ugly.  It also makes the idea of doing 32-bit CRIU
> restore by starting in 64-bit mode and switching to 32-bit more
> complicated because it requires switching TASK_SIZE.

Well, you know I'm not sure it's that annoying. It serves as it should
for task limit. Sure we can add one more parameter into get-unmapped-addr
but same time the task-size will be present in say page faulting code
(the helper might be renamed but it will be here still). Same applies
to arch_get_unmapped_area_topdown, should there be some argument
passed instead of open-coded TASK_SIZE helper?

Don't get me wrong please, just trying to figure out how many code
places need to be patche if we start this procedure.

As to starting restore in 64 bit and switch into 32 bit -- should
not we simply scan for "current" memory map and test if all areas
mapped belong to compat limit? And that's all. (Sorry I didn't
follow precisely on your and Dmitry's conversation so I quite
probably missing something obvious here).

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

#1398349

On Tue, May 10, 2016 at 10:05 AM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> On Tue, May 10, 2016 at 09:45:34AM -0700, Andy Lutomirski wrote:
>> On Tue, May 10, 2016 at 9:30 AM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>> > On Tue, May 10, 2016 at 09:07:49AM -0700, Andy Lutomirski wrote:
>> >> Hi all-
>> >>
>> >> I'm trying to get rid of x86's dynamic TASK_SIZE and just redefine it
>> >> to TASK_SIZE_MAX.  So far, these are the TASK_SIZE users that actually
>> >> seem to care about the task in question:
>> >>
>> >> get_unmapped_area.  This is used by mmap, mremap, exec, uprobe XOL,
>> >> and maybe some other things.
>> >>
>> >>  - mmap, mremap, etc: IMO this should check in_compat_syscall, not
>> >> TIF_ADDR32.  If a 64-bit task does an explicit 32-bit mmap (using int
>> >> $0x80, for example), it should get a 32-bit address back.
>> >>
>> >>  - xol_add_vma: This one is weird: uprobes really is doing something
>> >> behind the task's back, and the addresses need to be consistent with
>> >> the address width.  I'm not quite sure what to do here.
>> >>
>> >>  - exec.  This wants to set up mappings that are appropriate for the new task.
>> >>
>> >> My inclination would be add a new 'limit' parameter to all the
>> >> get_unmapped_area variants and possible to vm_brk and friends and to
>> >> thus push the decision into the callers.  For the syscalls, we could
>> >> add:
>> >>
>> >> static inline unsigned long this_syscall_addr_limit(void) { return TASK_SIZE; }
>> >>
>> >> and override it on x86.
>> >>
>> >> I'm not super excited to write that patch, though...
>> >
>> > Andy, could you please highlight what's wrong with TASK_SIZE helper
>> > in first place? The idea behind is to clean up the code or there
>> > some real problem?
>>
>> It's annoying and ugly.  It also makes the idea of doing 32-bit CRIU
>> restore by starting in 64-bit mode and switching to 32-bit more
>> complicated because it requires switching TASK_SIZE.
>
> Well, you know I'm not sure it's that annoying. It serves as it should
> for task limit. Sure we can add one more parameter into get-unmapped-addr
> but same time the task-size will be present in say page faulting code
> (the helper might be renamed but it will be here still).

Why should the page faulting code care at all what type of task it is?
 If there's a vma there, fault it in.  If there isn't, then don't.

> Same applies
> to arch_get_unmapped_area_topdown, should there be some argument
> passed instead of open-coded TASK_SIZE helper?
>
> Don't get me wrong please, just trying to figure out how many code
> places need to be patche if we start this procedure.
>
> As to starting restore in 64 bit and switch into 32 bit -- should
> not we simply scan for "current" memory map and test if all areas
> mapped belong to compat limit?

I don't see what's wrong with leaving a high vma around.  The task is
unlikely to use it, but, if the task does use it (via long jump, for
example), it'll worj.

> And that's all. (Sorry I didn't
> follow precisely on your and Dmitry's conversation so I quite
> probably missing something obvious here).

It's not all.  We'd need an API to allow the task to cause TASK_SIZE
to change from TASK_SIZE64 to TASK_SIZE32.  I don't want to add that
API because I think its sole purpose is to work around kernel
silliness, and I'd rather we just fixed the silliness.

--Andy

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

#1398361

On Tue, May 10, 2016 at 10:26:05AM -0700, Andy Lutomirski wrote:
...
> >>
> >> It's annoying and ugly.  It also makes the idea of doing 32-bit CRIU
> >> restore by starting in 64-bit mode and switching to 32-bit more
> >> complicated because it requires switching TASK_SIZE.
> >
> > Well, you know I'm not sure it's that annoying. It serves as it should
> > for task limit. Sure we can add one more parameter into get-unmapped-addr
> > but same time the task-size will be present in say page faulting code
> > (the helper might be renamed but it will be here still).
> 
> Why should the page faulting code care at all what type of task it is?
> If there's a vma there, fault it in.  If there isn't, then don't.

__bad_area_nosemaphore
  ...
		/* Kernel addresses are always protection faults: */
		if (address >= TASK_SIZE)
			error_code |= PF_PROT;

For sure page faulting must consider what kind of fault is it.
Or we gonna drop such code at all?

> > Same applies
> > to arch_get_unmapped_area_topdown, should there be some argument
> > passed instead of open-coded TASK_SIZE helper?
> >
> > Don't get me wrong please, just trying to figure out how many code
> > places need to be patche if we start this procedure.
> >
> > As to starting restore in 64 bit and switch into 32 bit -- should
> > not we simply scan for "current" memory map and test if all areas
> > mapped belong to compat limit?
> 
> I don't see what's wrong with leaving a high vma around.  The task is
> unlikely to use it, but, if the task does use it (via long jump, for
> example), it'll worj.

True, from cpu perspective there is nothing wrong if in compat
(kernel compat) mode some memory slabs get left. Just thought
at first iteration we wanted unchanged behaviour.

> > And that's all. (Sorry I didn't
> > follow precisely on your and Dmitry's conversation so I quite
> > probably missing something obvious here).
> 
> It's not all.  We'd need an API to allow the task to cause TASK_SIZE
> to change from TASK_SIZE64 to TASK_SIZE32.  I don't want to add that
> API because I think its sole purpose is to work around kernel
> silliness, and I'd rather we just fixed the silliness.

I implied the change of task-size. Anyway, I see what you mean, thanks
for clarification. Still I think we won't be able to completely
replace task-size with task-size-mask. Some places such as base
for elf-dynload use it as a part of api (not directly though),
and at least in load_elf_binary the choose of base address should
be preserved.

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

#1398533

On Tue, May 10, 2016 at 10:49 AM, Cyrill Gorcunov <gorcunov@gmail.com> wrote:
> On Tue, May 10, 2016 at 10:26:05AM -0700, Andy Lutomirski wrote:
> ...
>> >>
>> >> It's annoying and ugly.  It also makes the idea of doing 32-bit CRIU
>> >> restore by starting in 64-bit mode and switching to 32-bit more
>> >> complicated because it requires switching TASK_SIZE.
>> >
>> > Well, you know I'm not sure it's that annoying. It serves as it should
>> > for task limit. Sure we can add one more parameter into get-unmapped-addr
>> > but same time the task-size will be present in say page faulting code
>> > (the helper might be renamed but it will be here still).
>>
>> Why should the page faulting code care at all what type of task it is?
>> If there's a vma there, fault it in.  If there isn't, then don't.
>
> __bad_area_nosemaphore
>   ...
>                 /* Kernel addresses are always protection faults: */
>                 if (address >= TASK_SIZE)
>                         error_code |= PF_PROT;
>
> For sure page faulting must consider what kind of fault is it.
> Or we gonna drop such code at all?

That code was bogus.  (Well, it was correct unless user code had a way
to create a funny high mapping in an otherwise 32-bit task, but it
still should have been TASK_SIZE_MAX.)  Fix sent.

--Andy

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

#1398703

On Tue, May 10, 2016 at 02:11:41PM -0700, Andy Lutomirski wrote:
 >
> > For sure page faulting must consider what kind of fault is it.
> > Or we gonna drop such code at all?
> 
> That code was bogus.  (Well, it was correct unless user code had a way
> to create a funny high mapping in an otherwise 32-bit task, but it
> still should have been TASK_SIZE_MAX.)  Fix sent.

OK, great!

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

#1398382

On 05/10, Andy Lutomirski wrote:
>
>  - xol_add_vma: This one is weird: uprobes really is doing something
> behind the task's back, and the addresses need to be consistent with
> the address width.  I'm not quite sure what to do here.

It can use mm->task_size instead, plus this is just a hint. And perhaps
mm->task_size should have more users, say get_unmapped_area...

Not sure we should really get rid of dynamic TASK_SIZE completely, but
personally I agree it looks a bit ugly.

Oleg.

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

#1398512

On May 10, 2016 11:21 AM, "Oleg Nesterov" <oleg@redhat.com> wrote:
>
> On 05/10, Andy Lutomirski wrote:
> >
> >  - xol_add_vma: This one is weird: uprobes really is doing something
> > behind the task's back, and the addresses need to be consistent with
> > the address width.  I'm not quite sure what to do here.
>
> It can use mm->task_size instead, plus this is just a hint. And perhaps
> mm->task_size should have more users, say get_unmapped_area...

Ick.  I hadn't noticed mm->task_size.  We have a *lot* of different
indicators of task size.  mm->task_size appears to have basically no
useful uses except maybe for ppc.

On x86, bitness can change without telling the kernel, and tasks
running in 64-bit mode can do 32-bit syscalls.

So maybe I should add mm->task_size to my list of things that would be
nice to remove.  Or maybe I'm just tilting at windmills.

>
> Not sure we should really get rid of dynamic TASK_SIZE completely, but
> personally I agree it looks a bit ugly.
>
> Oleg.
>

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

#1399397

On 05/10, Andy Lutomirski wrote:
>
> On May 10, 2016 11:21 AM, "Oleg Nesterov" <oleg@redhat.com> wrote:
> >
> > On 05/10, Andy Lutomirski wrote:
> > >
> > >  - xol_add_vma: This one is weird: uprobes really is doing something
> > > behind the task's back, and the addresses need to be consistent with
> > > the address width.  I'm not quite sure what to do here.
> >
> > It can use mm->task_size instead, plus this is just a hint. And perhaps
> > mm->task_size should have more users, say get_unmapped_area...
>
> Ick.  I hadn't noticed mm->task_size.  We have a *lot* of different
> indicators of task size.  mm->task_size appears to have basically no
> useful uses except maybe for ppc.
>
> On x86, bitness can change without telling the kernel, and tasks
> running in 64-bit mode can do 32-bit syscalls.

Sure, but imo this doesn't mean that mm->task_size or (say) is_64bit_mm()
make no sense.

> So maybe I should add mm->task_size to my list of things that would be
> nice to remove.  Or maybe I'm just tilting at windmills.

I dunno. But afaics there is no other way to look at foreign mm and find
out its limit. Say, the usage of mm->task_size in validate_range() looks
valid even if (afaics) nothing bad can happen if start/end >= task_size,
so validate_range() could just check that len+start doesn't overflow.

Oleg.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web