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