Page MenuHomeFreeBSD

iflib: add unlock to call ether_ifattach
ClosedPublic

Authored by przemyslawx.lewandowski_intel.com on Jun 15 2023, 7:05 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Oct 16, 7:58 AM
Unknown Object (File)
Oct 6 2024, 11:02 AM
Unknown Object (File)
Oct 4 2024, 11:13 AM
Unknown Object (File)
Oct 1 2024, 12:19 PM
Unknown Object (File)
Sep 27 2024, 4:29 AM
Unknown Object (File)
Sep 27 2024, 4:26 AM
Unknown Object (File)
Sep 8 2024, 5:38 PM
Unknown Object (File)
Sep 8 2024, 4:03 PM

Details

Summary

Panic occurs during loading driver using kldload. It exists since netlink is enabled.
There is problem with double locking ctx. This fix allows to call ether_ifattach() without locked ctx.

Bugzilla id: 271768
Part of panic:
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe010b8475d0
vpanic() at vpanic+0x150/frame 0xfffffe010b847620
panic() at panic+0x43/frame 0xfffffe010b847680
_sx_xlock_hard() at _sx_xlock_hard+0x82d/frame 0xfffffe010b847720
_sx_xlock() at _sx_xlock+0xb3/frame 0xfffffe010b847760
iflib_media_status() at iflib_media_status+0x32/frame 0xfffffe010b847790
ifmedia_ioctl() at ifmedia_ioctl+0x16a/frame 0xfffffe010b8477c0
dump_iface() at dump_iface+0x12f/frame 0xfffffe010b847840
rtnl_handle_ifevent() at rtnl_handle_ifevent+0xab/frame 0xfffffe010b8478c0
if_attach_internal() at if_attach_internal+0x41e/frame 0xfffffe010b847910
ether_ifattach() at ether_ifattach+0x29/frame 0xfffffe010b847940
iflib_device_register() at iflib_device_register+0xbf7/frame 0xfffffe010b8479d0
iflib_device_attach() at iflib_device_attach+0xb5/frame 0xfffffe010b847a00

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

I guess it deserves some discussion on the interface model part. The alternative fix can be done on the Netlink side (try to avoid calling driver-specific ioctls on the ifarrival event) or by moving ifnet_arrival event later in the chain.
My question is what are the desired ether_ifattach() and ifnet_arrival calls/events semantics. Does the ifnet_arrival event mean "interface is fully ready" so it can be queried/updated for the caller? If not, how can the event listener get notified on the readiness?

I think this change is logically correct. The driver lock needs to be a leaf lock and not held across calls into the generic ifnet layer IMO. You shouldn't be holding it across ether_ifdetach either. I would update the comment to not be netlink-specific though, it's just am old bug that was only exposed by netlink.

I guess it deserves some discussion on the interface model part. The alternative fix can be done on the Netlink side (try to avoid calling driver-specific ioctls on the ifarrival event) or by moving ifnet_arrival event later in the chain.
My question is what are the desired ether_ifattach() and ifnet_arrival calls/events semantics. Does the ifnet_arrival event mean "interface is fully ready" so it can be queried/updated for the caller? If not, how can the event listener get notified on the readiness?

We originally looked at it from the perspective that it was a problem with netlink and tried to see what we could do there, but as @jhb says, this is a bug in iflib and the code should get changed here.

This revision is now accepted and ready to land.Jun 20 2023, 9:57 PM