Re: [patch V2 08/16] x86/ioperm: Add bitmap sequence number
From: Andy Lutomirski
Date: Tue Nov 12 2019 - 11:09:14 EST
On Mon, Nov 11, 2019 at 2:35 PM Thomas Gleixner <tglx@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> ---
> 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