On Mon, Aug 25, 2025 at 06:36:18PM +0200, Martin Wilck wrote: > On Fri, 2025-07-25 at 23:58 -0400, Benjamin Marzinski wrote: > > There is a race condition when changing reservation keys where a > > failed > > path could come back online after libmpathpersist checks the paths, > > but > > before it updates the reservation key. In this case, the path would > > come > > up and get reregistered with the old key by multipathd, and > > libmpathpersist would not update its key, because the path was down > > when it checked. > > > > To fix this, check the paths after updating the key, so any path that > > comes up after getting checked will use the updated key. > > In do_mpath_persistent_reserve_out(), you call update_prkey_flags() > before checking for MPATH_PR_SYNTAX_ERROR. Perhaps you should check the > key parameters first?
We could, but the way the code currently is, if you called update_prkey_flags() earlier, you can't fail those MPATH_PR_SYNTAX_ERROR checks. You can only call update_prkey_flags() if rq_servact is MPATH_PROUT_REG_IGN_SA or MPATH_PROUT_REG_SA, so we only need to look at the True branch of the outermost if statement in those MPATH_PR_SYNTAX_ERROR checks. We also cannot fail those checks if unregistering is true, so we only care about the case were paramp->sa_key is non-zero. And when we call update_prkey_flags(), we also copy the value of paramp->sa_key to mpp->reservation_key, so it too must be non-zero. This means that can't fail the checks since they check if mpp->reservation_key is zero or not equal to paramp->sa_key. But it doesn't hury anything to move the update_prkey_flags() call to after those checks either. So I'm fine with either solving this by adding an explanation of why we don't need to worry about failing these checks if we updated the key to the comments above the checks, or by just moving the update_prkey_flags() call to after them. Do you have a preference? -Ben > Martin
