Page MenuHomeFreeBSD

tcp: don't upgrade a lock just for logging
ClosedPublic

Authored by tuexen on Nov 23 2021, 6:08 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 28, 6:23 AM
Unknown Object (File)
Wed, Nov 20, 6:31 AM
Unknown Object (File)
Oct 26 2024, 9:03 PM
Unknown Object (File)
Oct 24 2024, 11:23 AM
Unknown Object (File)
Oct 23 2024, 4:41 AM
Unknown Object (File)
Oct 18 2024, 4:38 PM
Unknown Object (File)
Oct 18 2024, 7:13 AM
Unknown Object (File)
Oct 9 2024, 12:36 AM

Details

Summary

When a outgoing packet can not be logged, because only a read lock is held, don't
upgrade the the lock. Upgrading the lock might harm in case a SYN flood is ongoing.

Diff Detail

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

Event Timeline

sys/netinet/tcp_subr.c
2117

I'd suggest deleting We can not log the packet, since we only own the read lock, but a write lock is needed. as it's not very relevant even though true. Someone could read this comment later and wonder why not try the wlock upgrade approach or another method to ensure the log is generated.

What is more relevant and IMO should be captured in this comment block in place of the above sentence is that the 2 cases which pass through this block are considered to be events which are not appropriately associated with the tcpcb/connection, and therefore we are explicitly choosing not to log them.

Minor language/grammar nit:
... CLOSED state or received in a state later then listen is the UDP encapsulation port is unexpected.
->
... CLOSED state, or received in a more advanced state than LISTEN and the UDP encapsulation port does not match.

Improve comment based on feedback from lstewart@.

sys/netinet/tcp_subr.c
2117

I kept the statement about the read/write lock and added an explanation, why we don't do the upgrade dance.

I also added that the segments don't need to be logged since we don't consider them belonging to the connection. I also added a note, that the incoming SYN segments are also not logged.

Looks like just what we agreed too ;)

This revision is now accepted and ready to land.Nov 29 2021, 12:18 PM