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)
Oct 2 2024, 12:26 PM
Unknown Object (File)
Sep 24 2024, 11:56 AM
Unknown Object (File)
Sep 23 2024, 6:56 PM
Unknown Object (File)
Sep 20 2024, 6:23 AM
Unknown Object (File)
Sep 19 2024, 3:02 PM
Unknown Object (File)
Sep 19 2024, 1:17 AM
Unknown Object (File)
Sep 17 2024, 1:42 AM
Unknown Object (File)
Sep 16 2024, 12:26 PM
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.