-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[1/3] Reserve SecureSession
for CASE establishment on the responder at init time
#19261
[1/3] Reserve SecureSession
for CASE establishment on the responder at init time
#19261
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments inline.
Looks good to me.
5e38410
to
9b166a8
Compare
PR #19261: Size comparison from c868f81 to 9b166a8 Increases above 0.2%:
Increases (32 builds for cc13x2_26x2, cyw30739, k32w, linux, mbed, p6, telink)
Decreases (9 builds for cc13x2_26x2, k32w)
Full report (32 builds for cc13x2_26x2, cyw30739, k32w, linux, mbed, p6, telink)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, though I have some questions on whether we can isolate the new previouslyEstablishedPeer
and sessionEvictionHint
parameters to just Cleanup()
.
- Semantic merge conflict between project-chip#19277 and project-chip#19261 - One line mismatch
Problem
To setup well for CASE session eviction, we need to ensure that the call to
SessionManager::AllocateSession
contains the node-id of the peer we're either about to establish a session to, or just established a session with.This is trivial when initiating a session on the initiator, but when responding to a session attempt, we currently do not have this information. This can result in unintentional denial-of-service from one fabric to another since we may incorrectly evict the wrong session based on vanilla LRU at the time of servicing Sigma1.
Solution
Instead of allocating a
SecureSession
when Sigma1 is received, always reserve aSecureSession
well in advance of actually needing to service Sigma1, i.e allocate one when we are about to listen for session establishment. In addition, always reserve 1 extraSecureSession
for this responder use-case.In the case of doing so right at boot, we know we'll always have at least 1 SecureSession available for allocation. In the more important case of just completing session establishment, we'll have the information about the peer on the just established session to help inform subsequent session allocation/eviction decisions.
Other changes:
CHIP_CONFIG_PEER_CONNECTION_POOL_SIZE
toCHIP_CONFIG_PEER_SECURE_SESSION_POOL_SIZE
since connection is less precise than the termSecureSession
.Testing
Using #19257, was able to validate that on completion of
ListenForSessionEstablishment
that the ref-count on the responder-sideSecureSession
was 2 (1 for the underlying SecureSessionHolder, and 1 for the extra strong-ref SessionHandle being stored in CASEServer).Also validated that after a successful session establishment, that the count drops back down to 1 (i.e retained by the SecureSessionManager).