How NLM support posix threads?

How NLM support posix threads?

[Pankaj Singh]

Hi,

I am getting a strange problem where locks are denied by NFSv3 server
even though no locks on same file where taken.

While looking at the code it seems like NLM compares a lock by using
"pid" and other options like form - to file offset, type of lock etc.
But we are interested in "pid" because other comparison can be same
for different file_locks.

 As we know posix threads use tgid as pid for all its thread hence
NFSv3 server will can get lock request from different posix thread but
with same pid. Hence NLM  will treat the locking request as they are
coming from same process. This may result in following problems

1. Different threads of same process can get lock on same file.
2. During "fl_grant" callback wrong "block" will be compared hence
this will result in lock failure even if lock is actually granted.

Is this a limitation of NLM?

-- 
Regards,
Pankaj SIngh
Phone No: 8826266889

Re: How NLM support posix threads?

[Trond Myklebust]

T24gRnJpLCAyMDE3LTAyLTEwIGF0IDE0OjUxICswNTMwLCBQYW5rYWogU2luZ2ggd3JvdGU6DQo+
IEhpLA0KPiANCj4gSSBhbSBnZXR0aW5nIGEgc3RyYW5nZSBwcm9ibGVtIHdoZXJlIGxvY2tzIGFy
ZSBkZW5pZWQgYnkgTkZTdjMgc2VydmVyDQo+IGV2ZW4gdGhvdWdoIG5vIGxvY2tzIG9uIHNhbWUg
ZmlsZSB3aGVyZSB0YWtlbi4NCj4gDQo+IFdoaWxlIGxvb2tpbmcgYXQgdGhlIGNvZGUgaXQgc2Vl
bXMgbGlrZSBOTE0gY29tcGFyZXMgYSBsb2NrIGJ5IHVzaW5nDQo+ICJwaWQiIGFuZCBvdGhlciBv
cHRpb25zIGxpa2UgZm9ybSAtIHRvIGZpbGUgb2Zmc2V0LCB0eXBlIG9mIGxvY2sgZXRjLg0KPiBC
dXQgd2UgYXJlIGludGVyZXN0ZWQgaW4gInBpZCIgYmVjYXVzZSBvdGhlciBjb21wYXJpc29uIGNh
biBiZSBzYW1lDQo+IGZvciBkaWZmZXJlbnQgZmlsZV9sb2Nrcy4NCj4gDQo+IMKgQXMgd2Uga25v
dyBwb3NpeCB0aHJlYWRzIHVzZSB0Z2lkIGFzIHBpZCBmb3IgYWxsIGl0cyB0aHJlYWQgaGVuY2UN
Cj4gTkZTdjMgc2VydmVyIHdpbGwgY2FuIGdldCBsb2NrIHJlcXVlc3QgZnJvbSBkaWZmZXJlbnQg
cG9zaXggdGhyZWFkDQo+IGJ1dA0KPiB3aXRoIHNhbWUgcGlkLiBIZW5jZSBOTE3CoMKgd2lsbCB0
cmVhdCB0aGUgbG9ja2luZyByZXF1ZXN0IGFzIHRoZXkgYXJlDQo+IGNvbWluZyBmcm9tIHNhbWUg
cHJvY2Vzcy4gVGhpcyBtYXkgcmVzdWx0IGluIGZvbGxvd2luZyBwcm9ibGVtcw0KDQpUaHJlYWRz
IHRoYXQgc2hhcmUgdGhlIHNhbWUgcHJvY2VzcyBpZCBiZWxvbmcsIGJ5IGRlZmluaXRpb24sIHRv
IHRoZQ0Kc2FtZSBwcm9jZXNzLg0KDQo+IDEuIERpZmZlcmVudCB0aHJlYWRzIG9mIHNhbWUgcHJv
Y2VzcyBjYW4gZ2V0IGxvY2sgb24gc2FtZSBmaWxlLg0KDQpUaGF0IGlzIHByZWNpc2VseSBob3cg
UE9TSVggbG9ja3MgYXJlIGV4cGVjdGVkIHRvIHdvcmsgaW4gdGhyZWFkZWQNCmVudmlyb25tZW50
cy4NCmh0dHA6Ly9wdWJzLm9wZW5ncm91cC5vcmcvb25saW5lcHVicy85Njk5OTE5Nzk5L2Z1bmN0
aW9ucy9mY250bC5odG1sDQoiQW4gZXhjbHVzaXZlIGxvY2sgc2hhbGwgcHJldmVudCBhbnkgb3Ro
ZXIgcHJvY2VzcyBmcm9tIHNldHRpbmcgYQ0Kc2hhcmVkIGxvY2sgb3IgYW4gZXhjbHVzaXZlIGxv
Y2sgb24gYW55IHBvcnRpb24gb2YgdGhlIHByb3RlY3RlZCBhcmVhLiINCg0KDQo+IDIuIER1cmlu
ZyAiZmxfZ3JhbnQiIGNhbGxiYWNrIHdyb25nICJibG9jayIgd2lsbCBiZSBjb21wYXJlZCBoZW5j
ZQ0KPiB0aGlzIHdpbGwgcmVzdWx0IGluIGxvY2sgZmFpbHVyZSBldmVuIGlmIGxvY2sgaXMgYWN0
dWFsbHkgZ3JhbnRlZC4NCg0KSG93IHNvPw0KDQo+IElzIHRoaXMgYSBsaW1pdGF0aW9uIG9mIE5M
TT8NCj4gDQoNCg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRh
aW5lciwgUHJpbWFyeURhdGENCnRyb25kLm15a2xlYnVzdEBwcmltYXJ5ZGF0YS5jb20NCg==

Re: How NLM support posix threads?

[Pankaj Singh]

> 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))


On Fri, Feb 10, 2017 at 8:40 PM, Trond Myklebust
<trondmy@primarydata.com> wrote:
> On Fri, 2017-02-10 at 14:51 +0530, Pankaj Singh wrote:
>> Hi,
>>
>> I am getting a strange problem where locks are denied by NFSv3 server
>> even though no locks on same file where taken.
>>
>> While looking at the code it seems like NLM compares a lock by using
>> "pid" and other options like form - to file offset, type of lock etc.
>> But we are interested in "pid" because other comparison can be same
>> for different file_locks.
>>
>>  As we know posix threads use tgid as pid for all its thread hence
>> NFSv3 server will can get lock request from different posix thread
>> but
>> with same pid. Hence NLM  will treat the locking request as they are
>> coming from same process. This may result in following problems
>
> Threads that share the same process id belong, by definition, to the
> same process.
>
>> 1. Different threads of same process can get lock on same file.
>
> That is precisely how POSIX locks are expected to work in threaded
> environments.
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/fcntl.html
> "An exclusive lock shall prevent any other process from setting a
> shared lock or an exclusive lock on any portion of the protected area."
>
>
>> 2. During "fl_grant" callback wrong "block" will be compared hence
>> this will result in lock failure even if lock is actually granted.
>
> How so?
>
>> Is this a limitation of NLM?
>>
>
>
> --
> Trond Myklebust
> Linux NFS client maintainer, PrimaryData
> trond.myklebust@primarydata.com



-- 
Regards,
Pankaj SIngh
Phone No: 8826266889

Re: How NLM support posix threads?

[Jeff Layton]

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))
> 
> 

POSIX locks don't work the way you expect between threads, even on local
filesystems. The original spec for POSIX locking was written before
threads were really a "thing", so different threads within the same PID
can't have conflicting POSIX locks.

OFD locks [1] will work between threads, and the NFS client tries to
emulate flock() with NLM locks as well. The latter two are not well-
tested with NLM though, so there may be bugs there.

What might be best is to post a testcase for this, so we can see exactly
what you're trying to do, what you expect to happen, and what's really
happening.

Also, please post what kernel you're using as that may be relevant.

[1]: https://lwn.net/Articles/586904/


> On Fri, Feb 10, 2017 at 8:40 PM, Trond Myklebust
> <trondmy@primarydata.com> wrote:
> > On Fri, 2017-02-10 at 14:51 +0530, Pankaj Singh wrote:
> > > Hi,
> > > 
> > > I am getting a strange problem where locks are denied by NFSv3 server
> > > even though no locks on same file where taken.
> > > 
> > > While looking at the code it seems like NLM compares a lock by using
> > > "pid" and other options like form - to file offset, type of lock etc.
> > > But we are interested in "pid" because other comparison can be same
> > > for different file_locks.
> > > 
> > >  As we know posix threads use tgid as pid for all its thread hence
> > > NFSv3 server will can get lock request from different posix thread
> > > but
> > > with same pid. Hence NLM  will treat the locking request as they are
> > > coming from same process. This may result in following problems
> > 
> > Threads that share the same process id belong, by definition, to the
> > same process.
> > 
> > > 1. Different threads of same process can get lock on same file.
> > 
> > That is precisely how POSIX locks are expected to work in threaded
> > environments.
> > http://pubs.opengroup.org/onlinepubs/9699919799/functions/fcntl.html
> > "An exclusive lock shall prevent any other process from setting a
> > shared lock or an exclusive lock on any portion of the protected area."
> > 
> > 
> > > 2. During "fl_grant" callback wrong "block" will be compared hence
> > > this will result in lock failure even if lock is actually granted.
> > 
> > How so?
> > 
> > > Is this a limitation of NLM?
> > > 
> > 
> > 
> > --
> > Trond Myklebust
> > Linux NFS client maintainer, PrimaryData
> > trond.myklebust@primarydata.com
> 
> 
> 

-- 
Jeff Layton <jlayton@poochiereds.net>

Re: How NLM support posix threads?

[Trond Myklebust]

T24gU2F0LCAyMDE3LTAyLTExIGF0IDExOjQ5ICswNTMwLCBQYW5rYWogU2luZ2ggd3JvdGU6DQo+
ID4gRHVyaW5nICJmbF9ncmFudCIgY2FsbGJhY2sgd3JvbmcgImJsb2NrIiB3aWxsIGJlIGNvbXBh
cmVkIGhlbmNlDQo+ID4gdGhpcyB3aWxsIHJlc3VsdCBpbiBsb2NrIGZhaWx1cmUgZXZlbiBpZiBs
b2NrIGlzIGFjdHVhbGx5IGdyYW50ZWQuDQo+IA0KPiAibmxtX2NvbXBhcmVfbG9ja3MiIHdpbGwg
Y29tcGFyZSB0aGUgbG9ja3Mgb24gdGhlIGJhc2VzIG9mIHBpZCwNCj4gb3duZXIsDQo+IHN0YXJ0
IG9mZnNldCwgZW5kIG9mZnNldCBhbmQgdHlwZS4gSW4gY2FzZSBvZiBwb3NpeCB0aHJlYWRzIGFz
IHBpZCBpcw0KPiBzYW1lLCBhbHNvIGFsbCBvdGhlciBwYXJhbWV0ZXJzIGNhbiBiZSBzYW1lIGZv
ciBsb2NrcyBvZiBkaWZmZXJlbnQNCj4gZmlsZXMuDQo+IA0KPiBOb3cgdGhlIHNjZW5hcmlvIGlz
LCBpZiB0aGVyZSBhcmUgdHdvIHBvc2l4IHRocmVhZCB3aXRoIHBpZCBwMSBhbmQNCj4gdGhleSB0
cnkgdG8gdGFrZSB0aGUgbG9jayBvbiBkaWZmZXJlbnQgZmlsZXMgKHNheSBsMSBhbmQgbDIpIHRo
ZW4NCj4gZGlmZmVyZW50IGJsb2NrcyB3aWxsIGJlIGNyZWF0ZWQgKHNheSBiMSBhbmQgYjIpLiBJ
biB0aGlzIGNhc2UNCj4gdW5kZXJsaW5lIGZpbGVzeXN0ZW0gbWF5IHNlbmQgIkZJTEVfTE9DS19E
RUZFUlJFRCIgZm9yIGJvdGggdGhlIGxvY2tzDQo+IGFuZCB0aGVzZSBibG9ja3Mgd2lsbCBiZSBh
ZGRlZCB0byBkZWZlcnJlZCBibG9jayBsaXN0Lg0KPiANCj4gU28gZHVyaW5nICJmbF9ncmFudCIg
Y2FsbGJhY2sgbG9jayBhcmUgY29tcGFyZWQgd2l0aCB0aGUgYmxvY2tzIG9mDQo+IGJsb2NrIGxp
c3QuIE5vdyBsZXRzIHNheSBjYWxsYmFjayBpcyBjYWxsZWQgZm9yIGwxIGFuZCB0aGUgY29tcGFy
aXNvbg0KPiB3aWxsIHN1Y2NlZWQgd2l0aCBiMSAodGhpcyBpcyBhcyBleHBlY3RlZCkgYW5kIEJf
R09UX0NBTExCQUNLIGZsYWcNCj4gd2lsbCBiZSBzZXQuIEJ1dCBhcyBiMSBpcyBzdGlsbCBpbiBi
bG9jayBsaXN0LCBub3cgd2hlbiBjYWxsYmFjayBmb3INCj4gbDIgYXJyaXZlcyB0aGVuIGFsc28g
Y29tcGFyaXNvbiB3aXRoIGIxIGNhbiBzdWNjZWVkIGluc3RlYWQgb2YgYjINCj4gYmVjYXVzZSBi
MSBpcyBwcmlvciB0byBiMiBpbiB0aGUgbGlzdC4gSGVuY2UgQl9HT1RfQ0FMTEJBQ0sgZmxhZyB3
aWxsDQo+IG5vdCBiZSBzZXQgZm9yIGIyIGFuZCB3aGVuIE5GUyBjbGllbnQgd2lsbCByZXRyeSBm
b3IgbG9jayB0aGVuIGxvY2sNCj4gd2lsbCBiZSBkZW5pZWQuDQo+IA0KPiBCZWxvdyBpcyB0aGUg
c25pcHQgZmxfZ3JhbnQgY29kZSB3aGVyZSBjb21wYXJpc29uIGhhcHBlbnMuDQo+IGxpc3RfZm9y
X2VhY2hfZW50cnkoYmxvY2ssICZubG1fYmxvY2tlZCwgYl9saXN0KSB7DQo+IGlmIChubG1fY29t
cGFyZV9sb2NrcygmYmxvY2stPmJfY2FsbC0+YV9hcmdzLmxvY2suZmwsIGZsKQ0KDQpPSy4gSSBz
ZWUgd2hhdCB5b3UgbWVhbi4gWW91IGFyZSBzYXlpbmcsIGluIGVzc2VuY2UsIHRoYXQgaW4gdGhl
IGxvY2tkDQpzZXJ2ZXIgY29kZSwgbmxtc3ZjX25vdGlmeV9ibG9ja2VkKCkgbmVlZHMgdG8gY2hl
Y2sgdGhhdCB0aGUgZmlsZXMgYXJlDQp0aGUgc2FtZSAoanVzdCBsaWtlIG5sbXN2Y19sb29rdXBf
YmxvY2soKSBhbHJlYWR5IGRvZXMpLg0KDQpJIGFncmVlLiBUaGF0IGlzIGEgYnVnIGFuZCBpdCBu
ZWVkcyB0byBiZSBmaXhlZC4gSG93IGFib3V0IHRoZQ0KZm9sbG93aW5nPw0KDQpDaGVlcnMNCiAg
VHJvbmQNCjg8LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tDQpGcm9tIGZmZjc1ZTM1ZWQ4NTdiOWFkMjExOTA1ZmVlMmFjZmZhNTI4
Njk2ZDkgTW9uIFNlcCAxNyAwMDowMDowMCAyMDAxDQpGcm9tOiBUcm9uZCBNeWtsZWJ1c3QgPHRy
b25kLm15a2xlYnVzdEBwcmltYXJ5ZGF0YS5jb20+DQpEYXRlOiBTYXQsIDExIEZlYiAyMDE3IDEw
OjM3OjM4IC0wNTAwDQpTdWJqZWN0OiBbUEFUQ0hdIG5sbTogRW5zdXJlIGNhbGxiYWNrIGNvZGUg
YWxzbyBjaGVja3MgdGhhdCB0aGUgZmlsZXMgbWF0Y2gNCg0KSXQgaXMgbm90IHN1ZmZpY2llbnQg
dG8ganVzdCBjaGVjayB0aGF0IHRoZSBsb2NrIHBpZHMgbWF0Y2ggd2hlbg0KZ3JhbnRpbmcgYSBj
YWxsYmFjaywgd2UgYWxzbyBuZWVkIHRvIGVuc3VyZSB0aGF0IHdlJ3JlIGdyYW50aW5nDQp0aGUg
Y2FsbGJhY2sgb24gdGhlIHJpZ2h0IGZpbGUuDQoNClJlcG9ydGVkLWJ5OiBQYW5rYWogU2luZ2gg
PHBzaW5naC5haXRAZ21haWwuY29tPg0KRml4ZXM6IDFkYTE3N2U0YzNmNCAoIkxpbnV4LTIuNi4x
Mi1yYzIiKQ0KQ2M6IHN0YWJsZUB2Z2VyLmtlcm5lbC5vcmcNClNpZ25lZC1vZmYtYnk6IFRyb25k
IE15a2xlYnVzdCA8dHJvbmQubXlrbGVidXN0QHByaW1hcnlkYXRhLmNvbT4NCi0tLQ0KIGluY2x1
ZGUvbGludXgvbG9ja2QvbG9ja2QuaCB8IDMgKystDQogMSBmaWxlIGNoYW5nZWQsIDIgaW5zZXJ0
aW9ucygrKSwgMSBkZWxldGlvbigtKQ0KDQpkaWZmIC0tZ2l0IGEvaW5jbHVkZS9saW51eC9sb2Nr
ZC9sb2NrZC5oIGIvaW5jbHVkZS9saW51eC9sb2NrZC9sb2NrZC5oDQppbmRleCBjMTUzNzM4OTRh
NDIuLmIzN2RlZTNhY2FiYSAxMDA2NDQNCi0tLSBhL2luY2x1ZGUvbGludXgvbG9ja2QvbG9ja2Qu
aA0KKysrIGIvaW5jbHVkZS9saW51eC9sb2NrZC9sb2NrZC5oDQpAQCAtMzU1LDcgKzM1NSw4IEBA
IHN0YXRpYyBpbmxpbmUgaW50IG5sbV9wcml2aWxlZ2VkX3JlcXVlc3Rlcihjb25zdCBzdHJ1Y3Qg
c3ZjX3Jxc3QgKnJxc3RwKQ0KIHN0YXRpYyBpbmxpbmUgaW50IG5sbV9jb21wYXJlX2xvY2tzKGNv
bnN0IHN0cnVjdCBmaWxlX2xvY2sgKmZsMSwNCiAJCQkJICAgIGNvbnN0IHN0cnVjdCBmaWxlX2xv
Y2sgKmZsMikNCiB7DQotCXJldHVybglmbDEtPmZsX3BpZCAgID09IGZsMi0+ZmxfcGlkDQorCXJl
dHVybiBmaWxlX2lub2RlKGZsMS0+ZmxfZmlsZSkgPT0gZmlsZV9pbm9kZShmbDItPmZsX2ZpbGUp
DQorCSAgICAgJiYgZmwxLT5mbF9waWQgICA9PSBmbDItPmZsX3BpZA0KIAkgICAgICYmIGZsMS0+
Zmxfb3duZXIgPT0gZmwyLT5mbF9vd25lcg0KIAkgICAgICYmIGZsMS0+Zmxfc3RhcnQgPT0gZmwy
LT5mbF9zdGFydA0KIAkgICAgICYmIGZsMS0+ZmxfZW5kICAgPT0gZmwyLT5mbF9lbmQNCi0tIA0K
Mi45LjMNCg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRhaW5l
ciwgUHJpbWFyeURhdGENCnRyb25kLm15a2xlYnVzdEBwcmltYXJ5ZGF0YS5jb20NCg==

Re: How NLM support posix threads?

[Pankaj Singh]

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



-- 
Regards,
Pankaj SIngh
Phone No: 8826266889

Re: How NLM support posix threads?

[Pankaj Singh]

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



-- 
Regards,
Pankaj SIngh
Phone No: 8826266889