On Fri, 2023-07-21 at 09:09 -0400, Alexander Aring wrote:
> 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?
>
The scenario is: we have a block on the list already and now another
lock request comes in for the same thing: the client decided to just re-
poll for the lock. That's a plausible scenario. I think the Linux NLM
client will poll for locks periodically.
In this case though, the lock request was granted by the filesystem, so
this is likely racing with (and winning vs.) a lm_grant callback. Given
that the client decided to repoll for it, we're probably safe to just
dequeue the block and respond here, and not worry about sending a grant
callback.
Ditto for the other cases where the block is removed.
> > ret = nlm_granted;
> > goto out;
> > case -EAGAIN:
> > + if (!wait)
> > + nlmsvc_remove_block(block);
I was thinking that it would be best to not insert a block at all in the
!wait case, but it looks like DLM just returns DEFERRED and almost
always does a callback, even when it's not a blocking lock request?
Anyway, I think we probably do have to handle this like you are.
> > +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
>
--
Jeff Layton <jlayton@xxxxxxxxxx>
