iflib: add unlock to call ether_ifattach
ClosedPublic
Actions

Authored by przemyslawx.lewandowski_intel.com on Jun 15 2023, 7:05 AM.

Details

Reviewers

erj
shurd
jhb

Group Reviewers

Intel Networking
iflib

Commits

rG2617bca5a8c9: iflib: Unlock ctx lock around call to ether_ifattach()
rG93f39b467586: iflib: Unlock ctx lock around call to ether_ifattach()
rGa52f23f4c49e: iflib: Unlock ctx lock around call to ether_ifattach()

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

przemyslawx.lewandowski_intel.com created this revision.Jun 15 2023, 7:05 AM

Herald added a reviewer: shurd. · View Herald TranscriptJun 15 2023, 7:05 AM

Herald added 1 blocking reviewer(s): iflib. · View Herald Transcript

Herald added subscribers: glebius, melifaro, ae. · View Herald Transcript

przemyslawx.lewandowski_intel.com requested review of this revision.Jun 15 2023, 7:05 AM

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.

Updated comment.

jhb accepted this revision as: jhb.Jun 16 2023, 11:33 PM

In D40557#923239, @melifaro wrote:

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

Closed by commit rGa52f23f4c49e: iflib: Unlock ctx lock around call to ether_ifattach() (authored by erj). · Explain WhyJul 19 2023, 11:38 PM

This revision was automatically updated to reflect the committed changes.

erj added a commit: rGa52f23f4c49e: iflib: Unlock ctx lock around call to ether_ifattach().

Herald added a subscriber: imp. · View Herald TranscriptJul 19 2023, 11:38 PM

erj added a commit: rG93f39b467586: iflib: Unlock ctx lock around call to ether_ifattach().Jul 20 2023, 10:48 PM

erj added a commit: rG2617bca5a8c9: iflib: Unlock ctx lock around call to ether_ifattach().Jul 20 2023, 10:58 PM