LinuxLists.cc - [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage further

[permalink] [raw]

Subject: Re: [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage further

On Wed, Nov 6, 2019 at 12:57 PM Thomas Gleixner <[email protected]> wrote:
>
> Calculate both the position of the first zero bit and the last zero bit to
> limit the range which needs to be copied. This does not solve the problem
> when the previous tasked had only byte 0 cleared and the next one has only
> byte 65535 cleared, but trying to solve that would be too complex and
> heavyweight for the context switch path. As the ioperm() usage is very rare
> the case which is optimized is the single task/process which uses ioperm().

Hmm.

I may read this patch wrong, but from what I can tell, if we really
have just one process with an io bitmap, we're doing unnecessary
copies.

If we really have just one process that has an iobitmap, I think we
could just keep the bitmap of that process entirely unchanged. Then,
when we switch away from it, we set the io_bitmap_base to an invalid
base outside the TSS segment, and when we switch back, we set it back
to the valid one. No actual bitmap copies at all.

So I think that rather than the "begin/end offset" games, we should
perhaps have a "what was the last process that used the IO bitmap for
this TSS" pointer (and, I think, some sequence counter, so that when
the process updates its bitmap, it invalidates that case)?

Of course, you can do *nboth*, but if we really think that the common
case is "one special process", then I think the begin/end offset is
useless, but a "last bitmap process" would be very useful.

Am I missing something?

Linus

2019-11-07 07:41:44

by Ingo Molnar

[permalink] [raw]

Subject: Re: [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage further

* Thomas Gleixner <[email protected]> wrote:

> @@ -365,19 +365,19 @@ struct tss_struct {
> struct x86_hw_tss x86_tss;
>
> /*
> - * Store the dirty size of the last io bitmap offender. The next
> - * one will have to do the cleanup as the switch out to a non
> - * io bitmap user will just set x86_tss.io_bitmap_base to a value
> - * outside of the TSS limit. So for sane tasks there is no need
> - * to actually touch the io_bitmap at all.
> + * Store the dirty byte range of the last io bitmap offender. The
> + * next one will have to do the cleanup because the switch out to a
> + * non I/O bitmap user will just set x86_tss.io_bitmap_base to a
> + * value outside of the TSS limit to not penalize tasks which do
> + * not use the I/O bitmap at all.
> */
> - unsigned int io_bitmap_prev_max;
> + unsigned int io_zerobits_start;
> + unsigned int io_zerobits_end;
>
> /*
> - * The extra 1 is there because the CPU will access an
> - * additional byte beyond the end of the IO permission
> - * bitmap. The extra byte must be all 1 bits, and must
> - * be within the limit.
> + * The extra 1 is there because the CPU will access an additional
> + * byte beyond the end of the I/O permission bitmap. The extra byte
> + * must have all bits set and must be within the TSS limit.
> */
> unsigned long io_bitmap[IO_BITMAP_LONGS + 1];
> } __aligned(PAGE_SIZE);

Note that on 32-bit kernels this blows up our CPU area calculations:

./include/linux/compiler.h:350:38: error: call to ‘__compiletime_assert_181’ declared with attribute error: BUILD_BUG_ON failed: CPU_ENTRY_AREA_PAGES * PAGE_SIZE < CPU_ENTRY_AREA_MAP_SIZE
./include/linux/compiler.h:331:4: note: in definition of macro ‘__compiletime_assert’
./include/linux/compiler.h:350:2: note: in expansion of macro ‘_compiletime_assert’
./include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’
./include/linux/build_bug.h:50:2: note: in expansion of macro ‘BUILD_BUG_ON_MSG’
arch/x86/mm/cpu_entry_area.c:181:2: note: in expansion of macro ‘BUILD_BUG_ON’
make[2]: *** [scripts/Makefile.build:265: arch/x86/mm/cpu_entry_area.o] Error 1

Thanks,

Ingo

2019-11-07 07:47:52

[permalink] [raw]

Subject: Re: [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage further

On Wed, 6 Nov 2019, Linus Torvalds wrote:
> I may read this patch wrong, but from what I can tell, if we really
> have just one process with an io bitmap, we're doing unnecessary
> copies.
>
> If we really have just one process that has an iobitmap, I think we
> could just keep the bitmap of that process entirely unchanged. Then,
> when we switch away from it, we set the io_bitmap_base to an invalid
> base outside the TSS segment, and when we switch back, we set it back
> to the valid one. No actual bitmap copies at all.
>
> So I think that rather than the "begin/end offset" games, we should
> perhaps have a "what was the last process that used the IO bitmap for
> this TSS" pointer (and, I think, some sequence counter, so that when
> the process updates its bitmap, it invalidates that case)?
>
> Of course, you can do *nboth*, but if we really think that the common
> case is "one special process", then I think the begin/end offset is
> useless, but a "last bitmap process" would be very useful.
>
> Am I missing something?

No. You are right. I'll have a look at that.

Thanks,

tglx

2019-11-07 07:48:50

[permalink] [raw]

Subject: Re: [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage further

On Thu, 7 Nov 2019, Ingo Molnar wrote:
> * Thomas Gleixner <[email protected]> wrote:
> > /*
> > - * The extra 1 is there because the CPU will access an
> > - * additional byte beyond the end of the IO permission
> > - * bitmap. The extra byte must be all 1 bits, and must
> > - * be within the limit.
> > + * The extra 1 is there because the CPU will access an additional
> > + * byte beyond the end of the I/O permission bitmap. The extra byte
> > + * must have all bits set and must be within the TSS limit.
> > */
> > unsigned long io_bitmap[IO_BITMAP_LONGS + 1];
> > } __aligned(PAGE_SIZE);
>
> Note that on 32-bit kernels this blows up our CPU area calculations:
>
> ./include/linux/compiler.h:350:38: error: call to ‘__compiletime_assert_181’ declared with attribute error: BUILD_BUG_ON failed: CPU_ENTRY_AREA_PAGES * PAGE_SIZE < CPU_ENTRY_AREA_MAP_SIZE
> ./include/linux/compiler.h:331:4: note: in definition of macro ‘__compiletime_assert’
> ./include/linux/compiler.h:350:2: note: in expansion of macro ‘_compiletime_assert’
> ./include/linux/build_bug.h:39:37: note: in expansion of macro ‘compiletime_assert’
> ./include/linux/build_bug.h:50:2: note: in expansion of macro ‘BUILD_BUG_ON_MSG’
> arch/x86/mm/cpu_entry_area.c:181:2: note: in expansion of macro ‘BUILD_BUG_ON’
> make[2]: *** [scripts/Makefile.build:265: arch/x86/mm/cpu_entry_area.o] Error 1

Duh. /me goes investigate.

2019-11-07 08:18:39

by Ingo Molnar

[permalink] [raw]

Subject: Re: [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage further

* Thomas Gleixner <[email protected]> wrote:

> + /* Update the bitmap */
> + if (turn_on) {
> + bitmap_clear(bitmap, from, num);
> + } else {
> + bitmap_set(bitmap, from, num);
> + }
> +
> + /* Get the new range */
> + first = find_first_zero_bit(bitmap, IO_BITMAP_BITS);
> +
> + for (last = next = first; next < IO_BITMAP_BITS; last = next) {
> + /* Find the next set bit and update last */
> + next = find_next_bit(bitmap, IO_BITMAP_BITS, last);
> + last = next - 1;
> + if (next == IO_BITMAP_BITS)
> + break;
> + /* Find the next zero bit and continue searching */
> + next = find_next_zero_bit(bitmap, IO_BITMAP_BITS, next);
> + }
> +
> + /* Calculate the byte boundaries for the updated region */
> + copy_start = from / 8;
> + copy_len = (round_up(from + num, 8) / 8) - copy_start;

This might seem like a small detail, but since we do the range tracking
and copying at byte granularity anyway, why not do the zero range search
at byte granularity as well?

I bet it's faster and simpler as well than the bit-searching.

We could also change over the bitmap to a char or u8 based array and lose
all the sizeof(long) indexing headaches, resulting type casts, for
anything but the actual bitmap_set/clear() calls, etc.?

I.e. now that most of the logic is byte granular, the basic data
structure might as well reflect that?

> + /*
> + * Update the per thread storage and the TSS bitmap. This must be
> + * done with preemption disabled to prevent racing against a
> + * context switch.
> + */
> + preempt_disable();
> + tss = this_cpu_ptr(&cpu_tss_rw);
>
> + if (!t->io_bitmap_ptr) {
> + unsigned int tss_start = tss->io_zerobits_start;
> + /*
> + * If the task did not use the I/O bitmap yet then the
> + * perhaps stale content in the TSS needs to be taken into
> + * account. If tss start is out of bounds the TSS storage
> + * does not contain a zero bit and it's sufficient just to
> + * copy the new range over.
> + */

s/tss/TSS

> + if (tss_start < IO_BITMAP_BYTES) {
> + unsigned int tss_end = tss->io_zerobits_end;
> + unsigned int copy_end = copy_start + copy_len;
> +
> + copy_start = min(tss_start, copy_start);
> + copy_len = max(tss_end, copy_end) - copy_start;
> + }
> + }
> +
> + /* Copy the changed range over to the TSS bitmap */
> + dst = (char *)tss->io_bitmap;
> + src = (char *)bitmap;
> + memcpy(dst + copy_start, src + copy_start, copy_len);
> +
> + if (first >= IO_BITMAP_BITS) {
> + /*
> + * If the resulting bitmap has all permissions dropped, clear
> + * TIF_IO_BITMAP and set the IO bitmap offset in the TSS to
> + * invalid. Deallocate both the new and the thread's bitmap.
> + */
> + clear_thread_flag(TIF_IO_BITMAP);
> + tss->x86_tss.io_bitmap_base = IO_BITMAP_OFFSET_INVALID;
> + tofree = bitmap;
> + bitmap = NULL;

BTW., wouldn't it be simpler to just say that if a thread uses IO ops
even once, it gets a bitmap and that's it? I.e. we could further simplify
this seldom used piece of code.

> + } else {
> /*
> + * I/O bitmap contains zero bits. Set TIF_IO_BITMAP, make
> + * the bitmap offset valid and make sure that the TSS limit
> + * is correct. It might have been wreckaged by a VMEXiT.
> */
> + set_thread_flag(TIF_IO_BITMAP);
> + tss->x86_tss.io_bitmap_base = IO_BITMAP_OFFSET_VALID;
> refresh_tss_limit();
> }

I'm wondering, shouldn't we call refresh_tss_limit() in both branches, or
is a VMEXIT-wreckaged TSS limit harmless if we otherwise have
io_bitmap_base set to IO_BITMAP_OFFSET_INVALID?

> /*
> + * Update the range in the thread and the TSS
> *
> + * Get the byte position of the first zero bit and calculate
> + * the length of the range in which zero bits exist.
> */
> + start = first / 8;
> + end = first < IO_BITMAP_BITS ? round_up(last, 8) / 8 : 0;
> + t->io_zerobits_start = tss->io_zerobits_start = start;
> + t->io_zerobits_end = tss->io_zerobits_end = end;
>
> /*
> + * Finally exchange the bitmap pointer in the thread.
> */
> + bitmap = xchg(&t->io_bitmap_ptr, bitmap);
> + preempt_enable();
>
> + kfree(bitmap);
> + kfree(tofree);
>
> return 0;

Thanks,

Ingo

2019-11-07 08:28:24

by Ingo Molnar

[permalink] [raw]

Subject: Re: [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage further

* Linus Torvalds <[email protected]> wrote:

> On Wed, Nov 6, 2019 at 12:57 PM Thomas Gleixner <[email protected]> wrote:
> >
> > Calculate both the position of the first zero bit and the last zero bit to
> > limit the range which needs to be copied. This does not solve the problem
> > when the previous tasked had only byte 0 cleared and the next one has only
> > byte 65535 cleared, but trying to solve that would be too complex and
> > heavyweight for the context switch path. As the ioperm() usage is very rare
> > the case which is optimized is the single task/process which uses ioperm().
>
> Hmm.
>
> I may read this patch wrong, but from what I can tell, if we really
> have just one process with an io bitmap, we're doing unnecessary
> copies.
>
> If we really have just one process that has an iobitmap, I think we
> could just keep the bitmap of that process entirely unchanged. Then,
> when we switch away from it, we set the io_bitmap_base to an invalid
> base outside the TSS segment, and when we switch back, we set it back
> to the valid one. No actual bitmap copies at all.
>
> So I think that rather than the "begin/end offset" games, we should
> perhaps have a "what was the last process that used the IO bitmap for
> this TSS" pointer (and, I think, some sequence counter, so that when
> the process updates its bitmap, it invalidates that case)?
>
> Of course, you can do *nboth*, but if we really think that the common
> case is "one special process", then I think the begin/end offset is
> useless, but a "last bitmap process" would be very useful.
>
> Am I missing something?

In fact on SMP systems this would result in a very nice optimization:
pretty quickly *all* TSS's would be populated with that single task's
bitmap, and it would persist even across migrations from CPU to CPU.

I'd love to get rid of the offset caching and bit scanning games as well
- it doesn't really help in a number of common scenarios and it
complicates this non-trivial piece of code a *LOT* - and we probably
don't really have the natural testing density of this code anymore to
find any regressions quickly.

So intuitively I'd suggest we gravitate towards the simplest
implementation, with a good caching optimization for the single-task
case.

I.e. the model I'm suggesting is that if a task uses ioperm() or iopl()
then it should have a bitmap from that point on until exit(), even if
it's all zeroes or all ones. Most applications that are using those
primitives really need it all the time and are using just a few ioports,
so all the tracking doesn't help much anyway.

On a related note, another simplification would be that in principle we
could also use just a single bitmap and emulate iopl() as an ioperm(all)
or ioperm(none) calls. Yeah, it's not fully ABI compatible for mixed
ioperm()/iopl() uses, but is that ABI actually being relied on in
practice?

Thanks,

Ingo

2019-11-07 09:22:01

[permalink] [raw]

Subject: Re: [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage further

On Thu, Nov 07, 2019 at 09:25:41AM +0100, Ingo Molnar wrote:
> I.e. the model I'm suggesting is that if a task uses ioperm() or iopl()
> then it should have a bitmap from that point on until exit(), even if
> it's all zeroes or all ones. Most applications that are using those
> primitives really need it all the time and are using just a few ioports,
> so all the tracking doesn't help much anyway.

I'd go even further, considering that any task having called ioperm()
or iopl() once is granted access to all 64k ports for life: if the task
was granted access to any port, it will be able to request access for any
other port anyway. And we cannot claim that finely filtering accesses
brings any particular reliability in my opinion, considering that it's
generally possible to make the system really sick by starting to play
with most I/O ports. So for me that becomes a matter of trusted vs not
trusted task. Then we can simply have two pages of 0xFF to describe
their I/O access bitmap.

> On a related note, another simplification would be that in principle we
> could also use just a single bitmap and emulate iopl() as an ioperm(all)
> or ioperm(none) calls. Yeah, it's not fully ABI compatible for mixed
> ioperm()/iopl() uses, but is that ABI actually being relied on in
> practice?

You mean you'd have a unified map for all tasks ? In this case I think
it's simpler and equivalent to simply ignore the values in the calls
and grant full perms to the 64k ports range after the calls were
validated. I could be totally wrong and missing something obvious
though.

Regards,
Willy

2019-11-07 10:02:28

[permalink] [raw]

Subject: Re: [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage further

On Thu, 7 Nov 2019, Willy Tarreau wrote:
> On Thu, Nov 07, 2019 at 09:25:41AM +0100, Ingo Molnar wrote:
> > I.e. the model I'm suggesting is that if a task uses ioperm() or iopl()
> > then it should have a bitmap from that point on until exit(), even if
> > it's all zeroes or all ones. Most applications that are using those
> > primitives really need it all the time and are using just a few ioports,
> > so all the tracking doesn't help much anyway.
>
> I'd go even further, considering that any task having called ioperm()
> or iopl() once is granted access to all 64k ports for life: if the task
> was granted access to any port, it will be able to request access for any
> other port anyway. And we cannot claim that finely filtering accesses
> brings any particular reliability in my opinion, considering that it's
> generally possible to make the system really sick by starting to play
> with most I/O ports. So for me that becomes a matter of trusted vs not
> trusted task. Then we can simply have two pages of 0xFF to describe
> their I/O access bitmap.
>
> > On a related note, another simplification would be that in principle we
> > could also use just a single bitmap and emulate iopl() as an ioperm(all)
> > or ioperm(none) calls. Yeah, it's not fully ABI compatible for mixed
> > ioperm()/iopl() uses, but is that ABI actually being relied on in
> > practice?
>
> You mean you'd have a unified map for all tasks ? In this case I think
> it's simpler and equivalent to simply ignore the values in the calls
> and grant full perms to the 64k ports range after the calls were
> validated. I could be totally wrong and missing something obvious
> though.

Changing ioperm(single port, port range) to be ioperm(all) is going to
break a bunch of test cases which actually check whether the permission is
restricted to a single I/O port or the requested port range.

Thanks,

tglx

2019-11-07 10:18:08

[permalink] [raw]

Subject: Re: [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage further

On Thu, Nov 07, 2019 at 11:00:27AM +0100, Thomas Gleixner wrote:
> Changing ioperm(single port, port range) to be ioperm(all) is going to
> break a bunch of test cases which actually check whether the permission is
> restricted to a single I/O port or the requested port range.

But out of curiosity, are these solely test cases or things that real
applications do ? We could imagine having a sysctl entry to indicate
whether or not we want strict compatibility with older code in which
case we'd take the slow path, or a modernized behavior using only the
fast path. If we managed to deal with mmap_min_addr over time, I think
it should be manageable to deal with the rare applications using ioperm().

Willy

2019-11-07 10:25:22

[permalink] [raw]

Subject: Re: [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage further

On November 7, 2019 2:00:27 AM PST, Thomas Gleixner <[email protected]> wrote:
>On Thu, 7 Nov 2019, Willy Tarreau wrote:
>> On Thu, Nov 07, 2019 at 09:25:41AM +0100, Ingo Molnar wrote:
>> > I.e. the model I'm suggesting is that if a task uses ioperm() or
>iopl()
>> > then it should have a bitmap from that point on until exit(), even
>if
>> > it's all zeroes or all ones. Most applications that are using those
>
>> > primitives really need it all the time and are using just a few
>ioports,
>> > so all the tracking doesn't help much anyway.
>>
>> I'd go even further, considering that any task having called ioperm()
>> or iopl() once is granted access to all 64k ports for life: if the
>task
>> was granted access to any port, it will be able to request access for
>any
>> other port anyway. And we cannot claim that finely filtering accesses
>> brings any particular reliability in my opinion, considering that
>it's
>> generally possible to make the system really sick by starting to play
>> with most I/O ports. So for me that becomes a matter of trusted vs
>not
>> trusted task. Then we can simply have two pages of 0xFF to describe
>> their I/O access bitmap.
>>
>> > On a related note, another simplification would be that in
>principle we
>> > could also use just a single bitmap and emulate iopl() as an
>ioperm(all)
>> > or ioperm(none) calls. Yeah, it's not fully ABI compatible for
>mixed
>> > ioperm()/iopl() uses, but is that ABI actually being relied on in
>> > practice?
>>
>> You mean you'd have a unified map for all tasks ? In this case I
>think
>> it's simpler and equivalent to simply ignore the values in the calls
>> and grant full perms to the 64k ports range after the calls were
>> validated. I could be totally wrong and missing something obvious
>> though.
>
>Changing ioperm(single port, port range) to be ioperm(all) is going to
>break a bunch of test cases which actually check whether the permission
>is
>restricted to a single I/O port or the requested port range.
>
>Thanks,
>
> tglx

This seems very undesirable... as much as we might wish otherwise, the port bitmap is the equivalent to the MMU, and there are definitely users doing direct device I/O out there.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

2019-11-07 10:30:59

[permalink] [raw]

Subject: Re: [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage further

On Thu, Nov 07, 2019 at 02:19:19AM -0800, [email protected] wrote:
> >Changing ioperm(single port, port range) to be ioperm(all) is going to
> >break a bunch of test cases which actually check whether the permission
> >is restricted to a single I/O port or the requested port range.
> >
> >Thanks,
> >
> > tglx
>
> This seems very undesirable... as much as we might wish otherwise, the port
> bitmap is the equivalent to the MMU, and there are definitely users doing
> direct device I/O out there.

Doing these, sure, but doing these while ranges are really checked ?
I mean, the MMU grants you access to the pages you were assigned. Here
with the I/O bitmap you just have to ask for access to port X and you
get it. I could understand the benefit if we had EBUSY in return but
that's not the case, you can actually request access to a port range
another device driver or process is currently using, and mess up with
what it does even by accident. I remember streaming 1-bit music in
userland from the LED of my floppy drive in the late-90s, it used to
cause some trouble to the floppy driver when using mtools in parallel :-)

Willy

2019-11-07 13:01:23

[permalink] [raw]

Subject: Re: [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage further

On Thu, Nov 07, 2019 at 02:50:20AM -0800, [email protected] wrote:
> You get access to the ports you are assigned, just like pages you are
> assigned... the rest is kernel policy, or, for that matter, privileged
> userspace (get permissions to the necessary ports, then drop privilege... the
> usual stuff.)

I agree, my point is that there's already no policy checking at the
moment ports are assigned, hence a process having the permissions to
request just port 0x70-0x71 to read the hwclock will also have permission
to request access to the sensor chip a 0x2E and trigger a watchdog
reset or stop the CPU fan. Thus any policy enforcement is solely done
by the requesting process itself, assuming it doesn't simply use iopl()
already, which grants everything.

This is why I'm really wondering if the real use cases that need all
this stuff still exist at all in practice.

Willy

2019-11-07 14:46:10

[permalink] [raw]

Subject: Re: [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage further

On November 7, 2019 2:27:56 AM PST, Willy Tarreau <[email protected]> wrote:
>On Thu, Nov 07, 2019 at 02:19:19AM -0800, [email protected] wrote:
>> >Changing ioperm(single port, port range) to be ioperm(all) is going
>to
>> >break a bunch of test cases which actually check whether the
>permission
>> >is restricted to a single I/O port or the requested port range.
>> >
>> >Thanks,
>> >
>> > tglx
>>
>> This seems very undesirable... as much as we might wish otherwise,
>the port
>> bitmap is the equivalent to the MMU, and there are definitely users
>doing
>> direct device I/O out there.
>
>Doing these, sure, but doing these while ranges are really checked ?
>I mean, the MMU grants you access to the pages you were assigned. Here
>with the I/O bitmap you just have to ask for access to port X and you
>get it. I could understand the benefit if we had EBUSY in return but
>that's not the case, you can actually request access to a port range
>another device driver or process is currently using, and mess up with
>what it does even by accident. I remember streaming 1-bit music in
>userland from the LED of my floppy drive in the late-90s, it used to
>cause some trouble to the floppy driver when using mtools in parallel
>:-)
>
>Willy

You get access to the ports you are assigned, just like pages you are assigned... the rest is kernel policy, or, for that matter, privileged userspace (get permissions to the necessary ports, then drop privilege... the usual stuff.)

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

2019-11-07 16:46:23

by Eric W. Biederman

[permalink] [raw]

Subject: Re: [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage further

Willy Tarreau <[email protected]> writes:

> On Thu, Nov 07, 2019 at 02:50:20AM -0800, [email protected] wrote:
>> You get access to the ports you are assigned, just like pages you are
>> assigned... the rest is kernel policy, or, for that matter, privileged
>> userspace (get permissions to the necessary ports, then drop privilege... the
>> usual stuff.)
>
> I agree, my point is that there's already no policy checking at the
> moment ports are assigned, hence a process having the permissions to
> request just port 0x70-0x71 to read the hwclock will also have permission
> to request access to the sensor chip a 0x2E and trigger a watchdog
> reset or stop the CPU fan. Thus any policy enforcement is solely done
> by the requesting process itself, assuming it doesn't simply use iopl()
> already, which grants everything.
>
> This is why I'm really wondering if the real use cases that need all
> this stuff still exist at all in practice.

My memory is that the applications that didn't need fine grain access to
ports would just use iopl.

Further a quick look shows that dosemu uses ioperm in a fine grained
manner. From memory it would allow a handful of ports to allow directly
accessing a device and depended on the rest of the port accesses to be
disallowed so it could trap and emulate them.

So yes I do believe making ioperm ioperm(all) will break userspace.

Eric

2019-11-07 16:54:57

[permalink] [raw]

Subject: Re: [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage further

On Thu, Nov 7, 2019 at 8:45 AM Eric W. Biederman <[email protected]> wrote:
>
> Further a quick look shows that dosemu uses ioperm in a fine grained
> manner. From memory it would allow a handful of ports to allow directly
> accessing a device and depended on the rest of the port accesses to be
> disallowed so it could trap and emulate them.

Yes. Making ioperm() some all-or-nothing thing would be horribly bad,
and has no real advantages that I can see.

Linus

2019-11-07 17:03:06

[permalink] [raw]

Subject: Re: [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage further

On Thu, Nov 07, 2019 at 10:45:09AM -0600, Eric W. Biederman wrote:
> Further a quick look shows that dosemu uses ioperm in a fine grained
> manner. From memory it would allow a handful of ports to allow directly
> accessing a device and depended on the rest of the port accesses to be
> disallowed so it could trap and emulate them.
>
> So yes I do believe making ioperm ioperm(all) will break userspace.

OK, and I must confess I almost forgot about dosemu, that's a good point!

Thanks,
Willy

2019-11-07 18:04:59

[permalink] [raw]

Subject: Re: [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage further

On Thu, 7 Nov 2019, Ingo Molnar wrote:
> * Thomas Gleixner <[email protected]> wrote:
> This might seem like a small detail, but since we do the range tracking
> and copying at byte granularity anyway, why not do the zero range search
> at byte granularity as well?
>
> I bet it's faster and simpler as well than the bit-searching.

Not necessarily. The bitmap search uses unsigned long internally at least
to the point where it finds a zero bit.

But probably we should just ditch that optimization and rather have the
sequence number to figure out whether something needs to be copied at all.

> > + if (first >= IO_BITMAP_BITS) {
> > + /*
> > + * If the resulting bitmap has all permissions dropped, clear
> > + * TIF_IO_BITMAP and set the IO bitmap offset in the TSS to
> > + * invalid. Deallocate both the new and the thread's bitmap.
> > + */
> > + clear_thread_flag(TIF_IO_BITMAP);
> > + tss->x86_tss.io_bitmap_base = IO_BITMAP_OFFSET_INVALID;
> > + tofree = bitmap;
> > + bitmap = NULL;
>
> BTW., wouldn't it be simpler to just say that if a thread uses IO ops
> even once, it gets a bitmap and that's it? I.e. we could further simplify
> this seldom used piece of code.

Maybe.

> > + } else {
> > /*
> > + * I/O bitmap contains zero bits. Set TIF_IO_BITMAP, make
> > + * the bitmap offset valid and make sure that the TSS limit
> > + * is correct. It might have been wreckaged by a VMEXiT.
> > */
> > + set_thread_flag(TIF_IO_BITMAP);
> > + tss->x86_tss.io_bitmap_base = IO_BITMAP_OFFSET_VALID;
> > refresh_tss_limit();
> > }
>
> I'm wondering, shouldn't we call refresh_tss_limit() in both branches, or
> is a VMEXIT-wreckaged TSS limit harmless if we otherwise have
> io_bitmap_base set to IO_BITMAP_OFFSET_INVALID?

Yes. because the VMEXIT wreckage restricts TSS limit to 0x67 which is the
end of the hardware tss struct. As the invalid offset points in any case
outside the TSS limit it does not matter. It always #GP's when an I/O
access happens in user space.

Thanks,

tglx

2019-11-07 19:25:26

by Brian Gerst

[permalink] [raw]

Subject: Re: [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage further

On Wed, Nov 6, 2019 at 4:01 PM Thomas Gleixner <[email protected]> wrote:
>
> Sane ioperm() users only set the few bits in the I/O space which they need to
> access. But the update logic of the TSS I/O bitmap copies always from byte
> 0 to the last byte in the tasks bitmap which contains a zero permission bit.
>
> That means that for access only to port 65335 the full 8K bitmap has to be
> copied even if all the bytes in the TSS I/O bitmap are already filled with
> 0xff.
>
> Calculate both the position of the first zero bit and the last zero bit to
> limit the range which needs to be copied. This does not solve the problem
> when the previous tasked had only byte 0 cleared and the next one has only
> byte 65535 cleared, but trying to solve that would be too complex and
> heavyweight for the context switch path. As the ioperm() usage is very rare
> the case which is optimized is the single task/process which uses ioperm().

Here is a different idea: We already map the TSS virtually in
cpu_entry_area. Why not page-align the IO bitmap and remap it to the
task's bitmap on task switch? That would avoid all copying on task
switch.

--
Brian Gerst

2019-11-07 19:57:51

[permalink] [raw]

Subject: Re: [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage further

On Thu, Nov 7, 2019 at 11:24 AM Brian Gerst <[email protected]> wrote:
>
> Here is a different idea: We already map the TSS virtually in
> cpu_entry_area. Why not page-align the IO bitmap and remap it to the
> task's bitmap on task switch? That would avoid all copying on task
> switch.

We map the tss _once_, statically, percpu, without ever changing it,
and then we just (potentially) change a couple of fields in it on
process switch.

Your idea isn't horrible, but it would involve a TLB flush for the
page when the io bitmap changes. Which is almost certainly more
expensive than just copying the bitmap intelligently.

Particularly since I do think that the copy can basically be done
effectively never, assuming there really aren't multiple concurrent
users of ioperm() (and iopl).

Linus

2019-11-07 21:05:06

by Brian Gerst

[permalink] [raw]

Subject: Re: [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage further

On Thu, Nov 7, 2019 at 2:54 PM Linus Torvalds
<[email protected]> wrote:
>
> On Thu, Nov 7, 2019 at 11:24 AM Brian Gerst <[email protected]> wrote:
> >
> > Here is a different idea: We already map the TSS virtually in
> > cpu_entry_area. Why not page-align the IO bitmap and remap it to the
> > task's bitmap on task switch? That would avoid all copying on task
> > switch.
>
> We map the tss _once_, statically, percpu, without ever changing it,
> and then we just (potentially) change a couple of fields in it on
> process switch.
>
> Your idea isn't horrible, but it would involve a TLB flush for the
> page when the io bitmap changes. Which is almost certainly more
> expensive than just copying the bitmap intelligently.
>
> Particularly since I do think that the copy can basically be done
> effectively never, assuming there really aren't multiple concurrent
> users of ioperm() (and iopl).

There wouldn't have to be a flush on every task switch. If we make it
so that tasks that don't use a bitmap just unmap the pages in the
cpu_entry_area and set tss.io_bitmap_base to outside the segment
limit, we would only have to flush when switching from a task using
the bitmap (because the next task uses a different bitmap or we are
unmapping it). If the previous task doesn't have a bitmap the pages
in cpu_entry_area were unmapped and can't be in the TLB, so no flush
is needed.

Going a step further, we could track which task is mapped to the
current cpu like proposed above, and only flush when a different task
needs the IO bitmap, or when the bitmap is being freed on task exit.

--
Brian Gerst

2019-11-07 21:33:31

[permalink] [raw]

Subject: Re: [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage further

On Thu, 7 Nov 2019, Brian Gerst wrote:
> On Thu, Nov 7, 2019 at 2:54 PM Linus Torvalds
> <[email protected]> wrote:
> >
> > On Thu, Nov 7, 2019 at 11:24 AM Brian Gerst <[email protected]> wrote:
> > >
> > > Here is a different idea: We already map the TSS virtually in
> > > cpu_entry_area. Why not page-align the IO bitmap and remap it to the
> > > task's bitmap on task switch? That would avoid all copying on task
> > > switch.
> >
> > We map the tss _once_, statically, percpu, without ever changing it,
> > and then we just (potentially) change a couple of fields in it on
> > process switch.
> >
> > Your idea isn't horrible, but it would involve a TLB flush for the
> > page when the io bitmap changes. Which is almost certainly more
> > expensive than just copying the bitmap intelligently.
> >
> > Particularly since I do think that the copy can basically be done
> > effectively never, assuming there really aren't multiple concurrent
> > users of ioperm() (and iopl).
>
> There wouldn't have to be a flush on every task switch. If we make it
> so that tasks that don't use a bitmap just unmap the pages in the
> cpu_entry_area and set tss.io_bitmap_base to outside the segment
> limit, we would only have to flush when switching from a task using
> the bitmap (because the next task uses a different bitmap or we are
> unmapping it). If the previous task doesn't have a bitmap the pages
> in cpu_entry_area were unmapped and can't be in the TLB, so no flush
> is needed.

Funny. I was just debating exactly this with Peter Ziljstra over IRC :)

> Going a step further, we could track which task is mapped to the
> current cpu like proposed above, and only flush when a different task
> needs the IO bitmap, or when the bitmap is being freed on task exit.

Yes.

But, we really should check what aside of DoSemu is using this still. None
of my machines I checked have a single instance of ioperm()/iopl() usage.

So the real question is whether it's worth the trouble or if we are just
better off to copy if there is an actual user and the sequence count of the
bitmap is different than the one which was active last time.

Thanks,

tglx

2019-11-07 21:46:09

[permalink] [raw]

Subject: Re: [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage further

On Thu, Nov 7, 2019 at 1:00 PM Brian Gerst <[email protected]> wrote:
>
> There wouldn't have to be a flush on every task switch.

No. But we'd have to flush on any switch that currently does that memcpy.

And my point is that a tlb flush (even the single-page case) is likely
more expensive than the memcpy.

> Going a step further, we could track which task is mapped to the
> current cpu like proposed above, and only flush when a different task
> needs the IO bitmap, or when the bitmap is being freed on task exit.

Well, that's exactly my "track the last task" optimization for copying
the thing.

IOW, it's the same optimization as avoiding the memcpy.

Which I think is likely very effective, but also makes it fairly
pointless to then try to be clever..

So the basic issue remains that playing VM games has almost
universally been slower and more complex than simply not playing VM
games. TLB flushes - even invlpg - tends to be pretty slow.

Of course, we probably end up invalidating the TLB's anyway, so maybe
in this case we don't care. The ioperm bitmap is _technically_
per-thread, though, so it should be flushed even if the VM isn't
flushed...

Linus

2019-11-07 23:23:43

[permalink] [raw]

Subject: Re: [patch 5/9] x86/ioport: Reduce ioperm impact for sane usage further

On November 7, 2019 1:32:15 PM PST, Thomas Gleixner <[email protected]> wrote:
>On Thu, 7 Nov 2019, Brian Gerst wrote:
>> On Thu, Nov 7, 2019 at 2:54 PM Linus Torvalds
>> <[email protected]> wrote:
>> >
>> > On Thu, Nov 7, 2019 at 11:24 AM Brian Gerst <[email protected]>
>wrote:
>> > >
>> > > Here is a different idea: We already map the TSS virtually in
>> > > cpu_entry_area. Why not page-align the IO bitmap and remap it to
>the
>> > > task's bitmap on task switch? That would avoid all copying on
>task
>> > > switch.
>> >
>> > We map the tss _once_, statically, percpu, without ever changing
>it,
>> > and then we just (potentially) change a couple of fields in it on
>> > process switch.
>> >
>> > Your idea isn't horrible, but it would involve a TLB flush for the
>> > page when the io bitmap changes. Which is almost certainly more
>> > expensive than just copying the bitmap intelligently.
>> >
>> > Particularly since I do think that the copy can basically be done
>> > effectively never, assuming there really aren't multiple concurrent
>> > users of ioperm() (and iopl).
>>
>> There wouldn't have to be a flush on every task switch. If we make
>it
>> so that tasks that don't use a bitmap just unmap the pages in the
>> cpu_entry_area and set tss.io_bitmap_base to outside the segment
>> limit, we would only have to flush when switching from a task using
>> the bitmap (because the next task uses a different bitmap or we are
>> unmapping it). If the previous task doesn't have a bitmap the pages
>> in cpu_entry_area were unmapped and can't be in the TLB, so no flush
>> is needed.
>
>Funny. I was just debating exactly this with Peter Ziljstra over IRC :)
>
>> Going a step further, we could track which task is mapped to the
>> current cpu like proposed above, and only flush when a different task
>> needs the IO bitmap, or when the bitmap is being freed on task exit.
>
>Yes.
>
>But, we really should check what aside of DoSemu is using this still.
>None
>of my machines I checked have a single instance of ioperm()/iopl()
>usage.
>
>So the real question is whether it's worth the trouble or if we are
>just
>better off to copy if there is an actual user and the sequence count of
>the
>bitmap is different than the one which was active last time.
>
>Thanks,
>
> tglx

I have written suffer using this, because of far better real time performance. I just want to punch a hole (just like mmapping an MMIO device.)

I do agree that let's not optimize for the rare case.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

2019-11-08 01:17:14