Bug 274795

Summary: broken locking in e6000sw
Product: Base System Reporter: Mark Johnston <markj>
Component: kernAssignee: Mark Johnston <markj>
Status: Closed FIXED    
Severity: Affects Only Me CC: emaste, mw
Priority: ---    
Version: 15.0-CURRENT   
Hardware: Any   
OS: Any   

Description Mark Johnston freebsd_committer freebsd_triage 2023-10-29 17:26:22 UTC 
Commit 469290648005e13b819a19353032ca53dda4378f made e6000sw's implementation of miibus_(read|write)reg assume that the softc lock is held.  I presume that is to avoid lock recursion in e6000sw_attach() -> e6000sw_attach_miibus() -> mii_attach() -> MIIBUS_READREG().

However, the lock assertion in e6000sw_readphy_locked() can fail:

panic: Lock e6000sw not exclusively locked @ /usr/home/markj/src/freebsd/sys/dev/etherswitch/e6000sw/e6000sw.c:773

cpuid = 0
time = 1698599456
KDB: stack backtrace:
db_trace_self() at db_trace_self
db_trace_self_wrapper() at db_trace_self_wrapper+0x38
vpanic() at vpanic+0x1a0
panic() at panic+0x48
_sx_assert() at _sx_assert+0x100
e6000sw_readphy_locked() at e6000sw_readphy_locked+0x40
gentbi_probe() at gentbi_probe+0x7c
device_probe_child() at device_probe_child+0x150                               
device_probe() at device_probe+0xa0                                            
device_probe_and_attach() at device_probe_and_attach+0x38                      
bus_generic_attach() at bus_generic_attach+0x1c                                                                                                                
miibus_attach() at miibus_attach+0x88                                        
device_attach() at device_attach+0x3fc
device_probe_and_attach() at device_probe_and_attach+0x80                      
bus_generic_driver_added() at bus_generic_driver_added+0x90
devclass_driver_added() at devclass_driver_added+0x48
devclass_add_driver() at devclass_add_driver+0x148
module_register_init() at module_register_init+0xb4                                                                                                            
linker_load_module() at linker_load_module+0xacc                               
kern_kldload() at kern_kldload+0x190                                           
sys_kldload() at sys_kldload+0x64                                              
do_el0_sync() at do_el0_sync+0x59c                                             
handle_el0_sync() at handle_el0_sync+0x48

In particular, gentbi_probe() obviously didn't acquire the softc lock.
Comment 1 Mark Johnston freebsd_committer freebsd_triage 2023-11-04 16:24:01 UTC 
https://reviews.freebsd.org/D42466
Comment 2 commit-hook freebsd_committer freebsd_triage 2023-11-06 20:10:09 UTC 
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=725962a9f4c050b21488edd58d317e87c76d6f66

commit 725962a9f4c050b21488edd58d317e87c76d6f66
Author:     Mark Johnston <markj@FreeBSD.org>
AuthorDate: 2023-11-06 19:57:56 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2023-11-06 19:57:56 +0000

    e6000sw: Fix locking in miibus_{read,write}reg implementations

    Commit 469290648005e13b819a19353032ca53dda4378f made e6000sw's
    implementation of miibus_(read|write)reg assume that the softc lock is
    held.  I presume that is to avoid lock recursion in e6000sw_attach() ->
    e6000sw_attach_miibus() -> mii_attach() -> MIIBUS_READREG().

    However, the lock assertion in e6000sw_readphy_locked() can fail if a
    different driver uses the interface to probe registers.  Work around the
    problem by providing implementations which lock the softc if it is not
    already locked.

    PR:             274795
    Fixes:          469290648005 ("e6000sw: add readphy and writephy wrappers")
    Reviewed by:    kp, imp
    MFC after:      1 week
    Differential Revision:  https://reviews.freebsd.org/D42466

 sys/dev/etherswitch/e6000sw/e6000sw.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)
Comment 3 commit-hook freebsd_committer freebsd_triage 2023-11-13 15:20:40 UTC 
A commit in branch stable/14 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=766cceba19fe05095d81994e930a489ad545d42b

commit 766cceba19fe05095d81994e930a489ad545d42b
Author:     Mark Johnston <markj@FreeBSD.org>
AuthorDate: 2023-11-06 19:57:56 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2023-11-13 14:41:50 +0000

    e6000sw: Fix locking in miibus_{read,write}reg implementations

    Commit 469290648005e13b819a19353032ca53dda4378f made e6000sw's
    implementation of miibus_(read|write)reg assume that the softc lock is
    held.  I presume that is to avoid lock recursion in e6000sw_attach() ->
    e6000sw_attach_miibus() -> mii_attach() -> MIIBUS_READREG().

    However, the lock assertion in e6000sw_readphy_locked() can fail if a
    different driver uses the interface to probe registers.  Work around the
    problem by providing implementations which lock the softc if it is not
    already locked.

    PR:             274795
    Fixes:          469290648005 ("e6000sw: add readphy and writephy wrappers")
    Reviewed by:    kp, imp
    MFC after:      1 week
    Differential Revision:  https://reviews.freebsd.org/D42466

    (cherry picked from commit 725962a9f4c050b21488edd58d317e87c76d6f66)

 sys/dev/etherswitch/e6000sw/e6000sw.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)