Re: [patch V2 06/16] x86/io: Speedup schedule out of I/O bitmap user

[Andy Lutomirski]

On Mon, Nov 11, 2019 at 2:35 PM Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>
> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>
> There is no requirement to update the TSS I/O bitmap when a thread using it is
> scheduled out and the incoming thread does not use it.
>
> For the permission check based on the TSS I/O bitmap the CPU calculates the memory
> location of the I/O bitmap by the address of the TSS and the io_bitmap_base member
> of the tss_struct. The easiest way to invalidate the I/O bitmap is to switch the
> offset to an address outside of the TSS limit.
>
> If an I/O instruction is issued from user space the TSS limit causes #GP to be
> raised in the same was as valid I/O bitmap with all bits set to 1 would do.
>
> This removes the extra work when an I/O bitmap using task is scheduled out
> and puts the burden on the rare I/O bitmap users when they are scheduled
> in.
>
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>

> --- a/arch/x86/kernel/ioport.c
> +++ b/arch/x86/kernel/ioport.c

I won't swear this is wrong, but I'm not convinced it's correct
either.  I see two issues:

> @@ -40,8 +40,6 @@ long ksys_ioperm(unsigned long from, uns
>                         return -ENOMEM;
>
>                 memset(bitmap, 0xff, IO_BITMAP_BYTES);
> -               t->io_bitmap_ptr = bitmap;
> -               set_thread_flag(TIF_IO_BITMAP);
>
>                 /*
>                  * Now that we have an IO bitmap, we need our TSS limit to be
> @@ -50,6 +48,11 @@ long ksys_ioperm(unsigned long from, uns
>                  * limit correct.
>                  */
>                 preempt_disable();
> +               t->io_bitmap_ptr = bitmap;
> +               set_thread_flag(TIF_IO_BITMAP);
> +               /* Make the bitmap base in the TSS valid */
> +               tss = this_cpu_ptr(&cpu_tss_rw);
> +               tss->x86_tss.io_bitmap_base = IO_BITMAP_OFFSET_VALID;
>                 refresh_tss_limit();
>                 preempt_enable();
>         }

It's not shown in the diff, but the very next line of code turns
preemption back off.  This means that we might schedule right here
with TIF_IO_BITMAP set, the base set to VALID, but the wrong data in
the bitmap.  I *think* this will actually end up being okay, but it
certainly makes understanding the code harder.  Can you adjust the
code so that preemption stays off?

More importantly, the code below this modifies the TSS copy in place
instead of writing a whole new copy.  But now that you've added your
optimization, the TSS copy might be *someone else's* IO bitmap.  So I
think you might end up with more io ports allowed than you intended.
For example:

Task A uses ioperm() to enable all ports.
Switch to task B.  Now the TSS base is INVALID but all bitmap bits are still 0.
Task B calls ioperm().

The code will set the base to VALID and will correctly set up the
thread's copy of the bitmap, but I think the copy will only update the
bits 0 through whatever ioperm() touched and not the bits above that
in the TSS.

I would believe that this is fixed later in your patch set.  If so,
perhaps you should just memcpy() the whole thing without trying to
optimize in this patch and then let the changes later re-optimize it
as appropriate.  IOW change memcpy(tss->io_bitmap, t->io_bitmap_ptr,
bytes_updated); to memcpy(..., BYTES_PER_LONG * IO_BITMAP_LONGS) or
similar.

--Andy