LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Andy Lutomirski <luto@kernel.org>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: LKML <linux-kernel@vger.kernel.org>, X86 ML <x86@kernel.org>,
	Linus Torvalds <torvalds@linuxfoundation.org>,
	Andy Lutomirski <luto@kernel.org>,
	Stephen Hemminger <stephen@networkplumber.org>,
	Willy Tarreau <w@1wt.eu>, Juergen Gross <jgross@suse.com>,
	Sean Christopherson <sean.j.christopherson@intel.com>,
	"H. Peter Anvin" <hpa@zytor.com>
Subject: Re: [patch V2 06/16] x86/io: Speedup schedule out of I/O bitmap user
Date: Tue, 12 Nov 2019 08:00:54 -0800
Message-ID: <CALCETrUcY_DhZC8CH0NhoRp_r6mh4v1Z2dmhsdErV8wx6FsLaw@mail.gmail.com> (raw)
In-Reply-To: <20191111223052.086299881@linutronix.de>

On Mon, Nov 11, 2019 at 2:35 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> From: Thomas Gleixner <tglx@linutronix.de>
>
> 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@linutronix.de>
>

> --- 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

  reply index

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-11 22:03 [patch V2 00/16] x86/iopl: Prevent user space from using CLI/STI with iopl(3) Thomas Gleixner
2019-11-11 22:03 ` [patch V2 01/16] x86/ptrace: Prevent truncation of bitmap size Thomas Gleixner
2019-11-12 15:34   ` Andy Lutomirski
2019-11-11 22:03 ` [patch V2 02/16] x86/process: Unify copy_thread_tls() Thomas Gleixner
2019-11-11 22:03 ` [patch V2 03/16] x86/cpu: Unify cpu_init() Thomas Gleixner
2019-11-11 22:03 ` [patch V2 04/16] x86/tss: Fix and move VMX BUILD_BUG_ON() Thomas Gleixner
2019-11-11 22:44   ` Paolo Bonzini
2019-11-12 15:37   ` Andy Lutomirski
2019-11-11 22:03 ` [patch V2 05/16] x86/iopl: Cleanup include maze Thomas Gleixner
2019-11-12 15:37   ` Andy Lutomirski
2019-11-11 22:03 ` [patch V2 06/16] x86/io: Speedup schedule out of I/O bitmap user Thomas Gleixner
2019-11-12 16:00   ` Andy Lutomirski [this message]
2019-11-12 17:08     ` Thomas Gleixner
2019-11-11 22:03 ` [patch V2 07/16] x86/ioperm: Move iobitmap data into a struct Thomas Gleixner
2019-11-12 16:02   ` Andy Lutomirski
2019-11-12 17:08     ` Thomas Gleixner
2019-11-11 22:03 ` [patch V2 08/16] x86/ioperm: Add bitmap sequence number Thomas Gleixner
2019-11-12  9:22   ` Peter Zijlstra
2019-11-12  9:55     ` [patch V2 08/16] x86/ioperm: Add bitmap sequence numberc Thomas Gleixner
2019-11-12 16:08   ` [patch V2 08/16] x86/ioperm: Add bitmap sequence number Andy Lutomirski
2019-11-12 17:10     ` Thomas Gleixner
2019-11-11 22:03 ` [patch V2 09/16] x86/ioperm: Move TSS bitmap update to exit to user work Thomas Gleixner
2019-11-12 16:16   ` Andy Lutomirski
2019-11-12 17:20     ` Thomas Gleixner
2019-11-12 17:41       ` Andy Lutomirski
2019-11-12 17:46         ` Linus Torvalds
2019-11-13  8:30           ` Peter Zijlstra
2019-11-11 22:03 ` [patch V2 10/16] x86/ioperm: Remove bitmap if all permissions dropped Thomas Gleixner
2019-11-12 17:43   ` Andy Lutomirski
2019-11-11 22:03 ` [patch V2 11/16] x86/ioperm: Share I/O bitmap if identical Thomas Gleixner
2019-11-12  7:14   ` Ingo Molnar
2019-11-12  7:17     ` Thomas Gleixner
2019-11-12  7:52       ` Ingo Molnar
2019-11-12  9:15   ` Peter Zijlstra
2019-11-12  9:51     ` Thomas Gleixner
2019-11-14 11:02     ` David Laight
2019-11-14 12:39       ` Thomas Gleixner
2019-11-14 13:09       ` Peter Zijlstra
2019-11-14 13:22         ` David Laight
2019-11-12 18:12   ` Andy Lutomirski
2019-11-11 22:03 ` [patch V2 12/16] selftests/x86/ioperm: Extend testing so the shared bitmap is exercised Thomas Gleixner
2019-11-11 22:03 ` [patch V2 13/16] x86/iopl: Fixup misleading comment Thomas Gleixner
2019-11-12 18:14   ` Andy Lutomirski
2019-11-11 22:03 ` [patch V2 14/16] x86/iopl: Restrict iopl() permission scope Thomas Gleixner
2019-11-11 23:03   ` Thomas Gleixner
2019-11-12  6:32     ` Ingo Molnar
2019-11-12  8:42   ` Ingo Molnar
2019-11-12 10:07     ` Thomas Gleixner
2019-11-12 18:35   ` Andy Lutomirski
2019-11-11 22:03 ` [patch V2 15/16] x86/iopl: Remove legacy IOPL option Thomas Gleixner
2019-11-12 18:37   ` Andy Lutomirski
2019-11-12 19:40     ` Thomas Gleixner
2019-11-11 22:03 ` [patch V2 16/16] selftests/x86/iopl: Extend test to cover IOPL emulation Thomas Gleixner
2019-11-12  7:40 ` [PATCH] x86/iopl: Factor out IO-bitmap related TSS fields into 'struct x86_io_bitmap' Ingo Molnar
2019-11-12  7:59   ` [PATCH] x86/iopl: Harmonize 'struct io_bitmap' and 'struct x86_io_bitmap' nomenclature Ingo Molnar
2019-11-12  8:11   ` [PATCH] x86/iopl: Clear up the role of the two bitmap copying fields Ingo Molnar
2019-11-12  8:15   ` [PATCH] x86/iopl: Rename <asm/iobitmap.h> to <asm/io_bitmap.h> Ingo Molnar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CALCETrUcY_DhZC8CH0NhoRp_r6mh4v1Z2dmhsdErV8wx6FsLaw@mail.gmail.com \
    --to=luto@kernel.org \
    --cc=hpa@zytor.com \
    --cc=jgross@suse.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=sean.j.christopherson@intel.com \
    --cc=stephen@networkplumber.org \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linuxfoundation.org \
    --cc=w@1wt.eu \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git