On Wed, May 09, 2018 at 08:02:49AM -0400, Andrew Elble wrote:
> I noticed a memory corruption crash in nfsd in
> 4.17-rc1. This patch corrects the issue.
>
> Fix to return error if the delegation couldn't be hashed or there was
> a recall in progress. Use the existing error path instead of
> destroy_unhashed_delegation() for readability. Set the fields of the
> delegation to indicate that it does not need to be recalled.
>
> Signed-off-by: Andrew Elble <aweits@xxxxxxx>
> Fixes: 353601e7d323c ("nfsd: create a separate lease for each delegation")
> ---
> v2: typo in changelog, set delegation recall-suppression
> fs/nfsd/nfs4state.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 71b87738c015..20463944cd61 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4372,12 +4372,26 @@ static struct file_lock *nfs4_alloc_init_lease(struct nfs4_delegation *dp,
> status = -EAGAIN;
> else
> status = hash_delegation_locked(dp, fp);
> + /*
> + * This delegation is doomed, tell the recall logic
> + * that it's being destroyed here.
> + */
> +
> + if (status) {
> + dp->dl_time++;
> + list_del_init(&dp->dl_recall_lru);
> + dp->dl_stid.sc_type = NFS4_CLOSED_DELEG_STID;
> + }
I'm trying to figure out if this fixes an actual bug. The code should
be able to deal with a callback on an already unhashed delegation, so I
think you're right it would at worst just be an unnecessary recall.
This won't catch every such case (could be that nfsd4_cb_recall_prepare
already ran and we're too late), so I wonder if this is worth it.
More interesting to me is what exactly it would take to hit this
case.... Another thread would have to have succesfully hashed a
delegation for this client and file to make our hash_delegation_locked
fail. So there would be two leases for the same file and client, but
with different delegation pointers as the fl_owner. I *think* we handle
that OK. But it was likely problematic previously when we were still
using the file pointer as the fl_owner.
--b.
> spin_unlock(&fp->fi_lock);
> spin_unlock(&state_lock);
>
> if (status)
> - destroy_unhashed_deleg(dp);
> + goto out_unlock;
> +
> return dp;
> +
> +out_unlock:
> + vfs_setlease(fp->fi_deleg_file, F_UNLCK, NULL, (void **)&dp);
> out_clnt_odstate:
> put_clnt_odstate(dp->dl_clnt_odstate);
> out_stid:
> --
> 1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
