NetBSD-Bugs archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: kern/57226: NetBSD 10_BETA kernel crash
The following reply was made to PR kern/57226; it has been noted by GNATS.
From: PHO <pho%cielonegro.org@localhost>
To: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Cc: gnats-bugs%NetBSD.org@localhost
Subject: Re: kern/57226: NetBSD 10_BETA kernel crash
Date: Mon, 26 Jun 2023 11:16:48 +0900
This is a multi-part message in MIME format.
--------------AgangFOrsVYnJOd1CKoUL0ht
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
On 6/26/23 03:37, Taylor R Campbell wrote:
> Nice work! I agree this is the right analysis and correct solution.
Thanks!
> Couple nits:
>
>> callout(9): Obtain the lock before checking if a callout is destroyable
>>
>> One cannot safely observe the state of a callout without holding its lock,
>> because c->c_flags and c->c_cpu might be getting mutated somewhere else.
>
> Did you observe this to be necessary to fix the problem, or do you
> have a theory for why this should be necessary? I don't think it
> should be. I think if anything, it should be under #ifdef DEBUG (but
> I'm not sure we should keep it at all). The comment you deleted in
> this change,
>
> /*
> * It's not necessary to lock in order to see the correct value
> * of c->c_flags. If the callout could potentially have been
> * running, the current thread should have stopped it.
> */
>
> correctly reflects the callout(9) API contract, so if there is any
> race here, it is either a bug in callout(9) or a user error -- and we
> want to detect it and fail noisily in either case, not suppress it by
> taking a lock.
In fact it's my early speculation. I initially thought callout_destroy()
was reading stale values. So no objection to withdrawing the change.
> In the other change, which I think is the only one that should be
> needed to solve the problem:
>
>> @@ -593,7 +592,7 @@ callout_wait(callout_impl_t *c, void *interlock, kmutex_t *lock)
>> * - the callout itself has called callout_halt() (nice!)
>> */
>> cc = c->c_cpu;
>
> I think you need to make sure to load c->c_cpu exactly once here, not
> once in callout_wait and then once again in
> callout_running_somewhere_else.
>
> It's possible that one can prove it is OK to reload c->c_cpu twice,
> but let's keep the change minimal.
>
> In fact for now I would suggest just inlining it in callout_halt so
> that the fix is a one-line change to start, and to pull up, and then
> we can tidy it up afterward with no-functional-change commits.
Alright. Done.
>> - if (__predict_true(cc->cc_active != c || cc->cc_lwp == l))
>> + if (__predict_false(!callout_running_somewhere_else(c)))
>
> This should stay __predict_true. Callouts are supposed to be quick,
> so by the time we've reloaded c->c_cpu from memory, it may have
> changed again, even though the caller already determined
> callout_running_somewhere_else returned true.
Got it. That makes sense.
So I'm re-running the Pandoc test with updated patches (also attached to
this mail):
https://github.com/NetBSD/src/commit/e7da905b63a9bb35670066afe9aeb72c7c6351a6
https://github.com/NetBSD/src/commit/4ca3e49d3f3e3b59345b076b15a2eb212b256a65
https://github.com/NetBSD/src/commit/785e80aa567aabe0f4a9749dacd48a80681d4aac
It's going to take a while. When it finishes (or dies) I will inform you.
--------------AgangFOrsVYnJOd1CKoUL0ht
Content-Type: text/x-patch; charset=UTF-8;
name="0001-callout-9-Fix-panic-in-callout_destroy-kern-57226.patch"
Content-Disposition: attachment;
filename*0="0001-callout-9-Fix-panic-in-callout_destroy-kern-57226.patch"
Content-Transfer-Encoding: base64
RnJvbSBlN2RhOTA1YjYzYTliYjM1NjcwMDY2YWZlOWFlYjcyYzdjNjM1MWE2IE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBQSE8gPHBob0BjaWVsb25lZ3JvLm9yZz4KRGF0ZTog
U3VuLCAyNSBKdW4gMjAyMyAyMDoxNTo0NCArMDkwMApTdWJqZWN0OiBbUEFUQ0ggMS8zXSBj
YWxsb3V0KDkpOiBGaXggcGFuaWMoKSBpbiBjYWxsb3V0X2Rlc3Ryb3koKSAoa2Vybi81NzIy
NikKClRoZSBjdWxwcml0IHdhcyBjYWxsb3V0X2hhbHQoKS4gIihjLT5jX2ZsYWdzICYgQ0FM
TE9VVF9GSVJFRCkgIT0gMCIgd2Fzbid0CnRoZSBjb3JyZWN0IHdheSB0byBjaGVjayBpZiBh
IGNhbGxvdXQgaXMgcnVubmluZy4gSXQgZmFpbGVkIHRvIHdhaXQgZm9yIGEKcnVubmluZyBj
YWxsb3V0IHRvIGZpbmlzaCBpbiB0aGUgZm9sbG93aW5nIHNjZW5hcmlvOgoKMS4gY3B1MCBp
bml0aWFsaXplcyBhIGNhbGxvdXQgYW5kIHNjaGVkdWxlcyBpdC4KMi4gY3B1MCBpbnZva2Vz
IGNhbGxvdXRfc29mdGxvY2soKSBhbmQgZmlyZXMgdGhlIGNhbGxvdXQsIHNldHRpbmcgdGhl
IGZsYWcKICAgQ0FMTE9VVF9GSVJFRC4KMy4gVGhlIGNhbGxvdXQgaW52b2tlcyBjYWxsb3V0
X3NjaGVkdWxlKCkgdG8gcmUtc2NoZWR1bGUgaXRzZWxmLgo0LiBjYWxsb3V0X3NjaGVkdWxl
X2xvY2tlZCgpIGNsZWFycyB0aGUgZmxhZyBDQUxMT1VUX0ZJUkVELCBhbmQgcmVsZWFzZXMK
ICAgdGhlIGxvY2suCjUuIEJlZm9yZSB0aGUgbG9jayBpcyByZS1hY3F1aXJlZCBieSBjYWxs
b3V0X3NvZnRsb2NrKCksIGNwdTEgZGVjaWRlcyB0bwogICBkZXN0cm95IHRoZSBjYWxsb3V0
LiBJdCBmaXJzdCBpbnZva2VzIGNhbGxvdXRfaGFsdCgpIHRvIG1ha2Ugc3VyZSB0aGUKICAg
Y2FsbG91dCBmaW5pc2hlcyBydW5uaW5nLgo2LiBCdXQgc2luY2UgQ0FMTE9VVF9GSVJFRCBo
YXMgYmVlbiBjbGVhcmVkLCBjYWxsb3V0X2hhbHQoKSB0aGlua3MgaXQncyBub3QKICAgcnVu
bmluZyBhbmQgdGhlcmVmb3JlIHJldHVybnMgd2l0aG91dCBpbnZva2luZyBjYWxsb3V0X3dh
aXQoKS4KNy4gY3B1MSBwcm9jZWVkcyB0byBpbnZva2UgY2FsbG91dF9kZXN0cm95KCkgd2hp
bGUgaXQncyBzdGlsbCBydW5uaW5nIG9uCiAgIGNwdTAuIGNhbGxvdXRfZGVzdHJveSgpIGRl
dGVjdHMgdGhhdCBhbmQgcGFuaWNzLgotLS0KIHN5cy9rZXJuL2tlcm5fdGltZW91dC5jIHwg
MTAgKysrKystLS0tLQogMSBmaWxlIGNoYW5nZWQsIDUgaW5zZXJ0aW9ucygrKSwgNSBkZWxl
dGlvbnMoLSkKCmRpZmYgLS1naXQgYS9zeXMva2Vybi9rZXJuX3RpbWVvdXQuYyBiL3N5cy9r
ZXJuL2tlcm5fdGltZW91dC5jCmluZGV4IDNmMzMzOGE2Nzg3Li5lNmUzMTgxMzg5NiAxMDA2
NDQKLS0tIGEvc3lzL2tlcm4va2Vybl90aW1lb3V0LmMKKysrIGIvc3lzL2tlcm4va2Vybl90
aW1lb3V0LmMKQEAgLTU0Miw3ICs1NDIsNyBAQCBjYWxsb3V0X2hhbHQoY2FsbG91dF90ICpj
cywgdm9pZCAqaW50ZXJsb2NrKQogewogCWNhbGxvdXRfaW1wbF90ICpjID0gKGNhbGxvdXRf
aW1wbF90ICopY3M7CiAJa211dGV4X3QgKmxvY2s7Ci0JaW50IGZsYWdzOworCXN0cnVjdCBj
YWxsb3V0X2NwdSAqY2M7CiAKIAlLQVNTRVJUKGMtPmNfbWFnaWMgPT0gQ0FMTE9VVF9NQUdJ
Qyk7CiAJS0FTU0VSVCghY3B1X2ludHJfcCgpKTsKQEAgLTU1MiwxMSArNTUyLDExIEBAIGNh
bGxvdXRfaGFsdChjYWxsb3V0X3QgKmNzLCB2b2lkICppbnRlcmxvY2spCiAJbG9jayA9IGNh
bGxvdXRfbG9jayhjKTsKIAlTRFRfUFJPQkU0KHNkdCwga2VybmVsLCBjYWxsb3V0LCBoYWx0
LAogCSAgICBjLCBjLT5jX2Z1bmMsIGMtPmNfYXJnLCBjLT5jX2ZsYWdzKTsKLQlmbGFncyA9
IGMtPmNfZmxhZ3M7Ci0JaWYgKChmbGFncyAmIENBTExPVVRfUEVORElORykgIT0gMCkKKwlp
ZiAoKGMtPmNfZmxhZ3MgJiBDQUxMT1VUX1BFTkRJTkcpICE9IDApCiAJCUNJUkNRX1JFTU9W
RSgmYy0+Y19saXN0KTsKLQljLT5jX2ZsYWdzID0gZmxhZ3MgJiB+KENBTExPVVRfUEVORElO
R3xDQUxMT1VUX0ZJUkVEKTsKLQlpZiAoX19wcmVkaWN0X2ZhbHNlKGZsYWdzICYgQ0FMTE9V
VF9GSVJFRCkpIHsKKwljLT5jX2ZsYWdzICY9IH4oQ0FMTE9VVF9QRU5ESU5HfENBTExPVVRf
RklSRUQpOworCWNjID0gYy0+Y19jcHU7CisJaWYgKF9fcHJlZGljdF9mYWxzZShjYy0+Y2Nf
YWN0aXZlID09IGMgJiYgY2MtPmNjX2x3cCAhPSBjdXJsd3ApKSB7CiAJCWNhbGxvdXRfd2Fp
dChjLCBpbnRlcmxvY2ssIGxvY2spOwogCQlyZXR1cm4gdHJ1ZTsKIAl9Ci0tIAoyLjM5LjAK
Cg==
--------------AgangFOrsVYnJOd1CKoUL0ht
Content-Type: text/x-patch; charset=UTF-8;
name="0002-callout-9-Tidy-up-the-condition-for-callout-is-runni.patch"
Content-Disposition: attachment;
filename*0="0002-callout-9-Tidy-up-the-condition-for-callout-is-runni.pa";
filename*1="tch"
Content-Transfer-Encoding: base64
RnJvbSA0Y2EzZTQ5ZDNmM2UzYjU5MzQ1YjA3NmIxNWEyZWIyMTJiMjU2YTY1IE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBQSE8gPHBob0BjaWVsb25lZ3JvLm9yZz4KRGF0ZTog
TW9uLCAyNiBKdW4gMjAyMyAxMDo0Njo1MSArMDkwMApTdWJqZWN0OiBbUEFUQ0ggMi8zXSBj
YWxsb3V0KDkpOiBUaWR5IHVwIHRoZSBjb25kaXRpb24gZm9yICJjYWxsb3V0IGlzIHJ1bm5p
bmcKIG9uIGFub3RoZXIgTFdQIgoKTm8gZnVuY3Rpb25hbCBjaGFuZ2VzLgotLS0KIHN5cy9r
ZXJuL2tlcm5fdGltZW91dC5jIHwgMTkgKysrKysrKysrKysrKystLS0tLQogMSBmaWxlIGNo
YW5nZWQsIDE0IGluc2VydGlvbnMoKyksIDUgZGVsZXRpb25zKC0pCgpkaWZmIC0tZ2l0IGEv
c3lzL2tlcm4va2Vybl90aW1lb3V0LmMgYi9zeXMva2Vybi9rZXJuX3RpbWVvdXQuYwppbmRl
eCBlNmUzMTgxMzg5Ni4uYzBhZGZlMDVjZmMgMTAwNjQ0Ci0tLSBhL3N5cy9rZXJuL2tlcm5f
dGltZW91dC5jCisrKyBiL3N5cy9rZXJuL2tlcm5fdGltZW91dC5jCkBAIC0yNjIsNiArMjYy
LDE3IEBAIGNhbGxvdXRfbG9jayhjYWxsb3V0X2ltcGxfdCAqYykKIAl9CiB9CiAKKy8qCisg
KiBDaGVjayBpZiB0aGUgY2FsbG91dCBpcyBjdXJyZW50bHkgcnVubmluZyBvbiBhbiBMV1Ag
dGhhdCBpc24ndCBjdXJsd3AuCisgKi8KK3N0YXRpYyBpbmxpbmUgYm9vbAorY2FsbG91dF9y
dW5uaW5nX3NvbWV3aGVyZV9lbHNlKGNhbGxvdXRfaW1wbF90ICpjLCBzdHJ1Y3QgY2FsbG91
dF9jcHUgKmNjKQoreworCUtBU1NFUlQoYy0+Y19jcHUgPT0gY2MpOworCisJcmV0dXJuIGNj
LT5jY19hY3RpdmUgPT0gYyAmJiBjYy0+Y2NfbHdwICE9IGN1cmx3cDsKK30KKwogLyoKICAq
IGNhbGxvdXRfc3RhcnR1cDoKICAqCkBAIC0zNzgsNyArMzg5LDcgQEAgY2FsbG91dF9kZXN0
cm95KGNhbGxvdXRfdCAqY3MpCiAJS0FTU0VSVE1TRygoYy0+Y19mbGFncyAmIENBTExPVVRf
UEVORElORykgPT0gMCwKIAkgICAgInBlbmRpbmcgY2FsbG91dCAlcDogY19mdW5jICglcCkg
Y19mbGFncyAoJSN4KSBkZXN0cm95ZWQgZnJvbSAlcCIsCiAJICAgIGMsIGMtPmNfZnVuYywg
Yy0+Y19mbGFncywgX19idWlsdGluX3JldHVybl9hZGRyZXNzKDApKTsKLQlLQVNTRVJUTVNH
KGMtPmNfY3B1LT5jY19sd3AgPT0gY3VybHdwIHx8IGMtPmNfY3B1LT5jY19hY3RpdmUgIT0g
YywKKwlLQVNTRVJUTVNHKCFjYWxsb3V0X3J1bm5pbmdfc29tZXdoZXJlX2Vsc2UoYywgYy0+
Y19jcHUpLAogCSAgICAicnVubmluZyBjYWxsb3V0ICVwOiBjX2Z1bmMgKCVwKSBjX2ZsYWdz
ICglI3gpIGRlc3Ryb3llZCBmcm9tICVwIiwKIAkgICAgYywgYy0+Y19mdW5jLCBjLT5jX2Zs
YWdzLCBfX2J1aWx0aW5fcmV0dXJuX2FkZHJlc3MoMCkpOwogCWMtPmNfbWFnaWMgPSAwOwpA
QCAtNTQyLDcgKzU1Myw2IEBAIGNhbGxvdXRfaGFsdChjYWxsb3V0X3QgKmNzLCB2b2lkICpp
bnRlcmxvY2spCiB7CiAJY2FsbG91dF9pbXBsX3QgKmMgPSAoY2FsbG91dF9pbXBsX3QgKilj
czsKIAlrbXV0ZXhfdCAqbG9jazsKLQlzdHJ1Y3QgY2FsbG91dF9jcHUgKmNjOwogCiAJS0FT
U0VSVChjLT5jX21hZ2ljID09IENBTExPVVRfTUFHSUMpOwogCUtBU1NFUlQoIWNwdV9pbnRy
X3AoKSk7CkBAIC01NTUsOCArNTY1LDcgQEAgY2FsbG91dF9oYWx0KGNhbGxvdXRfdCAqY3Ms
IHZvaWQgKmludGVybG9jaykKIAlpZiAoKGMtPmNfZmxhZ3MgJiBDQUxMT1VUX1BFTkRJTkcp
ICE9IDApCiAJCUNJUkNRX1JFTU9WRSgmYy0+Y19saXN0KTsKIAljLT5jX2ZsYWdzICY9IH4o
Q0FMTE9VVF9QRU5ESU5HfENBTExPVVRfRklSRUQpOwotCWNjID0gYy0+Y19jcHU7Ci0JaWYg
KF9fcHJlZGljdF9mYWxzZShjYy0+Y2NfYWN0aXZlID09IGMgJiYgY2MtPmNjX2x3cCAhPSBj
dXJsd3ApKSB7CisJaWYgKF9fcHJlZGljdF9mYWxzZShjYWxsb3V0X3J1bm5pbmdfc29tZXdo
ZXJlX2Vsc2UoYywgYy0+Y19jcHUpKSkgewogCQljYWxsb3V0X3dhaXQoYywgaW50ZXJsb2Nr
LCBsb2NrKTsKIAkJcmV0dXJuIHRydWU7CiAJfQpAQCAtNTkyLDcgKzYwMSw3IEBAIGNhbGxv
dXRfd2FpdChjYWxsb3V0X2ltcGxfdCAqYywgdm9pZCAqaW50ZXJsb2NrLCBrbXV0ZXhfdCAq
bG9jaykKIAkJICogLSB0aGUgY2FsbG91dCBpdHNlbGYgaGFzIGNhbGxlZCBjYWxsb3V0X2hh
bHQoKSAobmljZSEpCiAJCSAqLwogCQljYyA9IGMtPmNfY3B1OwotCQlpZiAoX19wcmVkaWN0
X3RydWUoY2MtPmNjX2FjdGl2ZSAhPSBjIHx8IGNjLT5jY19sd3AgPT0gbCkpCisJCWlmIChf
X3ByZWRpY3RfdHJ1ZSghY2FsbG91dF9ydW5uaW5nX3NvbWV3aGVyZV9lbHNlKGMsIGNjKSkp
CiAJCQlicmVhazsKIAogCQkvKiBJdCdzIHJ1bm5pbmcgLSBuZWVkIHRvIHdhaXQgZm9yIGl0
IHRvIGNvbXBsZXRlLiAqLwotLSAKMi4zOS4wCgo=
--------------AgangFOrsVYnJOd1CKoUL0ht
Content-Type: text/x-patch; charset=UTF-8;
name="0003-callout-9-Delete-the-unused-member-cc_cancel-from-st.patch"
Content-Disposition: attachment;
filename*0="0003-callout-9-Delete-the-unused-member-cc_cancel-from-st.pa";
filename*1="tch"
Content-Transfer-Encoding: base64
RnJvbSA3ODVlODBhYTU2N2FhYmUwZjRhOTc0OWRhY2Q0OGE4MDY4MWQ0YWFjIE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBQSE8gPHBob0BjaWVsb25lZ3JvLm9yZz4KRGF0ZTog
U3VuLCAyNSBKdW4gMjAyMyAyMDo0Njo1MiArMDkwMApTdWJqZWN0OiBbUEFUQ0ggMy8zXSBj
YWxsb3V0KDkpOiBEZWxldGUgdGhlIHVudXNlZCBtZW1iZXIgY2NfY2FuY2VsIGZyb20KIHN0
cnVjdCBjYWxsb3V0X2NwdQoKSSBzZWUgbm8gcmVhc29uIHdoeSBpdCBzaG91bGQgYmUgdGhl
cmUsIGFuZCBiZWxpZXZlIGl0cyBhIGxlZnRvdmVyIGZyb20Kc29tZSBvbGQgY29kZS4KLS0t
CiBzeXMva2Vybi9rZXJuX3RpbWVvdXQuYyB8IDEyIC0tLS0tLS0tLS0tLQogMSBmaWxlIGNo
YW5nZWQsIDEyIGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL3N5cy9rZXJuL2tlcm5fdGlt
ZW91dC5jIGIvc3lzL2tlcm4va2Vybl90aW1lb3V0LmMKaW5kZXggYzBhZGZlMDVjZmMuLjEw
Y2EyMDg2OWFlIDEwMDY0NAotLS0gYS9zeXMva2Vybi9rZXJuX3RpbWVvdXQuYworKysgYi9z
eXMva2Vybi9rZXJuX3RpbWVvdXQuYwpAQCAtMTc0LDcgKzE3NCw2IEBAIHN0cnVjdCBjYWxs
b3V0X2NwdSB7CiAJdV9pbnQJCWNjX3RpY2tzOwogCWx3cF90CQkqY2NfbHdwOwogCWNhbGxv
dXRfaW1wbF90CSpjY19hY3RpdmU7Ci0JY2FsbG91dF9pbXBsX3QJKmNjX2NhbmNlbDsKIAlz
dHJ1Y3QgZXZjbnQJY2NfZXZfbGF0ZTsKIAlzdHJ1Y3QgZXZjbnQJY2NfZXZfYmxvY2s7CiAJ
c3RydWN0IGNhbGxvdXRfY2lyY3EgY2NfdG9kbzsJCS8qIFdvcmtsaXN0ICovCkBAIC01MDcs
NyArNTA2LDYgQEAgYm9vbAogY2FsbG91dF9zdG9wKGNhbGxvdXRfdCAqY3MpCiB7CiAJY2Fs
bG91dF9pbXBsX3QgKmMgPSAoY2FsbG91dF9pbXBsX3QgKiljczsKLQlzdHJ1Y3QgY2FsbG91
dF9jcHUgKmNjOwogCWttdXRleF90ICpsb2NrOwogCWJvb2wgZXhwaXJlZDsKIApAQCAtNTIw
LDE2ICs1MTgsNiBAQCBjYWxsb3V0X3N0b3AoY2FsbG91dF90ICpjcykKIAlleHBpcmVkID0g
KChjLT5jX2ZsYWdzICYgQ0FMTE9VVF9GSVJFRCkgIT0gMCk7CiAJYy0+Y19mbGFncyAmPSB+
KENBTExPVVRfUEVORElOR3xDQUxMT1VUX0ZJUkVEKTsKIAotCWNjID0gYy0+Y19jcHU7Ci0J
aWYgKGNjLT5jY19hY3RpdmUgPT0gYykgewotCQkvKgotCQkgKiBUaGlzIGlzIGZvciBub24t
TVBTQUZFIGNhbGxvdXRzIG9ubHkuICBUbyBzeW5jaHJvbml6ZQotCQkgKiBlZmZlY3RpdmVs
eSB3ZSBtdXN0IGJlIGNhbGxlZCB3aXRoIGtlcm5lbF9sb2NrIGhlbGQuCi0JCSAqIEl0J3Mg
YWxzbyB0YWtlbiBpbiBjYWxsb3V0X3NvZnRjbG9jay4KLQkJICovCi0JCWNjLT5jY19jYW5j
ZWwgPSBjOwotCX0KLQogCVNEVF9QUk9CRTUoc2R0LCBrZXJuZWwsIGNhbGxvdXQsIHN0b3As
CiAJICAgIGMsIGMtPmNfZnVuYywgYy0+Y19hcmcsIGMtPmNfZmxhZ3MsIGV4cGlyZWQpOwog
Ci0tIAoyLjM5LjAKCg==
--------------AgangFOrsVYnJOd1CKoUL0ht--
Home |
Main Index |
Thread Index |
Old Index