How NLM support posix threads?
diff mbox

Message ID 1486827922.3490.2.camel@primarydata.com
State New
Headers show

Commit Message

Trond Myklebust Feb. 11, 2017, 3:45 p.m. UTC
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 <trond.myklebust@primarydata.com>

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 <psingh.ait@gmail.com>
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>

---
 include/linux/lockd/lockd.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.9.3

-- 
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@primarydata.com

Comments

Pankaj Singh Feb. 13, 2017, 1:47 p.m. UTC | #1
Hi Trond,

> I agree. That is a bug and it needs to be fixed. How about the
following?

Thanks for acknowledging the bug. Fix seems to be good.

On Sat, Feb 11, 2017 at 9:15 PM, Trond Myklebust
<trondmy@primarydata.com> 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 <trond.myklebust@primarydata.com>
> 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 <psingh.ait@gmail.com>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> ---
>  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
Pankaj Singh Feb. 16, 2017, 6:56 a.m. UTC | #2
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
<trondmy@primarydata.com> 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 <trond.myklebust@primarydata.com>
> 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 <psingh.ait@gmail.com>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> ---
>  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

Patch
diff mbox

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