* How NLM support posix threads? @ 2017-02-10 9:21 Pankaj Singh 2017-02-10 15:10 ` Trond Myklebust 0 siblings, 1 reply; 7+ messages in thread From: Pankaj Singh @ 2017-02-10 9:21 UTC (permalink / raw) To: linux-nfs 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: How NLM support posix threads? 2017-02-10 9:21 How NLM support posix threads? Pankaj Singh @ 2017-02-10 15:10 ` Trond Myklebust 2017-02-11 6:19 ` Pankaj Singh 0 siblings, 1 reply; 7+ messages in thread From: Trond Myklebust @ 2017-02-10 15:10 UTC (permalink / raw) To: psingh.ait, linux-nfs 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== ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: How NLM support posix threads? 2017-02-10 15:10 ` Trond Myklebust @ 2017-02-11 6:19 ` Pankaj Singh 2017-02-11 12:50 ` Jeff Layton 2017-02-11 15:45 ` Trond Myklebust 0 siblings, 2 replies; 7+ messages in thread From: Pankaj Singh @ 2017-02-11 6:19 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: How NLM support posix threads? 2017-02-11 6:19 ` Pankaj Singh @ 2017-02-11 12:50 ` Jeff Layton 2017-02-11 15:45 ` Trond Myklebust 1 sibling, 0 replies; 7+ messages in thread From: Jeff Layton @ 2017-02-11 12:50 UTC (permalink / raw) To: Pankaj Singh, Trond Myklebust; +Cc: linux-nfs 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: How NLM support posix threads? 2017-02-11 6:19 ` Pankaj Singh 2017-02-11 12:50 ` Jeff Layton @ 2017-02-11 15:45 ` Trond Myklebust 2017-02-13 13:47 ` Pankaj Singh 2017-02-16 6:56 ` Pankaj Singh 1 sibling, 2 replies; 7+ messages in thread From: Trond Myklebust @ 2017-02-11 15:45 UTC (permalink / raw) To: psingh.ait; +Cc: linux-nfs 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== ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: How NLM support posix threads? 2017-02-11 15:45 ` Trond Myklebust @ 2017-02-13 13:47 ` Pankaj Singh 2017-02-16 6:56 ` Pankaj Singh 1 sibling, 0 replies; 7+ messages in thread From: Pankaj Singh @ 2017-02-13 13:47 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: How NLM support posix threads? 2017-02-11 15:45 ` Trond Myklebust 2017-02-13 13:47 ` Pankaj Singh @ 2017-02-16 6:56 ` Pankaj Singh 1 sibling, 0 replies; 7+ messages in thread From: Pankaj Singh @ 2017-02-16 6:56 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-02-16 6:56 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-10 9:21 How NLM support posix threads? Pankaj Singh 2017-02-10 15:10 ` Trond Myklebust 2017-02-11 6:19 ` Pankaj Singh 2017-02-11 12:50 ` Jeff Layton 2017-02-11 15:45 ` Trond Myklebust 2017-02-13 13:47 ` Pankaj Singh 2017-02-16 6:56 ` Pankaj Singh
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.