Bug 265484 - Adding INVARIANTS to GENERIC 13.1 kernel causes panic in mrsas
Summary: Adding INVARIANTS to GENERIC 13.1 kernel causes panic in mrsas
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 13.1-RELEASE
Hardware: amd64 Any
: --- Affects Some People
Assignee: Mark Johnston
URL:
Keywords: crash
Depends on:
Blocks:
 
Reported: 2022-07-28 18:26 UTC by Terry Beck
Modified: 2023-10-15 13:17 UTC (History)
7 users (show)

See Also:


Attachments
Core text from cfrash dump (65.86 KB, application/x-troff-man)
2022-07-28 18:26 UTC, Terry Beck
no flags Details
proposed patch (700 bytes, patch)
2023-03-24 18:32 UTC, Mark Johnston
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Terry Beck 2022-07-28 18:26:09 UTC
Created attachment 235531 [details]
Core text from cfrash dump

Adding INVARIANTS & INVARIANT_SUPPORT to GENERIC kernel configuration file compiles, but panics system during the next boot cycle. 
Panic string is similar to what is reported in BuG ID 259707.

panic: mutex mrsas_sim_lock not owned at /usr/src/sys/kern/kern_mutex.c:204

Attaching core.txt.2 from crash dump.
Comment 1 Mark Johnston freebsd_committer freebsd_triage 2022-08-01 14:23:58 UTC
Warner, are you in contact with the maintainer of this driver at Broadcom?
Comment 2 Warner Losh freebsd_committer freebsd_triage 2022-08-01 15:48:07 UTC
(In reply to Mark Johnston from comment #1)
I've forwarded this to them and see if they are still the right point of contact.
Comment 3 Terry Beck 2022-08-02 18:36:43 UTC
Update: I re-loaded the same system with the latest -STABLE branch release (07/29) and I am not seeing the issue.
Comment 4 Mark Johnston freebsd_committer freebsd_triage 2022-08-02 18:57:36 UTC
(In reply to Terry Beck from comment #3)
Is this still with INVARIANTS added?  If not, then that's probably because the assertion in question simply isn't enabled.
Comment 5 Terry Beck 2022-08-02 19:03:31 UTC
yes, INVARIANTS are enabled. I used the same conf file as with the -RELEASE installation.
Comment 6 Mark Johnston freebsd_committer freebsd_triage 2022-08-04 14:04:49 UTC
(In reply to Terry Beck from comment #5)
Looking at the code, I'm pretty sure that the problem is still there in stable/13, so I can't explain why it's not happening anymore.
Comment 7 Jérémie Jourdin 2023-03-22 16:59:21 UTC
(In reply to Mark Johnston from comment #6)

Hello, 

I faced the same issue running HardenedBSD (13-stable and current) on a HP Server DL360 Gen 10 plus with BROADCOM AERO-10E2 SAS Raid Controller. kernel panic occurs as soon as a write access is attempted on /dev/da0.

So I decided to test on FreeBSD and I can confirm that :
 - On FreeBSD-13.2-RC3 -> it works
 - On FreeBSD-14.0-CURRENT -> kernel panic

According to me this problem is related to the "INVARIANTS" options in kernel : the mrsas driver source code has not been modified to deal with these options. kernel panic occurs when trying to unlock the "sim_lock" mutex.

With a custom GENERIC kernel without these options (tested on HardenedBSD), it works.

As a proof of concept, I patched sys/kern/kern_mutex.c to not panic in case of wrong assertion (!mtx_owned(m)) on the mutex "mrsas_sim_lock" used in sys/dev/mrsas/mrsas.c. With this workaround in place, even with INVARIANTS options, no problem with mrsas driver anymore.

Hope it can help to fix this issue, I would be happy to help testing a patch or anything, do not hesitate.
Comment 8 Jérémie Jourdin 2023-03-22 17:25:13 UTC
(In reply to Jérémie Jourdin from comment #7)

Forgot to mention that problem occurs under RAID configuration (R1 in my use case).
No problem with JBOD.
Comment 9 Mark Johnston freebsd_committer freebsd_triage 2023-03-24 18:32:40 UTC
Created attachment 241093 [details]
proposed patch

Please give the attached patch a try with INVARIANTS enabled.
Comment 10 Jérémie Jourdin 2023-03-30 07:40:26 UTC
(In reply to Mark Johnston from comment #9)

Thank you. The proposed patch is working for me : no kernel panic anymore, even with INVARIANTS* options enabled in kernel.

Tested on :
 - HPE DL360 Gen10+ (Raid 1)
 - HardenedBSD 13-stable
 - Commit 02f071b2d30b0bfb4a674c9577892668772698bd
Comment 11 Mark Johnston freebsd_committer freebsd_triage 2023-04-13 15:41:38 UTC
https://reviews.freebsd.org/D39559
Comment 12 commit-hook freebsd_committer freebsd_triage 2023-10-07 00:33:10 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=4640df1b0a49697840b81f6bcd269a483514c6aa

commit 4640df1b0a49697840b81f6bcd269a483514c6aa
Author:     Mark Johnston <markj@FreeBSD.org>
AuthorDate: 2023-10-07 00:31:03 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2023-10-07 00:31:03 +0000

    mrsas: Fix callout locking in mrsas_complete_cmd()

    callout_stop() requires the associated lock to be held.

    This is a bit hacky, but I believe it's safe since the subsequent
    mrsas_cmd_done() call will also acquire the SIM lock to stop a different
    callout.

    PR:             265484
    Reviewed by:    imp
    Tested by:      Jérémie Jourdin <jeremie.jourdin@advens.fr>
    MFC after:      1 week
    Differential Revision:  https://reviews.freebsd.org/D39559

 sys/dev/mrsas/mrsas.c | 2 ++
 1 file changed, 2 insertions(+)
Comment 13 commit-hook freebsd_committer freebsd_triage 2023-10-14 15:29:40 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=1b934004c04c64e96ed08b1a0c560225def21d94

commit 1b934004c04c64e96ed08b1a0c560225def21d94
Author:     Mark Johnston <markj@FreeBSD.org>
AuthorDate: 2023-10-07 00:31:03 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2023-10-14 15:28:48 +0000

    mrsas: Fix callout locking in mrsas_complete_cmd()

    callout_stop() requires the associated lock to be held.

    This is a bit hacky, but I believe it's safe since the subsequent
    mrsas_cmd_done() call will also acquire the SIM lock to stop a different
    callout.

    PR:             265484
    Reviewed by:    imp
    Tested by:      Jérémie Jourdin <jeremie.jourdin@advens.fr>
    MFC after:      1 week
    Differential Revision:  https://reviews.freebsd.org/D39559

    (cherry picked from commit 4640df1b0a49697840b81f6bcd269a483514c6aa)

 sys/dev/mrsas/mrsas.c | 2 ++
 1 file changed, 2 insertions(+)
Comment 14 commit-hook freebsd_committer freebsd_triage 2023-10-14 15:29:42 UTC
A commit in branch stable/14 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=29de7af6ee47a3a251763e4220203a0d960ea532

commit 29de7af6ee47a3a251763e4220203a0d960ea532
Author:     Mark Johnston <markj@FreeBSD.org>
AuthorDate: 2023-10-07 00:31:03 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2023-10-14 15:29:11 +0000

    mrsas: Fix callout locking in mrsas_complete_cmd()

    callout_stop() requires the associated lock to be held.

    This is a bit hacky, but I believe it's safe since the subsequent
    mrsas_cmd_done() call will also acquire the SIM lock to stop a different
    callout.

    PR:             265484
    Reviewed by:    imp
    Tested by:      Jérémie Jourdin <jeremie.jourdin@advens.fr>
    MFC after:      1 week
    Differential Revision:  https://reviews.freebsd.org/D39559

    (cherry picked from commit 4640df1b0a49697840b81f6bcd269a483514c6aa)

 sys/dev/mrsas/mrsas.c | 2 ++
 1 file changed, 2 insertions(+)
Comment 15 Mark Johnston freebsd_committer freebsd_triage 2023-10-14 17:09:27 UTC
This will be included in 14.0 as well.
Comment 16 commit-hook freebsd_committer freebsd_triage 2023-10-15 13:17:12 UTC
A commit in branch releng/14.0 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=630daf68d5bd73136e981610449bf094ca00a4a3

commit 630daf68d5bd73136e981610449bf094ca00a4a3
Author:     Mark Johnston <markj@FreeBSD.org>
AuthorDate: 2023-10-07 00:31:03 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2023-10-15 13:16:12 +0000

    mrsas: Fix callout locking in mrsas_complete_cmd()

    callout_stop() requires the associated lock to be held.

    This is a bit hacky, but I believe it's safe since the subsequent
    mrsas_cmd_done() call will also acquire the SIM lock to stop a different
    callout.

    Approved by:    re (gjb)
    PR:             265484
    Reviewed by:    imp
    Tested by:      Jérémie Jourdin <jeremie.jourdin@advens.fr>
    MFC after:      1 week
    Differential Revision:  https://reviews.freebsd.org/D39559

    (cherry picked from commit 4640df1b0a49697840b81f6bcd269a483514c6aa)
    (cherry picked from commit 29de7af6ee47a3a251763e4220203a0d960ea532)

 sys/dev/mrsas/mrsas.c | 2 ++
 1 file changed, 2 insertions(+)