Date: Mon, 24 Nov 2008 11:23:47 -0800 From: "Maksim Yevmenkin" <emax@freebsd.org> To: "Bruce Evans" <brde@optusnet.com.au> Cc: Eygene Ryabinkin <rea-fbsd@codelabs.ru>, "current@freebsd.org" <current@freebsd.org> Subject: Re: syscons(4) races Message-ID: <bb4a86c70811241123v80b6d1eo9ca2fd797910e07f@mail.gmail.com> In-Reply-To: <20081122204840.K1008@besplex.bde.org> References: <bb4a86c70811201625v4dd78c64p64868b6d636a7602@mail.gmail.com> <20081122204840.K1008@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
------=_Part_139299_8944108.1227554627869
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
Bruce,
thanks for the clarifications. please read my comments/answers inline
>> ok, in this case i have a somewhat stupid question. when kbdmux(4) is
>> the default keyboard, all those kbdd_xxx() calls (that sccngetch()
>> makes) will call into kbdmux(4) functions that will grab giant.
>> kbdmux was changed about 2 months ago to do that. it sounds like those
>> changes are completely wrong. am i correct here?
>
> Yes, the low-level console functions are supposed to be reentrant (more
> details below), so they cannot do things like that.
all right. i will need to back those changes out then.
>>> BTW, sccngetch() shouldn't exist. It existed to demultiplex the 2
>>> console driver entry points sc_cngetch() and sc_cncheckc(), but
>>> sc_cncheckc() has gone away.
>>
>> the entry point in consdev struct is still there. also cncheckc() is
>> trying to call it (if its present). so it looks like it may not be
>> completely gone, just sysconst(4) no longer implements it.
>
> Ah, this is compatibility cruft which is bogusly only used by a driver
> that is newer than the cruft (xen). Normal drivers use CONSOLE_DRIVER()
> but xen uses the old interface CONS_DRIVER(). CONSOLE_DRIVER() differs
> from CONS_DRIVER() only in not taking the cn_checkc() entry point.
> Binary compatibility and correct naming were broken by renaming
> cn_checkc() to cn_getc(). Names are still correct at the top level
> (there, cncheckc() gives the non-blocking interface and cngetc() gives
> the blocking interface, while in normal drivers there is now only
> cn_getc() which gives the non-blocking interface).
>
> It is cn_dbctl() that has completely gone away.
ok. thanks for the explanation.
>> There is still a problem for calls to sc_cngetc() from the low-console
>>> driver. These can race with sckbdevent(). In debugger mode, no locking
>>> is permitted, so syscons' getc functions must be carefully written to
>>> do only harmless things when they lose races. They are not carefully
>>> written (e.g., almost the first thing they do, shown in the above, may
>>> give a buffer overrun:
>>>
>>> if (fkeycp < fkey.len) {
>>> mtx_unlock(&Giant);
>>> splx(s);
>>> return fkey.str[fkeycp++];
>>> }
>>
>> all right, so we can not use locks, i'm guessing this includes spin
>> locks too. can we use atomic operations here? since, in polling mode,
>> consumer is going to call getc() in a loop, can we use atomic
>> reference counter to make sure there is only one caller running at a
>> time? if someone already grabbed reference counter just return -1 as
>> if there is no input?
>
> It is safer to use local spinlocks and safer still to use home-made
> locks based on atomic_cmpset(), but still wrong to do simply that since
> it can cause deadlock, especially for debugging -- it is not possible to
> make sure that there is only 1 caller running, since debugging (or NMI,
> or perhaps an ordinary interrupt that cannot reasonably be masked) can
> preempt any caller. sio uses its local spinlock (I don't like this)
> together with a hack to allow reentering from the debugger only:
>
> % need_unlock = 0;
> % if (!kdb_active && sio_inited == 2 && !mtx_owned(&sio_lock)) {
> % mtx_lock_spin(&sio_lock);
> % need_unlock = 1;
> % }
>
> This is only done for cn_putc(). cn_getc() (really cn_checkc()) is
> completely missing locking (except in debugger mode) and needs
> higher-level locking anyway, since the whole polling loop in cngetc()
> needs to be locked to prevent interference.
all right, so i'm guessing chgetc() or rather cncheckc() in
kern_cons.c (or perhaps even gets() in libkern/) should be locked
using some sort of spinlock. cnputs() seems to be using cnputs_mtx.
maybe we need cngets() or something like this?
one thing that i do not really like about sccngetch() in syscons is
that its constantly trying to switch keyboard mode and keyboard
polling. i was wondering if it would be possible to clearly separate
sckbdevent() path from debugger/etc. the former would be interrupt
driven and the later would be poll driven.
> Here sio_lock is the local spinlock (actually per-driver so it isn't very
> local), sio_inited is a flag to prevent use of sio_lock here before
> sio_lock is initialized (this flag requires delicate initialization
> using atomic_cmpset to avoid races), and !kdb_active is the hack to
> let the debugger in irrespective of the state of the lock and without
> touching the lock.
i see.
> Any lock used for the console must be MTX_QUIET and/or MTX_NOWITNESS
> to prevent console i/o generating endless i/o about itself. I think
> it must be a spinlock too. The lock thus reduces to a simple spinlock
> which can be implemented directly using atomic_cmpset(), thus making it
> clear that the lock has no interactions with other parts of the system.
right
here is some straw-man patch that Eygene Ryabinkin and myself were
toying with. obviously its far from complete, but we'd like to get
some feedback to see if we are going in the right direction.
thanks,
max
------=_Part_139299_8944108.1227554627869
Content-Type: text/plain; name=syscons.c-atomic-locking.patch.txt
Content-Transfer-Encoding: base64
X-Attachment-Id: f_fnxibf5o0
Content-Disposition: attachment;
filename=syscons.c-atomic-locking.patch.txt
SW50ZXJsb2NrcyBzY2tiZGV2ZW50KCkgYW5kIHNjY25nZXRjaCgpIHdpdGggdGhlIGxvdy1sZXZl
bCBhdG9taWMKc2VtYXBob3Jlcy4KCi0tLSBzeXMvZGV2L3N5c2NvbnMvc3lzY29ucy5jLm9yaWcJ
MjAwOC0wOC0zMSAxNDoxNzo0MC4wMDAwMDAwMDAgKzA0MDAKKysrIHN5cy9kZXYvc3lzY29ucy9z
eXNjb25zLmMJMjAwOC0xMS0yNCAwODozOTowNy4wMDAwMDAwMDAgKzAzMDAKQEAgLTYwLDYgKzYw
LDcgQEAKICNpbmNsdWRlIDxzeXMvdHR5Lmg+CiAjaW5jbHVkZSA8c3lzL3Bvd2VyLmg+CiAKKyNp
bmNsdWRlIDxtYWNoaW5lL2F0b21pYy5oPgogI2luY2x1ZGUgPG1hY2hpbmUvY2xvY2suaD4KICNp
ZiBkZWZpbmVkKF9fc3BhcmM2NF9fKSB8fCBkZWZpbmVkKF9fcG93ZXJwY19fKQogI2luY2x1ZGUg
PG1hY2hpbmUvc2NfbWFjaGRlcC5oPgpAQCAtMTY1LDYgKzE2NiwxMyBAQAogCiBzdGF0aWMJaW50
CQlkZWJ1Z2dlcjsKIAorLyoKKyAqIFNlbWFwaG9yZSB0aGF0IHVzZXMgZm9yIGxvY2tpbmcgd2l0
aGluIHN5c2NvbnMuICBBcyB3YXMgbm90ZWQgYnkKKyAqIEJydWNlIEV2YW5zLCBsb2NrcyBjYW4n
dCBiZSBhY2Nlc3NlZCBpbiB0aGUgZGVidWdnZXIgbW9kZSwgYnV0CisgKiBkZWJ1Z2dlciB1c2Vz
IGtleWJvYXJkIHNvbWV0aW1lcyA7KSkKKyAqLworc3RhdGljCWludAkJX3N5c2NvbnNfc2VtYXBo
b3JlOworCiAvKiBwcm90b3R5cGVzICovCiBzdGF0aWMgaW50IHNjX2FsbG9jYXRlX2tleWJvYXJk
KHNjX3NvZnRjX3QgKnNjLCBpbnQgdW5pdCk7CiBzdGF0aWMgaW50IHNjdmlkcHJvYmUoaW50IHVu
aXQsIGludCBmbGFncywgaW50IGNvbnMpOwpAQCAtNjA5LDcgKzYxNyw3IEBACiB7CiAgICAgc2Nf
c29mdGNfdCAqc2M7CiAgICAgc3RydWN0IHR0eSAqY3VyX3R0eTsKLSAgICBpbnQgYywgZXJyb3Ig
PSAwOyAKKyAgICBpbnQgYywgZXJyb3IgPSAwLCBzZW1fb3duZWQgPSAwOwogICAgIHNpemVfdCBs
ZW47CiAgICAgdV9jaGFyICpjcDsKIApAQCAtNjMxLDEyICs2MzksMjAgQEAKIAlnb3RvIGRvbmU7
CiAgICAgfQogCisgICAgLyoKKyAgICAgKiBHcmFiIHNlbWFwaG9yZSB0byBub3RpZnkgb3RoZXJz
IHRoYXQgd2UncmUgZG9pbmcgb3VyIGpvYi4KKyAgICAgKi8KKyAgICBzZW1fb3duZWQgPSBhdG9t
aWNfY21wc2V0X2FjcV9pbnQoJl9zeXNjb25zX3NlbWFwaG9yZSwgMCwgMSk7CisKICAgICAvKiAK
ICAgICAgKiBMb29wIHdoaWxlIHRoZXJlIGlzIHN0aWxsIGlucHV0IHRvIGdldCBmcm9tIHRoZSBr
ZXlib2FyZC4KICAgICAgKiBJIGRvbid0IHRoaW5rIHRoaXMgaXMgbmVzc2VzYXJ5LCBhbmQgaXQg
ZG9lc24ndCBmaXgKICAgICAgKiB0aGUgWGFjY2VsLTIuMSBrZXlib2FyZCBoYW5nLCBidXQgaXQg
Y2FuJ3QgaHVydC4JCVhYWAorICAgICAqCisgICAgICogSWYgc2VtYXBob3JlIGlzIGFscmVhZHkg
Z3JhYmJlZCwgZG9uJ3QgcG9sbC4KICAgICAgKi8KLSAgICB3aGlsZSAoKGMgPSBzY2dldGMoc2Ms
IFNDR0VUQ19OT05CTE9DSykpICE9IE5PS0VZKSB7CisgICAgd2hpbGUgKHNlbV9vd25lZCAmJgor
ICAgICAgICAoYyA9IHNjZ2V0YyhzYywgU0NHRVRDX05PTkJMT0NLKSkgIT0gTk9LRVkpIHsKIAog
CWN1cl90dHkgPSBTQ19ERVYoc2MsIHNjLT5jdXJfc2NwLT5pbmRleCk7CiAJaWYgKCF0dHlfb3Bl
bmVkKGN1cl90dHkpKQpAQCAtNjc3LDYgKzY5Myw4IEBACiAgICAgc2MtPmN1cl9zY3AtPnN0YXR1
cyB8PSBNT1VTRV9ISURERU47CiAKIGRvbmU6CisgICAgaWYgKHNlbV9vd25lZCkKKwlhdG9taWNf
Y2xlYXJfaW50KCZfc3lzY29uc19zZW1hcGhvcmUsIDEpOwogICAgIG10eF91bmxvY2soJkdpYW50
KTsKICAgICByZXR1cm4gKGVycm9yKTsKIH0KQEAgLTE1NjksMTAgKzE1ODcsMTAgQEAKICAgICBz
Y3Jfc3RhdCAqc2NwOwogICAgIHVfY2hhciAqcDsKICAgICBpbnQgY3VyX21vZGU7Ci0gICAgaW50
IHMgPSBzcGx0dHkoKTsJLyogYmxvY2sgc2NrYmRldmVudCBhbmQgc2Nybl90aW1lciB3aGlsZSB3
ZSBwb2xsICovCiAgICAgaW50IGM7CiAKLSAgICAvKiBhc3NlcnQoc2NfY29uc29sZSAhPSBOVUxM
KSAqLworICAgIGlmICghYXRvbWljX2NtcHNldF9hY3FfaW50KCZfc3lzY29uc19zZW1hcGhvcmUs
IDAsIDEpKQorCXJldHVybiAtMTsKIAogICAgIC8qIAogICAgICAqIFN0b3AgdGhlIHNjcmVlbiBz
YXZlciBhbmQgdXBkYXRlIHRoZSBzY3JlZW4gaWYgbmVjZXNzYXJ5LgpAQCAtMTU4MywxMiArMTYw
MSwxMyBAQAogICAgIHNjY251cGRhdGUoc2NwKTsKIAogICAgIGlmIChma2V5Y3AgPCBma2V5Lmxl
bikgewotCXNwbHgocyk7Ci0JcmV0dXJuIGZrZXkuc3RyW2ZrZXljcCsrXTsKKwljID0gZmtleS5z
dHJbZmtleWNwKytdOworCWF0b21pY19jbGVhcl9pbnQoJl9zeXNjb25zX3NlbWFwaG9yZSwgMSk7
CisJcmV0dXJuIGM7CiAgICAgfQogCiAgICAgaWYgKHNjcC0+c2MtPmtiZCA9PSBOVUxMKSB7Ci0J
c3BseChzKTsKKwlhdG9taWNfY2xlYXJfaW50KCZfc3lzY29uc19zZW1hcGhvcmUsIDEpOwogCXJl
dHVybiAtMTsKICAgICB9CiAKQEAgLTE2MTAsMjYgKzE2MjksMjkgQEAKICAgICBzY3AtPmtiZF9t
b2RlID0gY3VyX21vZGU7CiAgICAga2JkZF9pb2N0bChzY3AtPnNjLT5rYmQsIEtEU0tCTU9ERSwg
KGNhZGRyX3QpJnNjcC0+a2JkX21vZGUpOwogICAgIGtiZGRfZGlzYWJsZShzY3AtPnNjLT5rYmQp
OwotICAgIHNwbHgocyk7CiAKICAgICBzd2l0Y2ggKEtFWUZMQUdTKGMpKSB7CiAgICAgY2FzZSAw
OgkvKiBub3JtYWwgY2hhciAqLwotCXJldHVybiBLRVlDSEFSKGMpOworCWMgPSBLRVlDSEFSKGMp
OworCWJyZWFrOwogICAgIGNhc2UgRktFWToJLyogZnVuY3Rpb24ga2V5ICovCiAJcCA9IGtiZGRf
Z2V0X2ZrZXlzdHIoc2NwLT5zYy0+a2JkLCBLRVlDSEFSKGMpLCAoc2l6ZV90ICopJmZrZXljcCk7
CiAJZmtleS5sZW4gPSBma2V5Y3A7CiAJaWYgKChwICE9IE5VTEwpICYmIChma2V5LmxlbiA+IDAp
KSB7CiAJICAgIGJjb3B5KHAsIGZrZXkuc3RyLCBma2V5Lmxlbik7CiAJICAgIGZrZXljcCA9IDE7
Ci0JICAgIHJldHVybiBma2V5LnN0clswXTsKKwkgICAgYyA9IGZrZXkuc3RyWzBdOwogCX0KLQly
ZXR1cm4gYzsJLyogWFhYICovCisJLyogWFhYICovCisJYnJlYWs7CiAgICAgY2FzZSBOT0tFWToK
ICAgICBjYXNlIEVSUktFWToKICAgICBkZWZhdWx0OgotCXJldHVybiAtMTsKKwljID0gLTE7CisJ
YnJlYWs7CiAgICAgfQotICAgIC8qIE5PVCBSRUFDSEVEICovCisgICAgYXRvbWljX2NsZWFyX2lu
dCgmX3N5c2NvbnNfc2VtYXBob3JlLCAxKTsKKyAgICByZXR1cm4gYzsKIH0KIAogc3RhdGljIHZv
aWQK
------=_Part_139299_8944108.1227554627869--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?bb4a86c70811241123v80b6d1eo9ca2fd797910e07f>