Return-Path: Received: from mail-wr0-f194.google.com ([209.85.128.194]:34170 "EHLO mail-wr0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753821AbdBPG4V (ORCPT ); Thu, 16 Feb 2017 01:56:21 -0500 Received: by mail-wr0-f194.google.com with SMTP id c4so865695wrd.1 for ; Wed, 15 Feb 2017 22:56:20 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <1486827922.3490.2.camel@primarydata.com> References: <1486739454.19587.1.camel@primarydata.com> <1486827922.3490.2.camel@primarydata.com> From: Pankaj Singh Date: Thu, 16 Feb 2017 12:26:18 +0530 Message-ID: Subject: Re: How NLM support posix threads? To: Trond Myklebust Cc: "linux-nfs@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-nfs-owner@vger.kernel.org List-ID: Hi Trond Myklebust, I have one point to make, after this fix check for "block->b_file == file" in nlmsvc_lookup_block will become redundant. "if (block->b_file == file && nlm_compare_locks(fl, &lock->fl)) {". So this check can be removed and only "nlm_compare_locks" will be enough. Thanks, ~Pankaj Singh On Sat, Feb 11, 2017 at 9:15 PM, Trond Myklebust wrote: > On Sat, 2017-02-11 at 11:49 +0530, Pankaj Singh wrote: >> > During "fl_grant" callback wrong "block" will be compared hence >> > this will result in lock failure even if lock is actually granted. >> >> "nlm_compare_locks" will compare the locks on the bases of pid, >> owner, >> start offset, end offset and type. In case of posix threads as pid is >> same, also all other parameters can be same for locks of different >> files. >> >> Now the scenario is, if there are two posix thread with pid p1 and >> they try to take the lock on different files (say l1 and l2) then >> different blocks will be created (say b1 and b2). In this case >> underline filesystem may send "FILE_LOCK_DEFERRED" for both the locks >> and these blocks will be added to deferred block list. >> >> So during "fl_grant" callback lock are compared with the blocks of >> block list. Now lets say callback is called for l1 and the comparison >> will succeed with b1 (this is as expected) and B_GOT_CALLBACK flag >> will be set. But as b1 is still in block list, now when callback for >> l2 arrives then also comparison with b1 can succeed instead of b2 >> because b1 is prior to b2 in the list. Hence B_GOT_CALLBACK flag will >> not be set for b2 and when NFS client will retry for lock then lock >> will be denied. >> >> Below is the snipt fl_grant code where comparison happens. >> list_for_each_entry(block, &nlm_blocked, b_list) { >> if (nlm_compare_locks(&block->b_call->a_args.lock.fl, fl) > > OK. I see what you mean. You are saying, in essence, that in the lockd > server code, nlmsvc_notify_blocked() needs to check that the files are > the same (just like nlmsvc_lookup_block() already does). > > I agree. That is a bug and it needs to be fixed. How about the > following? > > Cheers > Trond > 8<--------------------------------------------------------------- > From fff75e35ed857b9ad211905fee2acffa528696d9 Mon Sep 17 00:00:00 2001 > From: Trond Myklebust > Date: Sat, 11 Feb 2017 10:37:38 -0500 > Subject: [PATCH] nlm: Ensure callback code also checks that the files match > > It is not sufficient to just check that the lock pids match when > granting a callback, we also need to ensure that we're granting > the callback on the right file. > > Reported-by: Pankaj Singh > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Cc: stable@vger.kernel.org > Signed-off-by: Trond Myklebust > --- > include/linux/lockd/lockd.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h > index c15373894a42..b37dee3acaba 100644 > --- a/include/linux/lockd/lockd.h > +++ b/include/linux/lockd/lockd.h > @@ -355,7 +355,8 @@ static inline int nlm_privileged_requester(const struct svc_rqst *rqstp) > static inline int nlm_compare_locks(const struct file_lock *fl1, > const struct file_lock *fl2) > { > - return fl1->fl_pid == fl2->fl_pid > + return file_inode(fl1->fl_file) == file_inode(fl2->fl_file) > + && fl1->fl_pid == fl2->fl_pid > && fl1->fl_owner == fl2->fl_owner > && fl1->fl_start == fl2->fl_start > && fl1->fl_end == fl2->fl_end > -- > 2.9.3 > > -- > Trond Myklebust > Linux NFS client maintainer, PrimaryData > trond.myklebust@primarydata.com -- Regards, Pankaj SIngh Phone No: 8826266889