Differential D12586
improve inp locking in getsockopt, setsockopt, and related functions Authored by jason_eggnet.com on Oct 4 2017, 2:03 AM. Tags None Referenced Files
Details
Diff Detail
Event TimelineComment Actions This issue was discovered by rare memory corruption when calling ip6_optlen(). The likely culprit was unlocked manipulation of in6p->in6p_outputopts. We use DSCP / Traffic Class, which manipulates in6p_outputopts without locking in6p. That lead to a fuller audit of locking the inp / in6p at the IPPROTO_IP / IPPROTO_IPV6 levels. setsockopt(s, IPPROTO_IPV6, IPV6_TCLASS, ...); I have performed some testing with this code with WITNESS / INVARIANTS / INVARIANT_SUPPORT. But wanted to get feedback from the community before wider testing, as these are rather extensive locking changes. Comment Actions Probably you need to modify netipsec/ipsec_pcb.c too.
Comment Actions I'll take a look.
Comment Actions
Comment Actions Overall, correcting missing locking around non-atomic sequences of option changes -- e.g., the read-modify-write sequences relating to IPSEC -- seems an excellent idea. I'm not convinced, however, that additional locking is required around many of the single-field in-place updates (e.g., of the TTL) on x86 (with TSO), and from our conversation in the transport-protocol call last night, it sounds like you are not encountering problems with them. However, it may make sense to make the change regardless, in order to improve maintainability -- i.e., in case those structure fields see other lock-protected updates than are currently in the source tree, which could themselves suffer races. And it seems unlikely that any are in fast paths, nor will experience substantial contention. @jhb will be in Cambridge next week and I'll chat with him about the atomic write aspect (e.g., in particular, while the stores to fields such as the TTL will be atomic on all architectures, guarantees about ordering and progress will be weaker on some, such as POWER and ARMv8), to get his take. So, overall, I'm supportive, but I would be tempted to break this out into two commits: (1) addressing definite bugs involving non-atomic read-modify write or compound write sequences that are known to cause problems (or definitely cause problems), and (2) those that are more discretionary changes that are unlikely to resolve bugs but may improve future maintainability. Comment Actions update outstanding issues I believe this diff covers all outstanding issues, it's a fairly Comment Actions It looks like there might be cases in ip_ctloutput, ip6_ctloutput, rip_ctloutput, and ip6_raw_ctloutput where the inp/in6p being null is ok. Another patch incoming to address that. Comment Actions It looks like a socket is guaranteed to have a single defined inp that is valid (specifically, not free'd) from the time socket or accept is called all the way to the time that close is called. Due to that, I'm going to unwind the in_pcbref usage which will greatly reduce this patch. Comment Actions I've been staring at this for a bit and think we may be able to use mbuf_tags(9) instead of malloc/free Comment Actions I am abandoning this in favor of 6 smaller reviews, per @rwatson's general advice to break up the patch. https://reviews.freebsd.org/D14619 - refactor ip6_getpcbopt() for better locking and memory management | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||