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>,
	Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [patch V2 08/16] x86/ioperm: Add bitmap sequence number
Date: Tue, 12 Nov 2019 08:08:57 -0800
Message-ID: <CALCETrXcBpxwvoPtqa1-c+SF+4K9oeebUA_JFNaN2Yn2USwVNA@mail.gmail.com> (raw)
In-Reply-To: <20191111223052.292300453@linutronix.de>

On Mon, Nov 11, 2019 at 2:35 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Add a globally unique sequence number which is incremented when ioperm() is
> changing the I/O bitmap of a task. Store the new sequence number in the
> io_bitmap structure and compare it along with the actual struct pointer
> with the one which was last loaded on a CPU. Only update the bitmap if
> either of the two changes. That should further reduce the overhead of I/O
> bitmap scheduling when there are only a few I/O bitmap users on the system.
>
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
> V2: New patch
> ---
>  arch/x86/include/asm/iobitmap.h  |    1
>  arch/x86/include/asm/processor.h |    8 +++++++
>  arch/x86/kernel/cpu/common.c     |    1
>  arch/x86/kernel/ioport.c         |    9 +++++---
>  arch/x86/kernel/process.c        |   40 +++++++++++++++++++++++++++++----------
>  5 files changed, 46 insertions(+), 13 deletions(-)
>
> --- a/arch/x86/include/asm/iobitmap.h
> +++ b/arch/x86/include/asm/iobitmap.h
> @@ -5,6 +5,7 @@
>  #include <asm/processor.h>
>
>  struct io_bitmap {
> +       u64                     sequence;
>         unsigned int            io_bitmap_max;
>         union {
>                 unsigned long   bits[IO_BITMAP_LONGS];
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -366,6 +366,14 @@ struct tss_struct {
>         struct x86_hw_tss       x86_tss;
>
>         /*
> +        * The bitmap pointer and the sequence number of the last active
> +        * bitmap. last_bitmap cannot be dereferenced. It's solely for
> +        * comparison.
> +        */
> +       struct io_bitmap        *last_bitmap;
> +       u64                     last_sequence;
> +
> +       /*
>          * 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

Why is all this stuff in the TSS?  Would it make more sense in a
percpu variable tss_state?  By putting it in the TSS, you're putting
it in cpu_entry_area, which is at least a bit odd if not an actual
security problem.

And why do you need a last_bitmap pointer?  I thin that comparing just
last_sequence should be enough.  Keeping last_bitmap around at all is
asking for trouble, since it might point to freed memory.

> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -1861,6 +1861,7 @@ void cpu_init(void)
>         /* Initialize the TSS. */
>         tss_setup_ist(tss);
>         tss->x86_tss.io_bitmap_base = IO_BITMAP_OFFSET_INVALID;
> +       tss->last_bitmap = NULL;
>         tss->io_bitmap_prev_max = 0;
>         memset(tss->io_bitmap_bytes, 0xff, sizeof(tss->io_bitmap_bytes));
>         set_tss_desc(cpu, &get_cpu_entry_area(cpu)->tss.x86_tss);
> --- a/arch/x86/kernel/ioport.c
> +++ b/arch/x86/kernel/ioport.c
> @@ -14,6 +14,8 @@
>  #include <asm/iobitmap.h>
>  #include <asm/desc.h>
>
> +static atomic64_t io_bitmap_sequence;
> +
>  /*
>   * this changes the io permissions bitmap in the current task.
>   */
> @@ -76,14 +78,15 @@ long ksys_ioperm(unsigned long from, uns
>
>         iobm->io_bitmap_max = bytes;
>
> -       /* Update the TSS: */
> -       memcpy(tss->io_bitmap_bytes, iobm->bitmap_bytes, bytes_updated);
> -
> +       /* Update the sequence number to force an update in switch_to() */
> +       iobm->sequence = atomic64_add_return(1, &io_bitmap_sequence);
>         /* Set the tasks io_bitmap pointer (might be the same) */
>         t->io_bitmap = iobm;
>         /* Mark it active for context switching */
>         set_thread_flag(TIF_IO_BITMAP);
>
> +       /* Update the TSS: */
> +       memcpy(tss->io_bitmap_bytes, iobm->bitmap_bytes, bytes_updated);
>         /* Make the bitmap base in the TSS valid */
>         tss->x86_tss.io_bitmap_base = IO_BITMAP_OFFSET_VALID;
>
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -354,6 +354,29 @@ void arch_setup_new_exec(void)
>         }
>  }
>
> +static void switch_to_update_io_bitmap(struct tss_struct *tss,
> +                                      struct io_bitmap *iobm)
> +{
> +       /*
> +        * Copy at least the byte range of the incoming tasks bitmap which
> +        * covers the permitted I/O ports.
> +        *
> +        * If the previous task which used an I/O bitmap had more bits
> +        * permitted, then the copy needs to cover those as well so they
> +        * get turned off.
> +        */
> +       memcpy(tss->io_bitmap_bytes, iobm->bitmap_bytes,
> +              max(tss->io_bitmap_prev_max, iobm->io_bitmap_max));
> +
> +       /*
> +        * Store the new max and the sequence number of this bitmap
> +        * and a pointer to the bitmap itself.
> +        */
> +       tss->io_bitmap_prev_max = iobm->io_bitmap_max;
> +       tss->last_sequence = iobm->sequence;
> +       tss->last_bitmap = iobm;
> +}
> +
>  static inline void switch_to_bitmap(struct thread_struct *next,
>                                     unsigned long tifp, unsigned long tifn)
>  {
> @@ -363,18 +386,15 @@ static inline void switch_to_bitmap(stru
>                 struct io_bitmap *iobm = next->io_bitmap;
>
>                 /*
> -                * Copy at least the size of the incoming tasks bitmap
> -                * which covers the last permitted I/O port.
> -                *
> -                * If the previous task which used an io bitmap had more
> -                * bits permitted, then the copy needs to cover those as
> -                * well so they get turned off.
> +                * Only copy bitmap data when the bitmap or the sequence
> +                * number differs. The update time is accounted to the
> +                * incoming task.
>                  */
> -               memcpy(tss->io_bitmap_bytes, iobm->bitmap_bytes,
> -                      max(tss->io_bitmap_prev_max, iobm->io_bitmap_max));
> +               if (tss->last_bitmap != iobm ||
> +                   tss->last_sequence != iobm->sequence)

As above, I think this could just be if (tss->last_sequence !=
iobm->sequence) or even if (this_cpu_read(cpu_tss_state.iobm_sequence)
!= iobm->sequence).

--Andy

  parent 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
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   ` Andy Lutomirski [this message]
2019-11-12 17:10     ` [patch V2 08/16] x86/ioperm: Add bitmap sequence number 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=CALCETrXcBpxwvoPtqa1-c+SF+4K9oeebUA_JFNaN2Yn2USwVNA@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@linux-foundation.org \
    --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