Have everything in in sprot use the default retries - #2657
Conversation
We have a constant `DEFAULT_ATTEMPTS` for sprot retries that is used inconsistently. A handful of functions have no retries. On at least one sidecar system we've seen sprot need at least one retry on a cold boot. If the first sprot command happens to be one with no retry this will result in a timeout. This is annoying so just make everything that was previously trying one time use `DEFAULT_ATTEMPTS`.
|
Some of the operations are not idempotent, the reset message being the most obvious. However, in the case of reset, it winds up being ok in practice because RoT reset takes more than 2 seconds. We expect a timeout on success for reset. "record" is also not idempotent. If the record message included the slot number it expected to fill then it would be idempotent because the RoT could reject the message if appending the attestation log would fill a different slot. Some of the software update related operations are also not idempotent though they all fail safe if received more than once or at inappropriate times. We should improve the logic related to the At a minimum, the cases where retry count is not the default should be using a named constant that indicates why the non-default value is being used. There might be an argument for reducing the default to be 2 instead of 3, the first try in case the RoT was not listening, and the 2nd because the RoT has no excuses at that point. But, analysis more careful that that should be done. |
lzrd
left a comment
lzrd
left a comment
There was a problem hiding this comment.
Some operations are not idempotent and the logic about what is ok to retry should be improved. At a minimum, non-idempotent operations should be better identified.
|
I do see the point about idempotentcy but I also don't think we can really argue that's a property we have now. The original commit where this was introduced
I see the argument for This change adjusts the following functions: These should actually be idempotent and safe May not actually be idempotent I can drop the ones that are not technically idempotent from this commit since those seem pretty low risk but I'd like to make a case to keep |
|
I ran a Claude session to verify what is idempotent or safely repeatable.
We could better track "RoT known to be in sync" state and if in question do a cs_pulse to force sync. I'd rather that than do it unilaterally. Right now, any message with only 1 attempt budgeted may spend that on syncing the RoT's sprot server's state machine. I think that we should look at "enable_sp_slot_watchdog" having a parameter that says which slot is the recovery slot rather than ask for a toggle operation. But that would be a different PR. |
lzrd
left a comment
lzrd
left a comment
There was a problem hiding this comment.
While we're here, I think it's worth changing "retries" to "attempts" and fixing the math/comparison in the attempt loop. Otherwise it look good.
There was a problem hiding this comment.
This name is misleading. See line 478 where attempts_left is the number of "retries".
Also, retries is a u16 and can be decremented below zero at the end of the loop. It is tested against zero.
So at least one attempt will always be made and the math and comparison at the end of the loop should be improved.
There was a problem hiding this comment.
I changed the name to be "attempts".
I agree there's a potential issue if we pass in zero attempts. The failure mode to trigger it would have to be passing 0 from a function that calls do_send_recv_retries and have that first call fail. Passing in 0 for attempts is arguably a driver bug but I think this code could do with some refactoring to better make it obvious what's going on. I think that's also outside the immediate scope of this change.
There was a problem hiding this comment.
we also already pass overflow-checks=y to all code so the failure mode should also be a panic (ugly but arguably safe and maybe equivalent to what we want anyway)
| tx_size, | ||
| TIMEOUT_QUICK, | ||
| // This is not idempotent so only retry once | ||
| 1, |
There was a problem hiding this comment.
We should rename this parameter to indicate it is the number of attempts, not the number of retries.
| /// Reset the RoT | ||
| fn reset( | ||
| &mut self, | ||
| _msg: &userlib::RecvMessage, | ||
| ) -> Result<(), idol_runtime::RequestError<SprotError>> { | ||
| let body = ReqBody::Update(UpdateReq::Reset); | ||
| let tx_size = Request::pack(&body, self.tx_buf); | ||
| let rsp = self.do_send_recv_retries(tx_size, TIMEOUT_QUICK, 1)?; | ||
| let rsp = self.do_send_recv_retries( | ||
| tx_size, | ||
| TIMEOUT_QUICK, | ||
| DEFAULT_ATTEMPTS, | ||
| )?; |
There was a problem hiding this comment.
I think keeping this consistent with the other code to be DEFAULT_ATTEMPTS is cleaner than trying to optimize it to 2
2 participants
We have a constant
DEFAULT_ATTEMPTSfor sprot retries that is used inconsistently. A handful of functions have no retries. On at least one sidecar system we've seen sprot need at least one retry on a cold boot. If the first sprot command happens to be one with no retry this will result in a timeout. This is annoying so just make everything that was previously trying one time useDEFAULT_ATTEMPTS.