Hi,
On Thu, Jul 20, 2023 at 8:58 AM Alexander Aring <aahringo@xxxxxxxxxx> wrote:
>
> This patch fixes a race in async lock request handling between adding
> the relevant struct nlm_block to nlm_blocked list after the request was
> sent by vfs_lock_file() and nlmsvc_grant_deferred() does a lookup of the
> nlm_block in the nlm_blocked list. It could be that the async request is
> completed before the nlm_block was added to the list. This would end
> in a -ENOENT and a kernel log message of "lockd: grant for unknown
> block".
>
> To solve this issue we add the nlm_block before the vfs_lock_file() call
> to be sure it has been added when a possible nlmsvc_grant_deferred() is
> called. If the vfs_lock_file() results in an case when it wouldn't be
> added to nlm_blocked list, the nlm_block struct will be removed from
> this list again.
>
> Signed-off-by: Alexander Aring <aahringo@xxxxxxxxxx>
> ---
> fs/lockd/svclock.c | 80 +++++++++++++++++++++++++++----------
> include/linux/lockd/lockd.h | 1 +
> 2 files changed, 60 insertions(+), 21 deletions(-)
>
> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> index 28abec5c451d..62ef27a69a9e 100644
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -297,6 +297,8 @@ static void nlmsvc_free_block(struct kref *kref)
>
> dprintk("lockd: freeing block %p...\n", block);
>
> + WARN_ON_ONCE(block->b_flags & B_PENDING_CALLBACK);
> +
> /* Remove block from file's list of blocks */
> list_del_init(&block->b_flist);
> mutex_unlock(&file->f_mutex);
> @@ -543,6 +545,12 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
> goto out;
> }
>
> + if (block->b_flags & B_PENDING_CALLBACK)
> + goto pending_request;
> +
> + /* Append to list of blocked */
> + nlmsvc_insert_block(block, NLM_NEVER);
> +
> if (!wait)
> lock->fl.fl_flags &= ~FL_SLEEP;
> mode = lock_to_openmode(&lock->fl);
> @@ -552,9 +560,13 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
> dprintk("lockd: vfs_lock_file returned %d\n", error);
> switch (error) {
> case 0:
> + nlmsvc_remove_block(block);
reacting here with nlmsvc_remove_block() assumes that the block was
not being added to the nlm_blocked list before nlmsvc_insert_block()
was called. I am not sure if this is always the case here.
Does somebody see a problem with that?
> ret = nlm_granted;
> goto out;
> case -EAGAIN:
> + if (!wait)
> + nlmsvc_remove_block(block);
> +pending_request:
> /*
> * If this is a blocking request for an
> * already pending lock request then we need
> @@ -565,6 +577,8 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
> ret = async_block ? nlm_lck_blocked : nlm_lck_denied;
> goto out;
> case FILE_LOCK_DEFERRED:
> + block->b_flags |= B_PENDING_CALLBACK;
> +
> if (wait)
> break;
> /* Filesystem lock operation is in progress
> @@ -572,17 +586,16 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
> ret = nlmsvc_defer_lock_rqst(rqstp, block);
> goto out;
> case -EDEADLK:
> + nlmsvc_remove_block(block);
> ret = nlm_deadlock;
> goto out;
> default: /* includes ENOLCK */
> + nlmsvc_remove_block(block);
> ret = nlm_lck_denied_nolocks;
> goto out;
> }
>
> ret = nlm_lck_blocked;
> -
> - /* Append to list of blocked */
> - nlmsvc_insert_block(block, NLM_NEVER);
> out:
> mutex_unlock(&file->f_mutex);
> nlmsvc_release_block(block);
> @@ -739,34 +752,59 @@ nlmsvc_update_deferred_block(struct nlm_block *block, int result)
> block->b_flags |= B_TIMED_OUT;
> }
- Alex
