Re: [PATCH 12/14] libmpatpersist: update reservation key before checking paths

[Benjamin Marzinski <bmarzins@redhat.com>]

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