Page MenuHomeFreeBSD

lagg: wrap lagg_port2req() into LAGG_SLOCK()
ClosedPublic

Authored by glebius on Jan 18 2024, 7:20 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Jan 20, 10:38 PM
Unknown Object (File)
Thu, Jan 2, 2:09 PM
Unknown Object (File)
Wed, Jan 1, 4:51 PM
Unknown Object (File)
Dec 5 2024, 5:45 PM
Unknown Object (File)
Nov 20 2024, 9:06 AM
Unknown Object (File)
Nov 16 2024, 3:54 PM
Unknown Object (File)
Nov 8 2024, 5:14 AM
Unknown Object (File)
Nov 7 2024, 10:49 AM
Subscribers

Details

Summary

Although a port addition is coded in a sequence where first all softc
information is fulfilled and only then it is attached to the lagg, we
still need a locking primitive to guarantee cache invalidation. Panic
observed in the wild shows that lacp_portreq() called via
lagg_port_ioctl(SIOCGLAGGPORT) immediately after port creation may see
lp->lp_psc as NULL and panic. In the core file we will see valid data
all around. A race via lagg_ioctl() wasn't observed but potentially
is possible.

Diff Detail

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

Event Timeline

sys/net/if_lagg.c
1049

ifunit() calls CK_STAILQ_FOREACH() so I guess there should not be cache invalidation issues.

I'm getting confused.

sys/net/if_lagg.c
1049

This isn't related to ifunit() at all. The whole code path of SIOCGLAGGPORT goes lockless. The syscall enters lacp_portreq() without taking any locks. There it may see a NULL lp->lp_psc and panic. The CPU the syscall executes on did not take any measures to synchronize the the CPU that did lacp_port_create() and assigned lp_psc to a value.

glebius retitled this revision from lagg: wrap lagg_port2req() into LAGG_XLOCK() to lagg: wrap lagg_port2req() into LAGG_SLOCK().Feb 21 2024, 5:56 PM

Use a shared lock. That should be sufficient.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 24 2024, 1:57 AM
This revision was automatically updated to reflect the committed changes.