Re: kern/127446: [patch] fix race in sys/dev/kbdmux/kbdmux.c

Maksim, good day.

Fri, Sep 19, 2008 at 09:20:41AM -0700, Maksim Yevmenkin wrote:
> > Tried my initial patch on some 7.0-PRERELEASE -- it locks keyboard when
> >  geli asks for the password.  Had not much time to dig it out, will try
> >  to do it as soon as I can.  Substituting KBDMUX_LOCK/UNLOCK with Giant
> >  locking helps even on this FreeBSD version.
> >
> >  More testing needed, may be there are some other issues that aren't
> >  revealing themselves...
> 
> did you have a chance to do some testing? i tried substituting
> KBDMUX_LOCK/UNLOCK with Giant locking here locally and played with a
> couple of keyboards under X and console. no apparent issues or witness
> complains.

Sorry for being a bit slow, but I have good news: another race was found
and patched.  Now it is in the syscons code: high-level procedures are
racing with sckbdevent(): on my (magical ;))) notebook scgetc() from
syscons.c got the right key, but on return to sccngetch() the code was
substituted with 0x100 (NOKEY) and scancode was effectively throwed out.

So what I had seen was not the keyboard lock, but just the visual effect
of it.

Since syscons.c has some splx()/spltty() calls still hanging around and
comments are saying that these are to protect from the sckbdevent() and
scrn_timer().  I had wrapped these procedures with Giant operations.
There is one suspicious function, scstart(): I had not touched it, but
may be it should also be protected with some kind of lock.

The attached patch does this.  I did some limited testing for it: still
continuing to do it on all available systems.

> would it be ok for me to commit this?
> 
> --- kbdmux.c.orig       2008-07-29 14:21:20.000000000 -0700
> +++ kbdmux.c    2008-09-19 09:02:54.000000000 -0700
> _at__at_ -104,9 +104,11 _at__at_
> 
>  #define KBDMUX_LOCK_DESTROY(s)
> 
> -#define KBDMUX_LOCK(s)
> +#define KBDMUX_LOCK(s) \
> +       mtx_lock(&Giant)
> 
> -#define KBDMUX_UNLOCK(s)
> +#define KBDMUX_UNLOCK(s) \
> +       mtx_unlock(&Giant)
> 
>  #define KBDMUX_LOCK_ASSERT(s, w)

Yes, I think it will be fine -- I have no issues with this patch.
Although now I am testing the new patch together with my old one.
Will try to roll this patch on some systems too.

Thanks!
-- 
Eygene
 _                ___       _.--.   #
 \`.|\..----...-'`   `-._.-'_.-'`   #  Remember that it is hard
 /  ' `         ,       __.--'      #  to read the on-line manual   
 )/' _/     \   `-_,   /            #  while single-stepping the kernel.
 `-'" `"\_  ,_.-;_.-\_ ',  fsc/as   #
     _.-'_./   {_.'   ; /           #    -- FreeBSD Developers handbook 
    {_.-``-'         {_/            #