[Cluster-devel] [gfs2 PATCH] gfs2: Don't skip dlm unlock if glock has an lvb

[Steven Whitehouse]

Hi,

Longer term we should review whether this is really the correct fix. It 
seems a bit strange that we have to do something different according to 
whether there is an LVB or not. We are gradually increasing LVB use over 
time too. So should we fix the DLM so that either it can cope with locks 
with LVBs at lockspace shutdown time, or should we simply send an unlock 
for all DLM locks anyway? That would seem to make more sense than having 
two different systems depending on LVB existence, or otherwise,

Steve.

On 05/02/2021 18:50, Bob Peterson wrote:
> Patch fb6791d100d1bba20b5cdbc4912e1f7086ec60f8 was designed to allow
> gfs2 to unmount quicker by skipping the step where it tells dlm to
> unlock glocks in EX with lvbs. This was done because when gfs2 unmounts
> a file system, it destroys the dlm lockspace shortly after it destroys
> the glocks so it doesn't need to unlock them all: the unlock is implied
> when the lockspace is destroyed by dlm.
>
> However, that patch introduced a use-after-free in dlm: as part of its
> normal dlm_recoverd process, it can call ls_recovery to recover dead
> locks. In so doing, it can call recover_rsbs which calls recover_lvb for
> any mastered rsbs. Func recover_lvb runs through the list of lkbs queued
> to the given rsb (if the glock is cached but unlocked, it will still be
> queued to the lkb, but in NL--Unlocked--mode) and if it has an lvb,
> copies it to the rsb, thus trying to preserve the lkb. However, when
> gfs2 skips the dlm unlock step, it frees the glock and its lvb, which
> means dlm's function recover_lvb references the now freed lvb pointer,
> copying the freed lvb memory to the rsb.
>
> This patch changes the check in gdlm_put_lock so that it calls dlm_unlock
> for all glocks that contain an lvb pointer.
>
> Signed-off-by: Bob Peterson <rpeterso at redhat.com>
> Fixes: fb6791d100d1 "GFS2: skip dlm_unlock calls in unmount"
> ---
>   fs/gfs2/lock_dlm.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/fs/gfs2/lock_dlm.c b/fs/gfs2/lock_dlm.c
> index 9f2b5609f225..153272f82984 100644
> --- a/fs/gfs2/lock_dlm.c
> +++ b/fs/gfs2/lock_dlm.c
> @@ -284,7 +284,6 @@ static void gdlm_put_lock(struct gfs2_glock *gl)
>   {
>   	struct gfs2_sbd *sdp = gl->gl_name.ln_sbd;
>   	struct lm_lockstruct *ls = &sdp->sd_lockstruct;
> -	int lvb_needs_unlock = 0;
>   	int error;
>   
>   	if (gl->gl_lksb.sb_lkid == 0) {
> @@ -297,13 +296,10 @@ static void gdlm_put_lock(struct gfs2_glock *gl)
>   	gfs2_sbstats_inc(gl, GFS2_LKS_DCOUNT);
>   	gfs2_update_request_times(gl);
>   
> -	/* don't want to skip dlm_unlock writing the lvb when lock is ex */
> -
> -	if (gl->gl_lksb.sb_lvbptr && (gl->gl_state == LM_ST_EXCLUSIVE))
> -		lvb_needs_unlock = 1;
> +	/* don't want to skip dlm_unlock writing the lvb when lock has one */
>   
>   	if (test_bit(SDF_SKIP_DLM_UNLOCK, &sdp->sd_flags) &&
> -	    !lvb_needs_unlock) {
> +	    !gl->gl_lksb.sb_lvbptr) {
>   		gfs2_glock_free(gl);
>   		return;
>   	}
>