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

[Thomas Gleixner]

On Tue, 12 Nov 2019, Andy Lutomirski wrote:
> On Mon, Nov 11, 2019 at 2:35 PM Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> > @@ -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.

Yeah, you are right. Did not think about that. Will fix that up.
 
> 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.

Right.

Thanks for spotting that!

       tglx