Re: [PATCH] nfsd: fix delegation_blocked() to block correctly for at least 30 seconds

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2024-09-09 at 15:06 +1000, NeilBrown wrote:
> The pair of bloom filtered used by delegation_blocked() was intended to
> block delegations on given filehandles for between 30 and 60 seconds.  A
> new filehandle would be recorded in the "new" bit set.  That would then
> be switch to the "old" bit set between 0 and 30 seconds later, and it
> would remain as the "old" bit set for 30 seconds.
> 
> Unfortunately the code intended to clear the old bit set once it reached
> 30 seconds old, preparing it to be the next new bit set, instead cleared
> the *new* bit set before switching it to be the old bit set.  This means
> that the "old" bit set is always empty and delegations are blocked
> between 0 and 30 seconds.
> 
> This patch updates bd->new before clearing the set with that index,
> instead of afterwards.
> 
> Reported-by: Olga Kornievskaia <okorniev@redhat.com>
> Cc: stable@vger.kernel.org
> Fixes: 6282cd565553 ("NFSD: Don't hand out delegations for 30 seconds after recalling them.")
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/nfs4state.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 4313addbe756..6f18c1a7af2e 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1078,7 +1078,8 @@ static void nfs4_free_deleg(struct nfs4_stid *stid)
>   * When a delegation is recalled, the filehandle is stored in the "new"
>   * filter.
>   * Every 30 seconds we swap the filters and clear the "new" one,
> - * unless both are empty of course.
> + * unless both are empty of course.  This results in delegations for a
> + * given filehandle being blocked for between 30 and 60 seconds.
>   *
>   * Each filter is 256 bits.  We hash the filehandle to 32bit and use the
>   * low 3 bytes as hash-table indices.
> @@ -1107,9 +1108,9 @@ static int delegation_blocked(struct knfsd_fh *fh)
>  		if (ktime_get_seconds() - bd->swap_time > 30) {
>  			bd->entries -= bd->old_entries;
>  			bd->old_entries = bd->entries;
> +			bd->new = 1-bd->new;
>  			memset(bd->set[bd->new], 0,
>  			       sizeof(bd->set[0]));
> -			bd->new = 1-bd->new;
>  			bd->swap_time = ktime_get_seconds();
>  		}
>  		spin_unlock(&blocked_delegations_lock);

Nice catch!

Reviewed-by: Jeff Layton <jlayton@kernel.org>