[PATCH] sunrpc.ko: RPC cache fix races

[PATCH] sunrpc.ko: RPC cache fix races

[bstroesser]

U2Vjb25kIGF0dGVtcHQgdXNpbmcgdGhlIGNvcnJlY3QgRlJPTS4gU29ycnkgZm9yIHRoZSBu
b2lzZS4KCgpIaSwKCkkgZm91bmQgYSBwcm9ibGVtIGluIHN1bnJwYy5rbyBvbiBhIFNMRVMx
MSBTUDEgKDIuNi4zMi41OS0wLDcuMSkKYW5kIGZpeGVkIGl0IChzZWUgZmlyc3QgcGF0Y2gg
aWZvciAyLjYuMzIuNjAgYmVsb3cpLgpGb3IgdXMgdGhlIHBhdGNoIHdvcmtzIGZpbmUgKHRl
c3RlZCBvbiAyLjYuMzIuNTktMC43LjEpLgoKQUZBSUNTIGZyb20gdGhlIGNvZGUsIHRoZSBw
cm9ibGVtIHNlZW1zIHRvIHBlcnNpc3QgaW4gY3VycmVudAprZXJuZWwgdmVyc2lvbnMgYWxz
by4gVGh1cywgSSBhZGRlZCB0aGUgc2Vjb25kIHBhdGNoIGZvciAzLjcuOS4KQXMgdGhlIHNl
dHVwIHRvIHJlcHJvZHVjZSB0aGUgcHJvYmxlbSBpcyBxdWl0ZSBjb21wbGV4LCBJIGNvdWxk
bid0CnRlc3QgdGhlIHNlY29uZCBwYXRjaCB5ZXQuIFNvIGNvbnNpZGVyIHRoaXMgb25lIGFz
IGEgUkZDLgoKQmVzdCByZWdhcmRzLApCb2RvCgpQbGVhc2UgQ0MgbWUsIEknbSBub3Qgb24g
dGhlIGxpc3QuCgo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQpG
cm9tOiBCb2RvIFN0cm9lc3NlciA8YnN0cm9lc3NlckB0cy5mdWppdHN1LmNvbT4KRGF0ZTog
RnJpLCAwOCBGZWIgMjAxMwpTdWJqZWN0OiBbUEFUQ0hdIG5ldDogc3VucnBjOiBmaXggcmFj
ZXMgaW4gUlBDIGNhY2hlCgpXZSBmb3VuZCB0aGUgcHJvYmxlbSBhbmQgdGVzdGVkIHRoZSBw
YXRjaCBvbiBhIFNMRVMxMSBTUDEgMi42LjMyLjU5LTAuNy4xCgpUaGlzIHBhdGNoIGFwcGxp
ZXMgdG8gbGludXgtMi42LjMyLjYwIChjaGFuZ2VkIG1vbm90b25pY19zZWNvbmRzIC0tPgpn
ZXRfc2Vjb25kcygpKQoKU3BvcmFkaWNhbGx5IE5GUzMgUlBDIHJlcXVlc3RzIHRvIHRoZSBu
ZnMgc2VydmVyIGFyZSBkcm9wcGVkIGR1ZSB0bwpjYWNoZV9jaGVjaygpIChuZXQvc3VucnBj
L2NhY2hlLmMpIHJldHVybmluZyAtRVRJTUVET1VUIGZvciBhbiBlbnRyeQpvZiB0aGUgImF1
dGgudW5peC5naWQiIGNhY2hlLgpJbiB0aGlzIGNhc2UsIG5vIE5GUyByZXBseSBpcyBzZW50
IHRvIHRoZSBjbGllbnQuCgpUaGUgcmVhc29uIGZvciB0aGUgZHJvcHBlZCByZXF1ZXN0cyBh
cmUgcmFjZXMgaW4gY2FjaGVfY2hlY2soKSB3aGVuCmNhY2hlX21ha2VfdXBjYWxsKCkgcmV0
dXJucyAtRUlOVkFMIChiZWNhdXNlIGl0IGlzIGNhbGxlZCBmb3IgYSBjYWNoZQp3aXRob3V0
IHJlYWRlcnMpIGFuZCBjYWNoZV9jaGVjaygpIHRoZXJlZm9yZSByZWZyZXNoZXMgdGhlIGNh
Y2hlIGVudHJ5IAoocnYgPT0gLUVBR0FJTikuCgpUaGVyZSBhcmUgdGhyZWUgZGV0YWlscyB0
aGF0IG5lZWQgdG8gYmUgY2hhbmdlZDoKIDEpIGNhY2hlX3JldmlzaXRfcmVxdWVzdCgpIG11
c3Qgbm90IGJlIGNhbGxlZCBiZWZvcmUgY2FjaGVfZnJlc2hfbG9ja2VkKCkKICAgIGhhcyB1
cGRhdGVkIHRoZSBjYWNoZSBlbnRyeSwgYXMgY2FjaGVfcmV2aXNpdF9yZXF1ZXN0KCkgd2Fr
ZXMgdXAKICAgIHRocmVhZHMgd2FpdGluZyBmb3IgdGhlIGNhY2hlIGVudHJ5IHRvIGJlIHVw
ZGF0ZWQuCiAgICBUaGUgZXhwbGljaXQgY2FsbCB0byBjYWNoZV9yZXZpc2l0X3JlcXVlc3Qo
KSBpcyBub3QgbmVlZGVkLCBhcwogICAgY2FjaGVfZnJlc2hfdW5sb2NrZWQoKSBjYWxscyBp
dCBhbnl3YXkuCiAgICAoQnV0IGluIGNhc2Ugb2Ygbm90IHVwZGF0aW5nIHRoZSBjYWNoZSBl
bnRyeSwgY2FjaGVfcmV2aXNpdF9yZXF1ZXN0KCkKICAgIG11c3QgYmUgY2FsbGVkLiBUaHVz
LCB3ZSB1c2UgYSBmYWxsIHRocm91Z2ggaW4gdGhlICJjYXNlIikuCiAyKSBDQUNIRV9QRU5E
SU5HIG11c3Qgbm90IGJlIGNsZWFyZWQgYmVmb3JlIGNhY2hlX2ZyZXNoX2xvY2tlZCgpIGhh
cwogICAgdXBkYXRlZCB0aGUgY2FjaGUgZW50cnksIGFzIGNhY2hlX2RlZmVyX3JlcSgpIGNh
biByZXR1cm4gd2l0aG91dCByZWFsbHkKICAgIHNsZWVwaW5nIGlmIGl0IGRldGVjdHMgQ0FD
SEVfUEVORElORyB1bnNldC4KICAgIChJbiBjYXNlIG9mIG5vdCB1cGRhdGluZyB0aGUgY2Fj
aGUgZW50cnkgYWdhaW4gd2UgdXNlIHRoZSBmYWxsIHRocm91Z2gpCiAzKSBJbWFnaW5lIGEg
dGhyZWFkIHRoYXQgY2FsbHMgY2FjaGVfY2hlY2soKSBhbmQgZ2V0cyBydiA9IC1FTk9FTlQg
ZnJvbQogICAgY2FjaGVfaXNfdmFsaWQoKS4gVGhlbiBpdCBzZXRzIENBQ0hFX1BFTkRJTkdz
IGFuZCBjYWxscwogICAgY2FjaGVfbWFrZV91cGNhbGwoKS4KICAgIFdlIGFzc3VtZSB0aGF0
IG1lYW53aGlsZSBnZXRfc2Vjb25kcygpIGFkdmFuY2VzIHRvIHRoZSBuZXh0CiAgICBzZWMu
IGFuZCBhIHNlY29uZCB0aHJlYWQgYWxzbyBjYWxscyBjYWNoZV9jaGVjaygpLiBJdCBnZXRz
IC1FQUdBSU4gZnJvbQogICAgY2FjaGVfaXNfdmFsaWQoKSBmb3IgdGhlIHNhbWUgY2FjaGUg
ZW50cnkuIEFzIENBQ0hFX1BFTkRJTkcgc3RpbGwgaXMKICAgIHNldCwgaXQgY2FsbHMgY2Fj
aGVfZGVmZXJfcmVxKCkgaW1tZWRpYXRlbHkgYW5kIHdhaXRzIGZvciBhIHdha2V1cCBmcm9t
CiAgICB0aGUgZmlyc3QgdGhyZWFkLgogICAgQWZ0ZXIgY2FjaGVfbWFrZV91cGNhbGwoKSBy
ZXR1cm5lZCAtRUlOVkFMLCB0aGUgZmlyc3QgdGhyZWFkIGRvZXMgbm90CiAgICB1cGRhdGUg
dGhlIGNhY2hlIGVudHJ5IGFzIGl0IGhhZCBnb3QgcnYgPSAtRU5PRU5ULCBidXQgd2FrZXMg
dXAgdGhlCiAgICBzZWNvbmQgdGhyZWFkIGJ5IGNhbGxpbmcgY2FjaGVfcmV2aXNpdF9yZXF1
ZXN0KCkuCiAgICBUaHJlYWQgdHdvIHdha2VzIHVwLCBjYWxscyBjYWNoZV9pc192YWxpZCgp
IGFuZCBhZ2FpbiBnZXRzIC1FQUdBSU4uCiAgICBUaHVzLCB0aGUgcmVzdWx0IG9mIHRoZSBz
ZWNvbmQgY2FjaGVfY2hlY2soKSBpcyAtRVRJTUVET1VUIGFuZCB0aGUKICAgIE5GUyByZXF1
ZXN0IGlzIGRyb3BwZWQuCiAgICBUbyBzb2x2ZSB0aGlzLCB0aGUgY2FjaGUgZW50cnkgbm93
IGlzIHVwZGF0ZWQgbm90IG9ubHkgaWYgcnYgPT0gLUVBR0FJTgogICAgYnV0IGlmIHJ2ID09
IC1FTk9FTlQgYWxzby4gVGhpcyBpcyBhIHN1ZmZpY2llbnQgd29ya2Fyb3VuZCwgYXMgdGhl
CiAgICBmaXJzdCB0aHJlYWQgd291bGQgaGF2ZSB0byBzdGF5IGluIGNhY2hlX2NoZWNrKCkg
YmV0d2VlbiBpdHMgY2FsbCB0bwogICAgY2FjaGVfaXNfdmFsaWQoKSBhbmQgY2xlYXJpbmcg
Q0FDSEVfUEVORElORyBmb3IgbW9yZSB0aGFuIDYwIHNlY29uZHMKICAgIHRvIGJyZWFrIHRo
ZSB3b3JrYXJvdW5kLgogICAgClNpZ25lZC1vZmYtYnk6IEJvZG8gU3Ryb2Vzc2VyIDxic3Ry
b2Vzc2VyQHRzLmZ1aml0c3UuY29tPgotLS0KCi0tLSBhL25ldC9zdW5ycGMvY2FjaGUuYwky
MDEyLTA4LTA4IDIxOjM1OjA5LjAwMDAwMDAwMCArMDIwMAorKysgYi9uZXQvc3VucnBjL2Nh
Y2hlLmMJMjAxMy0wMi0wOCAxNDoyOTo0MS4wMDAwMDAwMDAgKzAxMDAKQEAgLTIzMywxNSAr
MjMzLDE0IEBAIGludCBjYWNoZV9jaGVjayhzdHJ1Y3QgY2FjaGVfZGV0YWlsICpkZXQKIAkJ
aWYgKCF0ZXN0X2FuZF9zZXRfYml0KENBQ0hFX1BFTkRJTkcsICZoLT5mbGFncykpIHsKIAkJ
CXN3aXRjaCAoY2FjaGVfbWFrZV91cGNhbGwoZGV0YWlsLCBoKSkgewogCQkJY2FzZSAtRUlO
VkFMOgotCQkJCWNsZWFyX2JpdChDQUNIRV9QRU5ESU5HLCAmaC0+ZmxhZ3MpOwotCQkJCWNh
Y2hlX3JldmlzaXRfcmVxdWVzdChoKTsKLQkJCQlpZiAocnYgPT0gLUVBR0FJTikgeworCQkJ
CWlmIChydiA9PSAtRUFHQUlOIHx8IHJ2ID09IC1FTk9FTlQpIHsKIAkJCQkJc2V0X2JpdChD
QUNIRV9ORUdBVElWRSwgJmgtPmZsYWdzKTsKIAkJCQkJY2FjaGVfZnJlc2hfbG9ja2VkKGgs
IGdldF9zZWNvbmRzKCkrQ0FDSEVfTkVXX0VYUElSWSk7CisJCQkJCWNsZWFyX2JpdChDQUNI
RV9QRU5ESU5HLCAmaC0+ZmxhZ3MpOwogCQkJCQljYWNoZV9mcmVzaF91bmxvY2tlZChoLCBk
ZXRhaWwpOwogCQkJCQlydiA9IC1FTk9FTlQ7CisJCQkJCWJyZWFrOwogCQkJCX0KLQkJCQli
cmVhazsKIAogCQkJY2FzZSAtRUFHQUlOOgogCQkJCWNsZWFyX2JpdChDQUNIRV9QRU5ESU5H
LCAmaC0+ZmxhZ3MpOwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PQpGcm9tOiBCb2RvIFN0cm9lc3NlciA8YnN0cm9lc3NlckB0cy5mdWppdHN1LmNvbT4KRGF0
ZTogRnJpLCAwOCBGZWIgMjAxMwpTdWJqZWN0OiBbUEFUQ0hdIG5ldDogc3VucnBjOiBmaXgg
cmFjZXMgaW4gUlBDIGNhY2hlCgpUaGlzIHBhdGNoIGFwcGxpZXMgdG8gU0xFUyAxMSBTUDIg
bGludXgtMy4wLjUxLTAuNy45IGFuZCBhbHNvIHRvCnZhbmlsbGEgbGludXgtMy43LjkKCkl0
IGlzIHVudGVzdGVkIGFuZCBpcyBvbmx5IGJhc2VkIG9uIGEgY29kZSByZXZpZXcgYWZ0ZXIg
d2UKYW5hbHl6ZWQgdGhlIHJlYXNvbiBmb3IgTkZTIHJlcXVlc3RzIGJlaW5nIGRyb3BwZWQg
b24gYQpTTEVTMTEgU1AxIChsaW51eC0yLjYuMzIuNTktMC43LjEpCgpTcG9yYWRpY2FsbHkg
TkZTMyBSUEMgcmVxdWVzdHMgdG8gdGhlIG5mcyBzZXJ2ZXIgYXJlIGRyb3BwZWQgZHVlIHRv
CmNhY2hlX2NoZWNrKCkgKG5ldC9zdW5ycGMvY2FjaGUuYykgcmV0dXJuaW5nIC1FVElNRURP
VVQgZm9yIGFuIGVudHJ5Cm9mIHRoZSAiYXV0aC51bml4LmdpZCIgY2FjaGUuCkluIHRoaXMg
Y2FzZSwgbm8gTkZTIHJlcGx5IGlzIHNlbnQgdG8gdGhlIGNsaWVudC4KClRoZSByZWFzb24g
Zm9yIHRoZSBkcm9wcGVkIHJlcXVlc3RzIGFyZSByYWNlcyBpbiBjYWNoZV9jaGVjaygpIHdo
ZW4KY2FjaGVfbWFrZV91cGNhbGwoKSByZXR1cm5zIC1FSU5WQUwgKGJlY2F1c2UgaXQgaXMg
Y2FsbGVkIGZvciBhIGNhY2hlCndpdGhvdXQgcmVhZGVycykgYW5kIGNhY2hlX2NoZWNrKCkg
dGhlcmVmb3JlIHJlZnJlc2hlcyB0aGUgY2FjaGUgZW50cnkgCihydiA9PSAtRUFHQUlOKS4K
ClRoZXJlIGFyZSB0d28gZGV0YWlscyB0aGF0IG5lZWQgdG8gYmUgY2hhbmdlZDoKIDEpIGNh
Y2hlX3JldmlzaXRfcmVxdWVzdCgpIG11c3Qgbm90IGJlIGNhbGxlZCBiZWZvcmUgY2FjaGVf
ZnJlc2hfbG9ja2VkKCkKICAgIGhhcyB1cGRhdGVkIHRoZSBjYWNoZSBlbnRyeSwgYXMgY2Fj
aGVfcmV2aXNpdF9yZXF1ZXN0KCkgd2FrZXMgdXAKICAgIHRocmVhZHMgd2FpdGluZyBmb3Ig
dGhlIGNhY2hlIGVudHJ5IHRvIGJlIHVwZGF0ZWQuCiAgICBUaGUgZXhwbGljaXQgY2FsbCB0
byBjYWNoZV9yZXZpc2l0X3JlcXVlc3QoKSBpcyBub3QgbmVlZGVkLCBhcwogICAgY2FjaGVf
ZnJlc2hfdW5sb2NrZWQoKSBjYWxscyBpdCBhbnl3YXkuCiAgICAoQnV0IGluIGNhc2Ugb2Yg
bm90IHVwZGF0aW5nIHRoZSBjYWNoZSBlbnRyeSwgY2FjaGVfcmV2aXNpdF9yZXF1ZXN0KCkK
ICAgIG11c3QgYmUgY2FsbGVkKS4KIDIpIENBQ0hFX1BFTkRJTkcgbXVzdCBub3QgYmUgY2xl
YXJlZCBiZWZvcmUgY2FjaGVfZnJlc2hfbG9ja2VkKCkgaGFzCiAgICB1cGRhdGVkIHRoZSBj
YWNoZSBlbnRyeSwgYXMgY2FjaGVfd2FpdF9yZXEoKSBjYWxsZWQgYnkKICAgIGNhY2hlX2Rl
ZmVyX3JlcSgpIGNhbiByZXR1cm4gd2l0aG91dCByZWFsbHkgc2xlZXBpbmcgaWYgaXQgZGV0
ZWN0cwogICAgQ0FDSEVfUEVORElORyB1bnNldC4KClNpZ25lZC1vZmYtYnk6IEJvZG8gU3Ry
b2Vzc2VyIDxic3Ryb2Vzc2VyQHRzLmZ1aml0c3UuY29tPgotLS0KCi0tLSBhL25ldC9zdW5y
cGMvY2FjaGUuYwkyMDEzLTAyLTA4IDE1OjU2OjA3LjAwMDAwMDAwMCArMDEwMAorKysgYi9u
ZXQvc3VucnBjL2NhY2hlLmMJMjAxMy0wMi0wOCAxNjowNDozMi4wMDAwMDAwMDAgKzAxMDAK
QEAgLTIzMCwxMSArMjMwLDE0IEBAIHN0YXRpYyBpbnQgdHJ5X3RvX25lZ2F0ZV9lbnRyeShz
dHJ1Y3QgY2EKIAlydiA9IGNhY2hlX2lzX3ZhbGlkKGRldGFpbCwgaCk7CiAJaWYgKHJ2ICE9
IC1FQUdBSU4pIHsKIAkJd3JpdGVfdW5sb2NrKCZkZXRhaWwtPmhhc2hfbG9jayk7CisJCWNs
ZWFyX2JpdChDQUNIRV9QRU5ESU5HLCAmaC0+ZmxhZ3MpOworCQljYWNoZV9yZXZpc2l0X3Jl
cXVlc3QoaCk7CiAJCXJldHVybiBydjsKIAl9CiAJc2V0X2JpdChDQUNIRV9ORUdBVElWRSwg
JmgtPmZsYWdzKTsKIAljYWNoZV9mcmVzaF9sb2NrZWQoaCwgc2Vjb25kc19zaW5jZV9ib290
KCkrQ0FDSEVfTkVXX0VYUElSWSk7CiAJd3JpdGVfdW5sb2NrKCZkZXRhaWwtPmhhc2hfbG9j
ayk7CisJY2xlYXJfYml0KENBQ0hFX1BFTkRJTkcsICZoLT5mbGFncyk7CiAJY2FjaGVfZnJl
c2hfdW5sb2NrZWQoaCwgZGV0YWlsKTsKIAlyZXR1cm4gLUVOT0VOVDsKIH0KQEAgLTI3NSw4
ICsyNzgsNiBAQCBpbnQgY2FjaGVfY2hlY2soc3RydWN0IGNhY2hlX2RldGFpbCAqZGV0CiAJ
CWlmICghdGVzdF9hbmRfc2V0X2JpdChDQUNIRV9QRU5ESU5HLCAmaC0+ZmxhZ3MpKSB7CiAJ
CQlzd2l0Y2ggKGNhY2hlX21ha2VfdXBjYWxsKGRldGFpbCwgaCkpIHsKIAkJCWNhc2UgLUVJ
TlZBTDoKLQkJCQljbGVhcl9iaXQoQ0FDSEVfUEVORElORywgJmgtPmZsYWdzKTsKLQkJCQlj
YWNoZV9yZXZpc2l0X3JlcXVlc3QoaCk7CiAJCQkJcnYgPSB0cnlfdG9fbmVnYXRlX2VudHJ5
KGRldGFpbCwgaCk7CiAJCQkJYnJlYWs7CiAJCQljYXNlIC1FQUdBSU46Cg==

Re: [PATCH] sunrpc.ko: RPC cache fix races

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 5027 bytes --]

On 25 Feb 2013 20:55:50 +0100 Bodo Stroesser <bstroesser@ts.fujitsu.com>
wrote:

> On 24 Feb 2013 23:53:00 +0100 neilb@suse.de wrote:
> 
> > On 21 Feb 2013 16:44:41 +0100 Bodo Stroesser <bstroesser@ts.fujitsu.com>
> > wrote:
> > 
> > > On 21 Feb 2013 00:55:00 +0100 neilb@suse.de wrote:
> > > 
> > > > On 20 Feb 2013 14:57:07 +0100 bstroesser@ts.fujitsu.com wrote:
> > > > 
> > > > > On 20 Feb 2013 04:09:00 +0100 neilb@suse.de wrote:
> > > 
> > > snip
> > > 
> > > > > > Maybe:
> > > > > > 
> > > > > >   switch(cache_make_upcall(detail, h)) {
> > > > > >   case -EINVAL:
> > > > > >         if (rv) {
> > > > > > 		set_bit(CACHE_NEGATIVE, &h->flags);
> > > > > > 		cache_fresh_locked(h, get_seconds() + CACHE_NEW_EXPIRY);
> > > > > > 		rv = -ENOENT;
> > > > > > 	}
> > > > > > 	/* FALLTHROUGH */
> > > > > >   case -EAGAIN:
> > > > > > 	cache_fresh_unlocked(h, detail);
> > > > > >   }
> > > > > 
> > > > > I agree, your patch is obviously better than the mine.
> > > > > But let me suggest one little change: I would like to substitute
> > > > > cache_fresh_unlocked() by clear_bit() and cache_revisit_request(),
> > > > > as the call to cache_dequeue() in cache_fresh_unlocked() seems to
> > > > > be obsolete here:
> > > > 
> > > > It is exactly this sort of thinking (on my part) that got us into this mess
> > > > in the first place.  I reasoned that the full locking/testing/whatever wasn't
> > > > necessary and took a short cut.  It wasn't a good idea.
> > > 
> > > Maybe I'm totally wrong, but AFAICS, calling cache_dequeue() here in extreme
> > > situations (two threads in parallel calling check_cache() while a first reader
> > > is going to open cache access) could again cause a race (?)
> > 
> > Can you explain the race you see?
> 
> O.k., let me try ...
> 
> Suppposed there are 2 threads calling cache_check concurrently and a
> user process that is going to become a reader for the involved cache.
> 
> The first thread calls cache_is_valid() and gets -EAGAIN.
> Next, it sets CACHE_PENDING and calls cache_make_upcall(), which
> returns -EINVAL, as there is no reader yet.
> 
> Meanwhile the second thread also calls cache_is_valid(), also gets
> -EAGAIN, but now is interrupted and delayed for a while.
> 
> The first thread continues its work, negates the entry and calls
> cache_fresh_locked() followed by cache_fresh_unlocked().
> In cache_fresh_unlocked it resets CACHE_PENDING and calls
> cache_revisit_request(). While this, it is interrupted and delayed.
> 
> The user process is scheduled and opens the channel to become a reader.
> 
> The second thread wakes up again and sets CACHE_PENDING. Next it calls
> cache_make_upcall(), which results in a request being queued, as there
> is a reader now.
> 
> The first thread wakes up and continues its work calling cache_dequeue().
> Maybe the request is dropped before the reader could process it.
> 
> Do you agree or did I miss something?

Yes, I see what you mean, thanks.

I would close that race with:
@@ -1022,6 +1016,9 @@ static void cache_dequeue(struct cache_detail *detail, struct cache_head *ch)
 			struct cache_request *cr = container_of(cq, struct cache_request, q);
 			if (cr->item != ch)
 				continue;
+			if (test_bit(CACHE_PENDING, &ch->flags))
+				/* Lost a race and it is pending again */
+				break;
 			if (cr->readers != 0)
 				continue;
 			list_del(&cr->q.list);

> 
> 
> > 
> > > 
> > > BTW: if there is a reader for a cache, is there any protection against many
> > > upcalls being queued for the same cache entry?
> > 
> > The CACHE_PENDING flag should provide that protection.  We only queue an
> > upcall after "test_and_set", and always dequeue after "clear_bit".
> 
> Sorry, my question wasn't very clear. CACHE_PENDING *does* provide the
> protection against parallel upcalls.
> 
> OTOH, as cache_is_valid() is called before test_and_set_bit(CACHE_PENDING)
> and then the upcall is made without again calling cache_is_valid(),
> I see a small chance of a second request being queued unnecessarily just
> after the first request was answered. 

A small chance, yes.  However  I don't think it is harmful, just unnecessary.
So I'm not sure it is worth fixing.

Thanks,
NeilBrown


> 
> Bodo
> 
> > 
> > NeilBrown
> > 
> > 
> > > 
> > > Bodo
> > > 
> > > > 
> > > > Given that this is obviously difficult code to get right, we should make it
> > > > as easy to review as possible.  Have "cache_fresh_unlocked" makes it more
> > > > obviously correct, and that is a good thing.
> > > > Maybe cache_dequeue isn't needed here, but it won't hurt so I'd much rather
> > > > have the clearer code.
> > > > In fact, I'd also like to change
> > > > 
> > > > 			if (test_and_clear_bit(CACHE_PENDING, &ch->flags))
> > > > 				cache_dequeue(current_detail, ch);
> > > > 			cache_revisit_request(ch);
> > > > 
> > > > near the end of cache_clean to call  cache_fresh_unlocked().
> > > > 


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] sunrpc.ko: RPC cache fix races

[Bodo Stroesser]

T24gMjQgRmViIDIwMTMgMjM6NTM6MDAgKzAxMDAgbmVpbGJAc3VzZS5kZSB3cm90ZToKCj4g
T24gMjEgRmViIDIwMTMgMTY6NDQ6NDEgKzAxMDAgQm9kbyBTdHJvZXNzZXIgPGJzdHJvZXNz
ZXJAdHMuZnVqaXRzdS5jb20+Cj4gd3JvdGU6Cj4gCj4gPiBPbiAyMSBGZWIgMjAxMyAwMDo1
NTowMCArMDEwMCBuZWlsYkBzdXNlLmRlIHdyb3RlOgo+ID4gCj4gPiA+IE9uIDIwIEZlYiAy
MDEzIDE0OjU3OjA3ICswMTAwIGJzdHJvZXNzZXJAdHMuZnVqaXRzdS5jb20gd3JvdGU6Cj4g
PiA+IAo+ID4gPiA+IE9uIDIwIEZlYiAyMDEzIDA0OjA5OjAwICswMTAwIG5laWxiQHN1c2Uu
ZGUgd3JvdGU6Cj4gPiAKPiA+IHNuaXAKPiA+IAo+ID4gPiA+ID4gTWF5YmU6Cj4gPiA+ID4g
PiAKPiA+ID4gPiA+ICAgc3dpdGNoKGNhY2hlX21ha2VfdXBjYWxsKGRldGFpbCwgaCkpIHsK
PiA+ID4gPiA+ICAgY2FzZSAtRUlOVkFMOgo+ID4gPiA+ID4gICAgICAgICBpZiAocnYpIHsK
PiA+ID4gPiA+IAkJc2V0X2JpdChDQUNIRV9ORUdBVElWRSwgJmgtPmZsYWdzKTsKPiA+ID4g
PiA+IAkJY2FjaGVfZnJlc2hfbG9ja2VkKGgsIGdldF9zZWNvbmRzKCkgKyBDQUNIRV9ORVdf
RVhQSVJZKTsKPiA+ID4gPiA+IAkJcnYgPSAtRU5PRU5UOwo+ID4gPiA+ID4gCX0KPiA+ID4g
PiA+IAkvKiBGQUxMVEhST1VHSCAqLwo+ID4gPiA+ID4gICBjYXNlIC1FQUdBSU46Cj4gPiA+
ID4gPiAJY2FjaGVfZnJlc2hfdW5sb2NrZWQoaCwgZGV0YWlsKTsKPiA+ID4gPiA+ICAgfQo+
ID4gPiA+IAo+ID4gPiA+IEkgYWdyZWUsIHlvdXIgcGF0Y2ggaXMgb2J2aW91c2x5IGJldHRl
ciB0aGFuIHRoZSBtaW5lLgo+ID4gPiA+IEJ1dCBsZXQgbWUgc3VnZ2VzdCBvbmUgbGl0dGxl
IGNoYW5nZTogSSB3b3VsZCBsaWtlIHRvIHN1YnN0aXR1dGUKPiA+ID4gPiBjYWNoZV9mcmVz
aF91bmxvY2tlZCgpIGJ5IGNsZWFyX2JpdCgpIGFuZCBjYWNoZV9yZXZpc2l0X3JlcXVlc3Qo
KSwKPiA+ID4gPiBhcyB0aGUgY2FsbCB0byBjYWNoZV9kZXF1ZXVlKCkgaW4gY2FjaGVfZnJl
c2hfdW5sb2NrZWQoKSBzZWVtcyB0bwo+ID4gPiA+IGJlIG9ic29sZXRlIGhlcmU6Cj4gPiA+
IAo+ID4gPiBJdCBpcyBleGFjdGx5IHRoaXMgc29ydCBvZiB0aGlua2luZyAob24gbXkgcGFy
dCkgdGhhdCBnb3QgdXMgaW50byB0aGlzIG1lc3MKPiA+ID4gaW4gdGhlIGZpcnN0IHBsYWNl
LiAgSSByZWFzb25lZCB0aGF0IHRoZSBmdWxsIGxvY2tpbmcvdGVzdGluZy93aGF0ZXZlciB3
YXNuJ3QKPiA+ID4gbmVjZXNzYXJ5IGFuZCB0b29rIGEgc2hvcnQgY3V0LiAgSXQgd2Fzbid0
IGEgZ29vZCBpZGVhLgo+ID4gCj4gPiBNYXliZSBJJ20gdG90YWxseSB3cm9uZywgYnV0IEFG
QUlDUywgY2FsbGluZyBjYWNoZV9kZXF1ZXVlKCkgaGVyZSBpbiBleHRyZW1lCj4gPiBzaXR1
YXRpb25zICh0d28gdGhyZWFkcyBpbiBwYXJhbGxlbCBjYWxsaW5nIGNoZWNrX2NhY2hlKCkg
d2hpbGUgYSBmaXJzdCByZWFkZXIKPiA+IGlzIGdvaW5nIHRvIG9wZW4gY2FjaGUgYWNjZXNz
KSBjb3VsZCBhZ2FpbiBjYXVzZSBhIHJhY2UgKD8pCj4gCj4gQ2FuIHlvdSBleHBsYWluIHRo
ZSByYWNlIHlvdSBzZWU/CgpPLmsuLCBsZXQgbWUgdHJ5IC4uLgoKU3VwcHBvc2VkIHRoZXJl
IGFyZSAyIHRocmVhZHMgY2FsbGluZyBjYWNoZV9jaGVjayBjb25jdXJyZW50bHkgYW5kIGEK
dXNlciBwcm9jZXNzIHRoYXQgaXMgZ29pbmcgdG8gYmVjb21lIGEgcmVhZGVyIGZvciB0aGUg
aW52b2x2ZWQgY2FjaGUuCgpUaGUgZmlyc3QgdGhyZWFkIGNhbGxzIGNhY2hlX2lzX3ZhbGlk
KCkgYW5kIGdldHMgLUVBR0FJTi4KTmV4dCwgaXQgc2V0cyBDQUNIRV9QRU5ESU5HIGFuZCBj
YWxscyBjYWNoZV9tYWtlX3VwY2FsbCgpLCB3aGljaApyZXR1cm5zIC1FSU5WQUwsIGFzIHRo
ZXJlIGlzIG5vIHJlYWRlciB5ZXQuCgpNZWFud2hpbGUgdGhlIHNlY29uZCB0aHJlYWQgYWxz
byBjYWxscyBjYWNoZV9pc192YWxpZCgpLCBhbHNvIGdldHMKLUVBR0FJTiwgYnV0IG5vdyBp
cyBpbnRlcnJ1cHRlZCBhbmQgZGVsYXllZCBmb3IgYSB3aGlsZS4KClRoZSBmaXJzdCB0aHJl
YWQgY29udGludWVzIGl0cyB3b3JrLCBuZWdhdGVzIHRoZSBlbnRyeSBhbmQgY2FsbHMKY2Fj
aGVfZnJlc2hfbG9ja2VkKCkgZm9sbG93ZWQgYnkgY2FjaGVfZnJlc2hfdW5sb2NrZWQoKS4K
SW4gY2FjaGVfZnJlc2hfdW5sb2NrZWQgaXQgcmVzZXRzIENBQ0hFX1BFTkRJTkcgYW5kIGNh
bGxzCmNhY2hlX3JldmlzaXRfcmVxdWVzdCgpLiBXaGlsZSB0aGlzLCBpdCBpcyBpbnRlcnJ1
cHRlZCBhbmQgZGVsYXllZC4KClRoZSB1c2VyIHByb2Nlc3MgaXMgc2NoZWR1bGVkIGFuZCBv
cGVucyB0aGUgY2hhbm5lbCB0byBiZWNvbWUgYSByZWFkZXIuCgpUaGUgc2Vjb25kIHRocmVh
ZCB3YWtlcyB1cCBhZ2FpbiBhbmQgc2V0cyBDQUNIRV9QRU5ESU5HLiBOZXh0IGl0IGNhbGxz
CmNhY2hlX21ha2VfdXBjYWxsKCksIHdoaWNoIHJlc3VsdHMgaW4gYSByZXF1ZXN0IGJlaW5n
IHF1ZXVlZCwgYXMgdGhlcmUKaXMgYSByZWFkZXIgbm93LgoKVGhlIGZpcnN0IHRocmVhZCB3
YWtlcyB1cCBhbmQgY29udGludWVzIGl0cyB3b3JrIGNhbGxpbmcgY2FjaGVfZGVxdWV1ZSgp
LgpNYXliZSB0aGUgcmVxdWVzdCBpcyBkcm9wcGVkIGJlZm9yZSB0aGUgcmVhZGVyIGNvdWxk
IHByb2Nlc3MgaXQuCgpEbyB5b3UgYWdyZWUgb3IgZGlkIEkgbWlzcyBzb21ldGhpbmc/CgoK
PiAKPiA+IAo+ID4gQlRXOiBpZiB0aGVyZSBpcyBhIHJlYWRlciBmb3IgYSBjYWNoZSwgaXMg
dGhlcmUgYW55IHByb3RlY3Rpb24gYWdhaW5zdCBtYW55Cj4gPiB1cGNhbGxzIGJlaW5nIHF1
ZXVlZCBmb3IgdGhlIHNhbWUgY2FjaGUgZW50cnk/Cj4gCj4gVGhlIENBQ0hFX1BFTkRJTkcg
ZmxhZyBzaG91bGQgcHJvdmlkZSB0aGF0IHByb3RlY3Rpb24uICBXZSBvbmx5IHF1ZXVlIGFu
Cj4gdXBjYWxsIGFmdGVyICJ0ZXN0X2FuZF9zZXQiLCBhbmQgYWx3YXlzIGRlcXVldWUgYWZ0
ZXIgImNsZWFyX2JpdCIuCgpTb3JyeSwgbXkgcXVlc3Rpb24gd2Fzbid0IHZlcnkgY2xlYXIu
IENBQ0hFX1BFTkRJTkcgKmRvZXMqIHByb3ZpZGUgdGhlCnByb3RlY3Rpb24gYWdhaW5zdCBw
YXJhbGxlbCB1cGNhbGxzLgoKT1RPSCwgYXMgY2FjaGVfaXNfdmFsaWQoKSBpcyBjYWxsZWQg
YmVmb3JlIHRlc3RfYW5kX3NldF9iaXQoQ0FDSEVfUEVORElORykKYW5kIHRoZW4gdGhlIHVw
Y2FsbCBpcyBtYWRlIHdpdGhvdXQgYWdhaW4gY2FsbGluZyBjYWNoZV9pc192YWxpZCgpLApJ
IHNlZSBhIHNtYWxsIGNoYW5jZSBvZiBhIHNlY29uZCByZXF1ZXN0IGJlaW5nIHF1ZXVlZCB1
bm5lY2Vzc2FyaWx5IGp1c3QKYWZ0ZXIgdGhlIGZpcnN0IHJlcXVlc3Qgd2FzIGFuc3dlcmVk
LiAKCkJvZG8KCj4gCj4gTmVpbEJyb3duCj4gCj4gCj4gPiAKPiA+IEJvZG8KPiA+IAo+ID4g
PiAKPiA+ID4gR2l2ZW4gdGhhdCB0aGlzIGlzIG9idmlvdXNseSBkaWZmaWN1bHQgY29kZSB0
byBnZXQgcmlnaHQsIHdlIHNob3VsZCBtYWtlIGl0Cj4gPiA+IGFzIGVhc3kgdG8gcmV2aWV3
IGFzIHBvc3NpYmxlLiAgSGF2ZSAiY2FjaGVfZnJlc2hfdW5sb2NrZWQiIG1ha2VzIGl0IG1v
cmUKPiA+ID4gb2J2aW91c2x5IGNvcnJlY3QsIGFuZCB0aGF0IGlzIGEgZ29vZCB0aGluZy4K
PiA+ID4gTWF5YmUgY2FjaGVfZGVxdWV1ZSBpc24ndCBuZWVkZWQgaGVyZSwgYnV0IGl0IHdv
bid0IGh1cnQgc28gSSdkIG11Y2ggcmF0aGVyCj4gPiA+IGhhdmUgdGhlIGNsZWFyZXIgY29k
ZS4KPiA+ID4gSW4gZmFjdCwgSSdkIGFsc28gbGlrZSB0byBjaGFuZ2UKPiA+ID4gCj4gPiA+
IAkJCWlmICh0ZXN0X2FuZF9jbGVhcl9iaXQoQ0FDSEVfUEVORElORywgJmNoLT5mbGFncykp
Cj4gPiA+IAkJCQljYWNoZV9kZXF1ZXVlKGN1cnJlbnRfZGV0YWlsLCBjaCk7Cj4gPiA+IAkJ
CWNhY2hlX3JldmlzaXRfcmVxdWVzdChjaCk7Cj4gPiA+IAo+ID4gPiBuZWFyIHRoZSBlbmQg
b2YgY2FjaGVfY2xlYW4gdG8gY2FsbCAgY2FjaGVfZnJlc2hfdW5sb2NrZWQoKS4KPiA+ID4g
Cg==

Re: [PATCH] sunrpc.ko: RPC cache fix races

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2389 bytes --]

On 21 Feb 2013 16:44:41 +0100 Bodo Stroesser <bstroesser@ts.fujitsu.com>
wrote:

> On 21 Feb 2013 00:55:00 +0100 neilb@suse.de wrote:
> 
> > On 20 Feb 2013 14:57:07 +0100 bstroesser@ts.fujitsu.com wrote:
> > 
> > > On 20 Feb 2013 04:09:00 +0100 neilb@suse.de wrote:
> 
> snip
> 
> > > > Maybe:
> > > > 
> > > >   switch(cache_make_upcall(detail, h)) {
> > > >   case -EINVAL:
> > > >         if (rv) {
> > > > 		set_bit(CACHE_NEGATIVE, &h->flags);
> > > > 		cache_fresh_locked(h, get_seconds() + CACHE_NEW_EXPIRY);
> > > > 		rv = -ENOENT;
> > > > 	}
> > > > 	/* FALLTHROUGH */
> > > >   case -EAGAIN:
> > > > 	cache_fresh_unlocked(h, detail);
> > > >   }
> > > 
> > > I agree, your patch is obviously better than the mine.
> > > But let me suggest one little change: I would like to substitute
> > > cache_fresh_unlocked() by clear_bit() and cache_revisit_request(),
> > > as the call to cache_dequeue() in cache_fresh_unlocked() seems to
> > > be obsolete here:
> > 
> > It is exactly this sort of thinking (on my part) that got us into this mess
> > in the first place.  I reasoned that the full locking/testing/whatever wasn't
> > necessary and took a short cut.  It wasn't a good idea.
> 
> Maybe I'm totally wrong, but AFAICS, calling cache_dequeue() here in extreme
> situations (two threads in parallel calling check_cache() while a first reader
> is going to open cache access) could again cause a race (?)

Can you explain the race you see?

> 
> BTW: if there is a reader for a cache, is there any protection against many
> upcalls being queued for the same cache entry?

The CACHE_PENDING flag should provide that protection.  We only queue an
upcall after "test_and_set", and always dequeue after "clear_bit".

NeilBrown


> 
> Bodo
> 
> > 
> > Given that this is obviously difficult code to get right, we should make it
> > as easy to review as possible.  Have "cache_fresh_unlocked" makes it more
> > obviously correct, and that is a good thing.
> > Maybe cache_dequeue isn't needed here, but it won't hurt so I'd much rather
> > have the clearer code.
> > In fact, I'd also like to change
> > 
> > 			if (test_and_clear_bit(CACHE_PENDING, &ch->flags))
> > 				cache_dequeue(current_detail, ch);
> > 			cache_revisit_request(ch);
> > 
> > near the end of cache_clean to call  cache_fresh_unlocked().
> > 


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] sunrpc.ko: RPC cache fix races

[Bodo Stroesser]

T24gMjEgRmViIDIwMTMgMDA6NTU6MDAgKzAxMDAgbmVpbGJAc3VzZS5kZSB3cm90ZToKCj4g
T24gMjAgRmViIDIwMTMgMTQ6NTc6MDcgKzAxMDAgYnN0cm9lc3NlckB0cy5mdWppdHN1LmNv
bSB3cm90ZToKPiAKPiA+IE9uIDIwIEZlYiAyMDEzIDA0OjA5OjAwICswMTAwIG5laWxiQHN1
c2UuZGUgd3JvdGU6CgpzbmlwCgo+ID4gPiBNYXliZToKPiA+ID4gCj4gPiA+ICAgc3dpdGNo
KGNhY2hlX21ha2VfdXBjYWxsKGRldGFpbCwgaCkpIHsKPiA+ID4gICBjYXNlIC1FSU5WQUw6
Cj4gPiA+ICAgICAgICAgaWYgKHJ2KSB7Cj4gPiA+IAkJc2V0X2JpdChDQUNIRV9ORUdBVElW
RSwgJmgtPmZsYWdzKTsKPiA+ID4gCQljYWNoZV9mcmVzaF9sb2NrZWQoaCwgZ2V0X3NlY29u
ZHMoKSArIENBQ0hFX05FV19FWFBJUlkpOwo+ID4gPiAJCXJ2ID0gLUVOT0VOVDsKPiA+ID4g
CX0KPiA+ID4gCS8qIEZBTExUSFJPVUdIICovCj4gPiA+ICAgY2FzZSAtRUFHQUlOOgo+ID4g
PiAJY2FjaGVfZnJlc2hfdW5sb2NrZWQoaCwgZGV0YWlsKTsKPiA+ID4gICB9Cj4gPiAKPiA+
IEkgYWdyZWUsIHlvdXIgcGF0Y2ggaXMgb2J2aW91c2x5IGJldHRlciB0aGFuIHRoZSBtaW5l
Lgo+ID4gQnV0IGxldCBtZSBzdWdnZXN0IG9uZSBsaXR0bGUgY2hhbmdlOiBJIHdvdWxkIGxp
a2UgdG8gc3Vic3RpdHV0ZQo+ID4gY2FjaGVfZnJlc2hfdW5sb2NrZWQoKSBieSBjbGVhcl9i
aXQoKSBhbmQgY2FjaGVfcmV2aXNpdF9yZXF1ZXN0KCksCj4gPiBhcyB0aGUgY2FsbCB0byBj
YWNoZV9kZXF1ZXVlKCkgaW4gY2FjaGVfZnJlc2hfdW5sb2NrZWQoKSBzZWVtcyB0bwo+ID4g
YmUgb2Jzb2xldGUgaGVyZToKPiAKPiBJdCBpcyBleGFjdGx5IHRoaXMgc29ydCBvZiB0aGlu
a2luZyAob24gbXkgcGFydCkgdGhhdCBnb3QgdXMgaW50byB0aGlzIG1lc3MKPiBpbiB0aGUg
Zmlyc3QgcGxhY2UuICBJIHJlYXNvbmVkIHRoYXQgdGhlIGZ1bGwgbG9ja2luZy90ZXN0aW5n
L3doYXRldmVyIHdhc24ndAo+IG5lY2Vzc2FyeSBhbmQgdG9vayBhIHNob3J0IGN1dC4gIEl0
IHdhc24ndCBhIGdvb2QgaWRlYS4KCk1heWJlIEknbSB0b3RhbGx5IHdyb25nLCBidXQgQUZB
SUNTLCBjYWxsaW5nIGNhY2hlX2RlcXVldWUoKSBoZXJlIGluIGV4dHJlbWUKc2l0dWF0aW9u
cyAodHdvIHRocmVhZHMgaW4gcGFyYWxsZWwgY2FsbGluZyBjaGVja19jYWNoZSgpIHdoaWxl
IGEgZmlyc3QgcmVhZGVyCmlzIGdvaW5nIHRvIG9wZW4gY2FjaGUgYWNjZXNzKSBjb3VsZCBh
Z2FpbiBjYXVzZSBhIHJhY2UgKD8pCgpCVFc6IGlmIHRoZXJlIGlzIGEgcmVhZGVyIGZvciBh
IGNhY2hlLCBpcyB0aGVyZSBhbnkgcHJvdGVjdGlvbiBhZ2FpbnN0IG1hbnkKdXBjYWxscyBi
ZWluZyBxdWV1ZWQgZm9yIHRoZSBzYW1lIGNhY2hlIGVudHJ5PwoKQm9kbwoKPiAKPiBHaXZl
biB0aGF0IHRoaXMgaXMgb2J2aW91c2x5IGRpZmZpY3VsdCBjb2RlIHRvIGdldCByaWdodCwg
d2Ugc2hvdWxkIG1ha2UgaXQKPiBhcyBlYXN5IHRvIHJldmlldyBhcyBwb3NzaWJsZS4gIEhh
dmUgImNhY2hlX2ZyZXNoX3VubG9ja2VkIiBtYWtlcyBpdCBtb3JlCj4gb2J2aW91c2x5IGNv
cnJlY3QsIGFuZCB0aGF0IGlzIGEgZ29vZCB0aGluZy4KPiBNYXliZSBjYWNoZV9kZXF1ZXVl
IGlzbid0IG5lZWRlZCBoZXJlLCBidXQgaXQgd29uJ3QgaHVydCBzbyBJJ2QgbXVjaCByYXRo
ZXIKPiBoYXZlIHRoZSBjbGVhcmVyIGNvZGUuCj4gSW4gZmFjdCwgSSdkIGFsc28gbGlrZSB0
byBjaGFuZ2UKPiAKPiAJCQlpZiAodGVzdF9hbmRfY2xlYXJfYml0KENBQ0hFX1BFTkRJTkcs
ICZjaC0+ZmxhZ3MpKQo+IAkJCQljYWNoZV9kZXF1ZXVlKGN1cnJlbnRfZGV0YWlsLCBjaCk7
Cj4gCQkJY2FjaGVfcmV2aXNpdF9yZXF1ZXN0KGNoKTsKPiAKPiBuZWFyIHRoZSBlbmQgb2Yg
Y2FjaGVfY2xlYW4gdG8gY2FsbCAgY2FjaGVfZnJlc2hfdW5sb2NrZWQoKS4KPiAK

Re: [PATCH] sunrpc.ko: RPC cache fix races

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 7963 bytes --]

On 20 Feb 2013 14:57:07 +0100 bstroesser@ts.fujitsu.com wrote:

> On 20 Feb 2013 04:09:00 +0100 neilb@suse.de wrote:
> 
> > On 19 Feb 2013 18:08:40 +0100 bstroesser@ts.fujitsu.com wrote:
> > 
> > > Second attempt using the correct FROM. Sorry for the noise.
> > > 
> > > 
> > > Hi,
> > > 
> > > I found a problem in sunrpc.ko on a SLES11 SP1 (2.6.32.59-0,7.1) and 
> > > fixed it (see first patch ifor 2.6.32.60 below).
> > > For us the patch works fine (tested on 2.6.32.59-0.7.1).
> > > 
> > > AFAICS from the code, the problem seems to persist in current kernel 
> > > versions also. Thus, I added the second patch for 3.7.9.
> > > As the setup to reproduce the problem is quite complex, I couldn't 
> > > test the second patch yet. So consider this one as a RFC.
> > > 
> > > Best regards,
> > > Bodo
> > > 
> > > Please CC me, I'm not on the list.
> > > 
> > > =========================================
> > > From: Bodo Stroesser <bstroesser@ts.fujitsu.com>
> > > Date: Fri, 08 Feb 2013
> > > Subject: [PATCH] net: sunrpc: fix races in RPC cache
> > > 
> > > We found the problem and tested the patch on a SLES11 SP1 
> > > 2.6.32.59-0.7.1
> > > 
> > > This patch applies to linux-2.6.32.60 (changed monotonic_seconds -->
> > > get_seconds())
> > > 
> > > Sporadically NFS3 RPC requests to the nfs server are dropped due to
> > > cache_check() (net/sunrpc/cache.c) returning -ETIMEDOUT for an entry 
> > > of the "auth.unix.gid" cache.
> > > In this case, no NFS reply is sent to the client.
> > > 
> > > The reason for the dropped requests are races in cache_check() when
> > > cache_make_upcall() returns -EINVAL (because it is called for a cache 
> > > without readers) and cache_check() therefore refreshes the cache entry 
> > > (rv == -EAGAIN).
> > > 
> > > There are three details that need to be changed:
> > >  1) cache_revisit_request() must not be called before cache_fresh_locked()
> > >     has updated the cache entry, as cache_revisit_request() wakes up
> > >     threads waiting for the cache entry to be updated.
> > 
> > This certainly seems correct.  It is wrong to call cache_revisit_request() so early.
> > 
> > >     The explicit call to cache_revisit_request() is not needed, as
> > >     cache_fresh_unlocked() calls it anyway.
> > 
> > But cache_fresh_unlocked is only called if "rv == -EAGAIN", however we also need to call it in the case where "age > refresh_age/2" - it must always be called after clearing CACHE_PENDING.
> > 
> > Also, cache_fresh_unlocked() only calls cache_revisit_request() if CACHE_PENDING is set, but we have just cleared it!  Some definitely something wrong here.
> > (Note that I'm looking at the SLES 2.6.32 code at the moment, mainline is a bit different).
> > 
> > 
> > >     (But in case of not updating the cache entry, cache_revisit_request()
> > >     must be called. Thus, we use a fall through in the "case").
> > 
> > Hmm... I don't like case fallthroughs unless they have nice big comments:
> >     /* FALLTHROUGH */
> > or similar. :-)
> > 
> > >  2) CACHE_PENDING must not be cleared before cache_fresh_locked() has
> > >     updated the cache entry, as cache_defer_req() can return without really
> > >     sleeping if it detects CACHE_PENDING unset.
> > 
> > Agreed.  So we should leave the clearing of CACHE_PENDING to cache_fresh_unlocked().
> > 
> > 
> > >     (In case of not updating the cache entry again we use the fall 
> > > through)
> > >  3) Imagine a thread that calls cache_check() and gets rv = -ENOENT from
> > >     cache_is_valid(). Then it sets CACHE_PENDINGs and calls
> > >     cache_make_upcall().
> > >     We assume that meanwhile get_seconds() advances to the next
> > >     sec. and a second thread also calls cache_check(). It gets -EAGAIN from
> > >     cache_is_valid() for the same cache entry. As CACHE_PENDING still is
> > >     set, it calls cache_defer_req() immediately and waits for a wakeup from
> > >     the first thread.
> > >     After cache_make_upcall() returned -EINVAL, the first thread does not
> > >     update the cache entry as it had got rv = -ENOENT, but wakes up the
> > >     second thread by calling cache_revisit_request().
> > >     Thread two wakes up, calls cache_is_valid() and again gets -EAGAIN.
> > >     Thus, the result of the second cache_check() is -ETIMEDOUT and the
> > >     NFS request is dropped.
> > 
> > Yep, that's not so good....
> > 
> > 
> > >     To solve this, the cache entry now is updated not only if rv == -EAGAIN
> > >     but if rv == -ENOENT also. This is a sufficient workaround, as the
> > >     first thread would have to stay in cache_check() between its call to
> > >     cache_is_valid() and clearing CACHE_PENDING for more than 60 seconds
> > >     to break the workaround.
> > 
> > Still, it isn't nice to just have a work-around.  It would be best to have a fix.
> > The key problem here is that cache_is_valid() is time-sensitive.  This have been address in mainline - cache_is_valid doesn't depend on the current time there.
> 
> So, the solution would be a backport of the current mainline code ...

That would always be my preference, when it is practical.

> 
> Anyway, I think for SLES11 SP1 and 2.6.32.60 the work-around would be sufficient.
> BTW: it has the positive side effect, that - while a cache entry is in its second
> half of life - no longer each cache_check() tries to do a cache_make_upcall().

I agree that is an improvement.

> 
> 
> > 
> > 
> > >     
> > > Signed-off-by: Bodo Stroesser <bstroesser@ts.fujitsu.com>
> > > ---
> > > 
> > > --- a/net/sunrpc/cache.c	2012-08-08 21:35:09.000000000 +0200
> > > +++ b/net/sunrpc/cache.c	2013-02-08 14:29:41.000000000 +0100
> > > @@ -233,15 +233,14 @@ int cache_check(struct cache_detail *det
> > >  		if (!test_and_set_bit(CACHE_PENDING, &h->flags)) {
> > >  			switch (cache_make_upcall(detail, h)) {
> > >  			case -EINVAL:
> > > -				clear_bit(CACHE_PENDING, &h->flags);
> > > -				cache_revisit_request(h);
> > > -				if (rv == -EAGAIN) {
> > > +				if (rv == -EAGAIN || rv == -ENOENT) {
> > >  					set_bit(CACHE_NEGATIVE, &h->flags);
> > >  					cache_fresh_locked(h, get_seconds()+CACHE_NEW_EXPIRY);
> > > +					clear_bit(CACHE_PENDING, &h->flags);
> > >  					cache_fresh_unlocked(h, detail);
> > >  					rv = -ENOENT;
> > > +					break;
> > >  				}
> > > -				break;
> > >  
> > >  			case -EAGAIN:
> > >  				clear_bit(CACHE_PENDING, &h->flags);
> > 
> > I agree with some of this....
> > Maybe:
> > 
> >   switch(cache_make_upcall(detail, h)) {
> >   case -EINVAL:
> >         if (rv) {
> > 		set_bit(CACHE_NEGATIVE, &h->flags);
> > 		cache_fresh_locked(h, get_seconds() + CACHE_NEW_EXPIRY);
> > 		rv = -ENOENT;
> > 	}
> > 	/* FALLTHROUGH */
> >   case -EAGAIN:
> > 	cache_fresh_unlocked(h, detail);
> >   }
> 
> I agree, your patch is obviously better than the mine.
> But let me suggest one little change: I would like to substitute
> cache_fresh_unlocked() by clear_bit() and cache_revisit_request(),
> as the call to cache_dequeue() in cache_fresh_unlocked() seems to
> be obsolete here:

It is exactly this sort of thinking (on my part) that got us into this mess
in the first place.  I reasoned that the full locking/testing/whatever wasn't
necessary and took a short cut.  It wasn't a good idea.

Given that this is obviously difficult code to get right, we should make it
as easy to review as possible.  Have "cache_fresh_unlocked" makes it more
obviously correct, and that is a good thing.
Maybe cache_dequeue isn't needed here, but it won't hurt so I'd much rather
have the clearer code.
In fact, I'd also like to change

			if (test_and_clear_bit(CACHE_PENDING, &ch->flags))
				cache_dequeue(current_detail, ch);
			cache_revisit_request(ch);

near the end of cache_clean to call  cache_fresh_unlocked().


NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] sunrpc.ko: RPC cache fix races

[bstroesser]

T24gMjAgRmViIDIwMTMgMDQ6MDk6MDAgKzAxMDAgbmVpbGJAc3VzZS5kZSB3cm90ZToKCj4g
T24gMTkgRmViIDIwMTMgMTg6MDg6NDAgKzAxMDAgYnN0cm9lc3NlckB0cy5mdWppdHN1LmNv
bSB3cm90ZToKPiAKPiA+IFNlY29uZCBhdHRlbXB0IHVzaW5nIHRoZSBjb3JyZWN0IEZST00u
IFNvcnJ5IGZvciB0aGUgbm9pc2UuCj4gPiAKPiA+IAo+ID4gSGksCj4gPiAKPiA+IEkgZm91
bmQgYSBwcm9ibGVtIGluIHN1bnJwYy5rbyBvbiBhIFNMRVMxMSBTUDEgKDIuNi4zMi41OS0w
LDcuMSkgYW5kIAo+ID4gZml4ZWQgaXQgKHNlZSBmaXJzdCBwYXRjaCBpZm9yIDIuNi4zMi42
MCBiZWxvdykuCj4gPiBGb3IgdXMgdGhlIHBhdGNoIHdvcmtzIGZpbmUgKHRlc3RlZCBvbiAy
LjYuMzIuNTktMC43LjEpLgo+ID4gCj4gPiBBRkFJQ1MgZnJvbSB0aGUgY29kZSwgdGhlIHBy
b2JsZW0gc2VlbXMgdG8gcGVyc2lzdCBpbiBjdXJyZW50IGtlcm5lbCAKPiA+IHZlcnNpb25z
IGFsc28uIFRodXMsIEkgYWRkZWQgdGhlIHNlY29uZCBwYXRjaCBmb3IgMy43LjkuCj4gPiBB
cyB0aGUgc2V0dXAgdG8gcmVwcm9kdWNlIHRoZSBwcm9ibGVtIGlzIHF1aXRlIGNvbXBsZXgs
IEkgY291bGRuJ3QgCj4gPiB0ZXN0IHRoZSBzZWNvbmQgcGF0Y2ggeWV0LiBTbyBjb25zaWRl
ciB0aGlzIG9uZSBhcyBhIFJGQy4KPiA+IAo+ID4gQmVzdCByZWdhcmRzLAo+ID4gQm9kbwo+
ID4gCj4gPiBQbGVhc2UgQ0MgbWUsIEknbSBub3Qgb24gdGhlIGxpc3QuCj4gPiAKPiA+ID09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Cj4gPiBGcm9tOiBCb2Rv
IFN0cm9lc3NlciA8YnN0cm9lc3NlckB0cy5mdWppdHN1LmNvbT4KPiA+IERhdGU6IEZyaSwg
MDggRmViIDIwMTMKPiA+IFN1YmplY3Q6IFtQQVRDSF0gbmV0OiBzdW5ycGM6IGZpeCByYWNl
cyBpbiBSUEMgY2FjaGUKPiA+IAo+ID4gV2UgZm91bmQgdGhlIHByb2JsZW0gYW5kIHRlc3Rl
ZCB0aGUgcGF0Y2ggb24gYSBTTEVTMTEgU1AxIAo+ID4gMi42LjMyLjU5LTAuNy4xCj4gPiAK
PiA+IFRoaXMgcGF0Y2ggYXBwbGllcyB0byBsaW51eC0yLjYuMzIuNjAgKGNoYW5nZWQgbW9u
b3RvbmljX3NlY29uZHMgLS0+Cj4gPiBnZXRfc2Vjb25kcygpKQo+ID4gCj4gPiBTcG9yYWRp
Y2FsbHkgTkZTMyBSUEMgcmVxdWVzdHMgdG8gdGhlIG5mcyBzZXJ2ZXIgYXJlIGRyb3BwZWQg
ZHVlIHRvCj4gPiBjYWNoZV9jaGVjaygpIChuZXQvc3VucnBjL2NhY2hlLmMpIHJldHVybmlu
ZyAtRVRJTUVET1VUIGZvciBhbiBlbnRyeSAKPiA+IG9mIHRoZSAiYXV0aC51bml4LmdpZCIg
Y2FjaGUuCj4gPiBJbiB0aGlzIGNhc2UsIG5vIE5GUyByZXBseSBpcyBzZW50IHRvIHRoZSBj
bGllbnQuCj4gPiAKPiA+IFRoZSByZWFzb24gZm9yIHRoZSBkcm9wcGVkIHJlcXVlc3RzIGFy
ZSByYWNlcyBpbiBjYWNoZV9jaGVjaygpIHdoZW4KPiA+IGNhY2hlX21ha2VfdXBjYWxsKCkg
cmV0dXJucyAtRUlOVkFMIChiZWNhdXNlIGl0IGlzIGNhbGxlZCBmb3IgYSBjYWNoZSAKPiA+
IHdpdGhvdXQgcmVhZGVycykgYW5kIGNhY2hlX2NoZWNrKCkgdGhlcmVmb3JlIHJlZnJlc2hl
cyB0aGUgY2FjaGUgZW50cnkgCj4gPiAocnYgPT0gLUVBR0FJTikuCj4gPiAKPiA+IFRoZXJl
IGFyZSB0aHJlZSBkZXRhaWxzIHRoYXQgbmVlZCB0byBiZSBjaGFuZ2VkOgo+ID4gIDEpIGNh
Y2hlX3JldmlzaXRfcmVxdWVzdCgpIG11c3Qgbm90IGJlIGNhbGxlZCBiZWZvcmUgY2FjaGVf
ZnJlc2hfbG9ja2VkKCkKPiA+ICAgICBoYXMgdXBkYXRlZCB0aGUgY2FjaGUgZW50cnksIGFz
IGNhY2hlX3JldmlzaXRfcmVxdWVzdCgpIHdha2VzIHVwCj4gPiAgICAgdGhyZWFkcyB3YWl0
aW5nIGZvciB0aGUgY2FjaGUgZW50cnkgdG8gYmUgdXBkYXRlZC4KPiAKPiBUaGlzIGNlcnRh
aW5seSBzZWVtcyBjb3JyZWN0LiAgSXQgaXMgd3JvbmcgdG8gY2FsbCBjYWNoZV9yZXZpc2l0
X3JlcXVlc3QoKSBzbyBlYXJseS4KPiAKPiA+ICAgICBUaGUgZXhwbGljaXQgY2FsbCB0byBj
YWNoZV9yZXZpc2l0X3JlcXVlc3QoKSBpcyBub3QgbmVlZGVkLCBhcwo+ID4gICAgIGNhY2hl
X2ZyZXNoX3VubG9ja2VkKCkgY2FsbHMgaXQgYW55d2F5Lgo+IAo+IEJ1dCBjYWNoZV9mcmVz
aF91bmxvY2tlZCBpcyBvbmx5IGNhbGxlZCBpZiAicnYgPT0gLUVBR0FJTiIsIGhvd2V2ZXIg
d2UgYWxzbyBuZWVkIHRvIGNhbGwgaXQgaW4gdGhlIGNhc2Ugd2hlcmUgImFnZSA+IHJlZnJl
c2hfYWdlLzIiIC0gaXQgbXVzdCBhbHdheXMgYmUgY2FsbGVkIGFmdGVyIGNsZWFyaW5nIENB
Q0hFX1BFTkRJTkcuCj4gCj4gQWxzbywgY2FjaGVfZnJlc2hfdW5sb2NrZWQoKSBvbmx5IGNh
bGxzIGNhY2hlX3JldmlzaXRfcmVxdWVzdCgpIGlmIENBQ0hFX1BFTkRJTkcgaXMgc2V0LCBi
dXQgd2UgaGF2ZSBqdXN0IGNsZWFyZWQgaXQhICBTb21lIGRlZmluaXRlbHkgc29tZXRoaW5n
IHdyb25nIGhlcmUuCj4gKE5vdGUgdGhhdCBJJ20gbG9va2luZyBhdCB0aGUgU0xFUyAyLjYu
MzIgY29kZSBhdCB0aGUgbW9tZW50LCBtYWlubGluZSBpcyBhIGJpdCBkaWZmZXJlbnQpLgo+
IAo+IAo+ID4gICAgIChCdXQgaW4gY2FzZSBvZiBub3QgdXBkYXRpbmcgdGhlIGNhY2hlIGVu
dHJ5LCBjYWNoZV9yZXZpc2l0X3JlcXVlc3QoKQo+ID4gICAgIG11c3QgYmUgY2FsbGVkLiBU
aHVzLCB3ZSB1c2UgYSBmYWxsIHRocm91Z2ggaW4gdGhlICJjYXNlIikuCj4gCj4gSG1tLi4u
IEkgZG9uJ3QgbGlrZSBjYXNlIGZhbGx0aHJvdWdocyB1bmxlc3MgdGhleSBoYXZlIG5pY2Ug
YmlnIGNvbW1lbnRzOgo+ICAgICAvKiBGQUxMVEhST1VHSCAqLwo+IG9yIHNpbWlsYXIuIDot
KQo+IAo+ID4gIDIpIENBQ0hFX1BFTkRJTkcgbXVzdCBub3QgYmUgY2xlYXJlZCBiZWZvcmUg
Y2FjaGVfZnJlc2hfbG9ja2VkKCkgaGFzCj4gPiAgICAgdXBkYXRlZCB0aGUgY2FjaGUgZW50
cnksIGFzIGNhY2hlX2RlZmVyX3JlcSgpIGNhbiByZXR1cm4gd2l0aG91dCByZWFsbHkKPiA+
ICAgICBzbGVlcGluZyBpZiBpdCBkZXRlY3RzIENBQ0hFX1BFTkRJTkcgdW5zZXQuCj4gCj4g
QWdyZWVkLiAgU28gd2Ugc2hvdWxkIGxlYXZlIHRoZSBjbGVhcmluZyBvZiBDQUNIRV9QRU5E
SU5HIHRvIGNhY2hlX2ZyZXNoX3VubG9ja2VkKCkuCj4gCj4gCj4gPiAgICAgKEluIGNhc2Ug
b2Ygbm90IHVwZGF0aW5nIHRoZSBjYWNoZSBlbnRyeSBhZ2FpbiB3ZSB1c2UgdGhlIGZhbGwg
Cj4gPiB0aHJvdWdoKQo+ID4gIDMpIEltYWdpbmUgYSB0aHJlYWQgdGhhdCBjYWxscyBjYWNo
ZV9jaGVjaygpIGFuZCBnZXRzIHJ2ID0gLUVOT0VOVCBmcm9tCj4gPiAgICAgY2FjaGVfaXNf
dmFsaWQoKS4gVGhlbiBpdCBzZXRzIENBQ0hFX1BFTkRJTkdzIGFuZCBjYWxscwo+ID4gICAg
IGNhY2hlX21ha2VfdXBjYWxsKCkuCj4gPiAgICAgV2UgYXNzdW1lIHRoYXQgbWVhbndoaWxl
IGdldF9zZWNvbmRzKCkgYWR2YW5jZXMgdG8gdGhlIG5leHQKPiA+ICAgICBzZWMuIGFuZCBh
IHNlY29uZCB0aHJlYWQgYWxzbyBjYWxscyBjYWNoZV9jaGVjaygpLiBJdCBnZXRzIC1FQUdB
SU4gZnJvbQo+ID4gICAgIGNhY2hlX2lzX3ZhbGlkKCkgZm9yIHRoZSBzYW1lIGNhY2hlIGVu
dHJ5LiBBcyBDQUNIRV9QRU5ESU5HIHN0aWxsIGlzCj4gPiAgICAgc2V0LCBpdCBjYWxscyBj
YWNoZV9kZWZlcl9yZXEoKSBpbW1lZGlhdGVseSBhbmQgd2FpdHMgZm9yIGEgd2FrZXVwIGZy
b20KPiA+ICAgICB0aGUgZmlyc3QgdGhyZWFkLgo+ID4gICAgIEFmdGVyIGNhY2hlX21ha2Vf
dXBjYWxsKCkgcmV0dXJuZWQgLUVJTlZBTCwgdGhlIGZpcnN0IHRocmVhZCBkb2VzIG5vdAo+
ID4gICAgIHVwZGF0ZSB0aGUgY2FjaGUgZW50cnkgYXMgaXQgaGFkIGdvdCBydiA9IC1FTk9F
TlQsIGJ1dCB3YWtlcyB1cCB0aGUKPiA+ICAgICBzZWNvbmQgdGhyZWFkIGJ5IGNhbGxpbmcg
Y2FjaGVfcmV2aXNpdF9yZXF1ZXN0KCkuCj4gPiAgICAgVGhyZWFkIHR3byB3YWtlcyB1cCwg
Y2FsbHMgY2FjaGVfaXNfdmFsaWQoKSBhbmQgYWdhaW4gZ2V0cyAtRUFHQUlOLgo+ID4gICAg
IFRodXMsIHRoZSByZXN1bHQgb2YgdGhlIHNlY29uZCBjYWNoZV9jaGVjaygpIGlzIC1FVElN
RURPVVQgYW5kIHRoZQo+ID4gICAgIE5GUyByZXF1ZXN0IGlzIGRyb3BwZWQuCj4gCj4gWWVw
LCB0aGF0J3Mgbm90IHNvIGdvb2QuLi4uCj4gCj4gCj4gPiAgICAgVG8gc29sdmUgdGhpcywg
dGhlIGNhY2hlIGVudHJ5IG5vdyBpcyB1cGRhdGVkIG5vdCBvbmx5IGlmIHJ2ID09IC1FQUdB
SU4KPiA+ICAgICBidXQgaWYgcnYgPT0gLUVOT0VOVCBhbHNvLiBUaGlzIGlzIGEgc3VmZmlj
aWVudCB3b3JrYXJvdW5kLCBhcyB0aGUKPiA+ICAgICBmaXJzdCB0aHJlYWQgd291bGQgaGF2
ZSB0byBzdGF5IGluIGNhY2hlX2NoZWNrKCkgYmV0d2VlbiBpdHMgY2FsbCB0bwo+ID4gICAg
IGNhY2hlX2lzX3ZhbGlkKCkgYW5kIGNsZWFyaW5nIENBQ0hFX1BFTkRJTkcgZm9yIG1vcmUg
dGhhbiA2MCBzZWNvbmRzCj4gPiAgICAgdG8gYnJlYWsgdGhlIHdvcmthcm91bmQuCj4gCj4g
U3RpbGwsIGl0IGlzbid0IG5pY2UgdG8ganVzdCBoYXZlIGEgd29yay1hcm91bmQuICBJdCB3
b3VsZCBiZSBiZXN0IHRvIGhhdmUgYSBmaXguCj4gVGhlIGtleSBwcm9ibGVtIGhlcmUgaXMg
dGhhdCBjYWNoZV9pc192YWxpZCgpIGlzIHRpbWUtc2Vuc2l0aXZlLiAgVGhpcyBoYXZlIGJl
ZW4gYWRkcmVzcyBpbiBtYWlubGluZSAtIGNhY2hlX2lzX3ZhbGlkIGRvZXNuJ3QgZGVwZW5k
IG9uIHRoZSBjdXJyZW50IHRpbWUgdGhlcmUuCgpTbywgdGhlIHNvbHV0aW9uIHdvdWxkIGJl
IGEgYmFja3BvcnQgb2YgdGhlIGN1cnJlbnQgbWFpbmxpbmUgY29kZSAuLi4KCkFueXdheSwg
SSB0aGluayBmb3IgU0xFUzExIFNQMSBhbmQgMi42LjMyLjYwIHRoZSB3b3JrLWFyb3VuZCB3
b3VsZCBiZSBzdWZmaWNpZW50LgpCVFc6IGl0IGhhcyB0aGUgcG9zaXRpdmUgc2lkZSBlZmZl
Y3QsIHRoYXQgLSB3aGlsZSBhIGNhY2hlIGVudHJ5IGlzIGluIGl0cyBzZWNvbmQKaGFsZiBv
ZiBsaWZlIC0gbm8gbG9uZ2VyIGVhY2ggY2FjaGVfY2hlY2soKSB0cmllcyB0byBkbyBhIGNh
Y2hlX21ha2VfdXBjYWxsKCkuCgoKPiAKPiAKPiA+ICAgICAKPiA+IFNpZ25lZC1vZmYtYnk6
IEJvZG8gU3Ryb2Vzc2VyIDxic3Ryb2Vzc2VyQHRzLmZ1aml0c3UuY29tPgo+ID4gLS0tCj4g
PiAKPiA+IC0tLSBhL25ldC9zdW5ycGMvY2FjaGUuYwkyMDEyLTA4LTA4IDIxOjM1OjA5LjAw
MDAwMDAwMCArMDIwMAo+ID4gKysrIGIvbmV0L3N1bnJwYy9jYWNoZS5jCTIwMTMtMDItMDgg
MTQ6Mjk6NDEuMDAwMDAwMDAwICswMTAwCj4gPiBAQCAtMjMzLDE1ICsyMzMsMTQgQEAgaW50
IGNhY2hlX2NoZWNrKHN0cnVjdCBjYWNoZV9kZXRhaWwgKmRldAo+ID4gIAkJaWYgKCF0ZXN0
X2FuZF9zZXRfYml0KENBQ0hFX1BFTkRJTkcsICZoLT5mbGFncykpIHsKPiA+ICAJCQlzd2l0
Y2ggKGNhY2hlX21ha2VfdXBjYWxsKGRldGFpbCwgaCkpIHsKPiA+ICAJCQljYXNlIC1FSU5W
QUw6Cj4gPiAtCQkJCWNsZWFyX2JpdChDQUNIRV9QRU5ESU5HLCAmaC0+ZmxhZ3MpOwo+ID4g
LQkJCQljYWNoZV9yZXZpc2l0X3JlcXVlc3QoaCk7Cj4gPiAtCQkJCWlmIChydiA9PSAtRUFH
QUlOKSB7Cj4gPiArCQkJCWlmIChydiA9PSAtRUFHQUlOIHx8IHJ2ID09IC1FTk9FTlQpIHsK
PiA+ICAJCQkJCXNldF9iaXQoQ0FDSEVfTkVHQVRJVkUsICZoLT5mbGFncyk7Cj4gPiAgCQkJ
CQljYWNoZV9mcmVzaF9sb2NrZWQoaCwgZ2V0X3NlY29uZHMoKStDQUNIRV9ORVdfRVhQSVJZ
KTsKPiA+ICsJCQkJCWNsZWFyX2JpdChDQUNIRV9QRU5ESU5HLCAmaC0+ZmxhZ3MpOwo+ID4g
IAkJCQkJY2FjaGVfZnJlc2hfdW5sb2NrZWQoaCwgZGV0YWlsKTsKPiA+ICAJCQkJCXJ2ID0g
LUVOT0VOVDsKPiA+ICsJCQkJCWJyZWFrOwo+ID4gIAkJCQl9Cj4gPiAtCQkJCWJyZWFrOwo+
ID4gIAo+ID4gIAkJCWNhc2UgLUVBR0FJTjoKPiA+ICAJCQkJY2xlYXJfYml0KENBQ0hFX1BF
TkRJTkcsICZoLT5mbGFncyk7Cj4gCj4gSSBhZ3JlZSB3aXRoIHNvbWUgb2YgdGhpcy4uLi4K
PiBNYXliZToKPiAKPiAgIHN3aXRjaChjYWNoZV9tYWtlX3VwY2FsbChkZXRhaWwsIGgpKSB7
Cj4gICBjYXNlIC1FSU5WQUw6Cj4gICAgICAgICBpZiAocnYpIHsKPiAJCXNldF9iaXQoQ0FD
SEVfTkVHQVRJVkUsICZoLT5mbGFncyk7Cj4gCQljYWNoZV9mcmVzaF9sb2NrZWQoaCwgZ2V0
X3NlY29uZHMoKSArIENBQ0hFX05FV19FWFBJUlkpOwo+IAkJcnYgPSAtRU5PRU5UOwo+IAl9
Cj4gCS8qIEZBTExUSFJPVUdIICovCj4gICBjYXNlIC1FQUdBSU46Cj4gCWNhY2hlX2ZyZXNo
X3VubG9ja2VkKGgsIGRldGFpbCk7Cj4gICB9CgpJIGFncmVlLCB5b3VyIHBhdGNoIGlzIG9i
dmlvdXNseSBiZXR0ZXIgdGhhbiB0aGUgbWluZS4KQnV0IGxldCBtZSBzdWdnZXN0IG9uZSBs
aXR0bGUgY2hhbmdlOiBJIHdvdWxkIGxpa2UgdG8gc3Vic3RpdHV0ZQpjYWNoZV9mcmVzaF91
bmxvY2tlZCgpIGJ5IGNsZWFyX2JpdCgpIGFuZCBjYWNoZV9yZXZpc2l0X3JlcXVlc3QoKSwK
YXMgdGhlIGNhbGwgdG8gY2FjaGVfZGVxdWV1ZSgpIGluIGNhY2hlX2ZyZXNoX3VubG9ja2Vk
KCkgc2VlbXMgdG8KYmUgb2Jzb2xldGUgaGVyZToKCiAgIHN3aXRjaChjYWNoZV9tYWtlX3Vw
Y2FsbChkZXRhaWwsIGgpKSB7CiAgIGNhc2UgLUVJTlZBTDoKCWlmIChydikgewogCQlzZXRf
Yml0KENBQ0hFX05FR0FUSVZFLCAmaC0+ZmxhZ3MpOwogCQljYWNoZV9mcmVzaF9sb2NrZWQo
aCwgZ2V0X3NlY29uZHMoKSArIENBQ0hFX05FV19FWFBJUlkpOwoJCXJ2ID0gLUVOT0VOVDsK
IAl9CgkvKiBGQUxMVEhST1VHSCAqLwogICBjYXNlIC1FQUdBSU46CgljbGVhcl9iaXQoQ0FD
SEVfUEVORElORywgJmgtPmZsYWdzKTsKCWNhY2hlX3JldmlzaXRfcmVxdWVzdChoLCBkZXRh
aWwpOwogICB9Cgo+IAo+IFRob3VnaCBpdCBpc24ndCBnb29kIHRoYXQgY2FjaGVfZnJlc2hf
bG9ja2VkKCkgaXMgYmVpbmcgY2FsbGVkIHdpdGhvdXQgaG9sZGluZyBhIGxvY2shICBNYXli
ZSB3ZSBzaG91bGQgaW1wb3J0IHRyeV90b19uZWdhdGVfZW50cnkoKSBmcm9tIG1haW5saW5l
LgoKT3Igc2ltcGx5IGFkZCB3cml0ZV9sb2NrKCkgYW5kIHdyaXRlX3VubG9jaygpIGFyb3Vu
ZCB0aGUgc2V0X2JpdCgpIGFuZCBjYWNoZV9mcmVzaF9sb2NrZWQoKQoKCj4gCj4gPiA9PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQo+ID4gRnJvbTogQm9kbyBT
dHJvZXNzZXIgPGJzdHJvZXNzZXJAdHMuZnVqaXRzdS5jb20+Cj4gPiBEYXRlOiBGcmksIDA4
IEZlYiAyMDEzCj4gPiBTdWJqZWN0OiBbUEFUQ0hdIG5ldDogc3VucnBjOiBmaXggcmFjZXMg
aW4gUlBDIGNhY2hlCj4gPiAKPiA+IFRoaXMgcGF0Y2ggYXBwbGllcyB0byBTTEVTIDExIFNQ
MiBsaW51eC0zLjAuNTEtMC43LjkgYW5kIGFsc28gdG8gCj4gPiB2YW5pbGxhIGxpbnV4LTMu
Ny45Cj4gPiAKPiA+IEl0IGlzIHVudGVzdGVkIGFuZCBpcyBvbmx5IGJhc2VkIG9uIGEgY29k
ZSByZXZpZXcgYWZ0ZXIgd2UgYW5hbHl6ZWQgCj4gPiB0aGUgcmVhc29uIGZvciBORlMgcmVx
dWVzdHMgYmVpbmcgZHJvcHBlZCBvbiBhCj4gPiBTTEVTMTEgU1AxIChsaW51eC0yLjYuMzIu
NTktMC43LjEpCj4gPiAKPiA+IFNwb3JhZGljYWxseSBORlMzIFJQQyByZXF1ZXN0cyB0byB0
aGUgbmZzIHNlcnZlciBhcmUgZHJvcHBlZCBkdWUgdG8KPiA+IGNhY2hlX2NoZWNrKCkgKG5l
dC9zdW5ycGMvY2FjaGUuYykgcmV0dXJuaW5nIC1FVElNRURPVVQgZm9yIGFuIGVudHJ5IAo+
ID4gb2YgdGhlICJhdXRoLnVuaXguZ2lkIiBjYWNoZS4KPiA+IEluIHRoaXMgY2FzZSwgbm8g
TkZTIHJlcGx5IGlzIHNlbnQgdG8gdGhlIGNsaWVudC4KPiA+IAo+ID4gVGhlIHJlYXNvbiBm
b3IgdGhlIGRyb3BwZWQgcmVxdWVzdHMgYXJlIHJhY2VzIGluIGNhY2hlX2NoZWNrKCkgd2hl
bgo+ID4gY2FjaGVfbWFrZV91cGNhbGwoKSByZXR1cm5zIC1FSU5WQUwgKGJlY2F1c2UgaXQg
aXMgY2FsbGVkIGZvciBhIGNhY2hlIAo+ID4gd2l0aG91dCByZWFkZXJzKSBhbmQgY2FjaGVf
Y2hlY2soKSB0aGVyZWZvcmUgcmVmcmVzaGVzIHRoZSBjYWNoZSBlbnRyeSAKPiA+IChydiA9
PSAtRUFHQUlOKS4KPiA+IAo+ID4gVGhlcmUgYXJlIHR3byBkZXRhaWxzIHRoYXQgbmVlZCB0
byBiZSBjaGFuZ2VkOgo+ID4gIDEpIGNhY2hlX3JldmlzaXRfcmVxdWVzdCgpIG11c3Qgbm90
IGJlIGNhbGxlZCBiZWZvcmUgY2FjaGVfZnJlc2hfbG9ja2VkKCkKPiA+ICAgICBoYXMgdXBk
YXRlZCB0aGUgY2FjaGUgZW50cnksIGFzIGNhY2hlX3JldmlzaXRfcmVxdWVzdCgpIHdha2Vz
IHVwCj4gPiAgICAgdGhyZWFkcyB3YWl0aW5nIGZvciB0aGUgY2FjaGUgZW50cnkgdG8gYmUg
dXBkYXRlZC4KPiA+ICAgICBUaGUgZXhwbGljaXQgY2FsbCB0byBjYWNoZV9yZXZpc2l0X3Jl
cXVlc3QoKSBpcyBub3QgbmVlZGVkLCBhcwo+ID4gICAgIGNhY2hlX2ZyZXNoX3VubG9ja2Vk
KCkgY2FsbHMgaXQgYW55d2F5Lgo+ID4gICAgIChCdXQgaW4gY2FzZSBvZiBub3QgdXBkYXRp
bmcgdGhlIGNhY2hlIGVudHJ5LCBjYWNoZV9yZXZpc2l0X3JlcXVlc3QoKQo+ID4gICAgIG11
c3QgYmUgY2FsbGVkKS4KPiA+ICAyKSBDQUNIRV9QRU5ESU5HIG11c3Qgbm90IGJlIGNsZWFy
ZWQgYmVmb3JlIGNhY2hlX2ZyZXNoX2xvY2tlZCgpIGhhcwo+ID4gICAgIHVwZGF0ZWQgdGhl
IGNhY2hlIGVudHJ5LCBhcyBjYWNoZV93YWl0X3JlcSgpIGNhbGxlZCBieQo+ID4gICAgIGNh
Y2hlX2RlZmVyX3JlcSgpIGNhbiByZXR1cm4gd2l0aG91dCByZWFsbHkgc2xlZXBpbmcgaWYg
aXQgZGV0ZWN0cwo+ID4gICAgIENBQ0hFX1BFTkRJTkcgdW5zZXQuCj4gPiAKPiA+IFNpZ25l
ZC1vZmYtYnk6IEJvZG8gU3Ryb2Vzc2VyIDxic3Ryb2Vzc2VyQHRzLmZ1aml0c3UuY29tPgo+
ID4gLS0tCj4gPiAKPiA+IC0tLSBhL25ldC9zdW5ycGMvY2FjaGUuYwkyMDEzLTAyLTA4IDE1
OjU2OjA3LjAwMDAwMDAwMCArMDEwMAo+ID4gKysrIGIvbmV0L3N1bnJwYy9jYWNoZS5jCTIw
MTMtMDItMDggMTY6MDQ6MzIuMDAwMDAwMDAwICswMTAwCj4gPiBAQCAtMjMwLDExICsyMzAs
MTQgQEAgc3RhdGljIGludCB0cnlfdG9fbmVnYXRlX2VudHJ5KHN0cnVjdCBjYQo+ID4gIAly
diA9IGNhY2hlX2lzX3ZhbGlkKGRldGFpbCwgaCk7Cj4gPiAgCWlmIChydiAhPSAtRUFHQUlO
KSB7Cj4gPiAgCQl3cml0ZV91bmxvY2soJmRldGFpbC0+aGFzaF9sb2NrKTsKPiA+ICsJCWNs
ZWFyX2JpdChDQUNIRV9QRU5ESU5HLCAmaC0+ZmxhZ3MpOwo+ID4gKwkJY2FjaGVfcmV2aXNp
dF9yZXF1ZXN0KGgpOwo+IAo+IFRoaXMgc2hvdWxkIGp1c3QgYmUgY2FjaGVfZnJlc2hfdW5s
b2NrZWQoKSwgYXMgYmVsb3cuCj4gPiAgCQlyZXR1cm4gcnY7Cj4gPiAgCX0KPiA+ICAJc2V0
X2JpdChDQUNIRV9ORUdBVElWRSwgJmgtPmZsYWdzKTsKPiA+ICAJY2FjaGVfZnJlc2hfbG9j
a2VkKGgsIHNlY29uZHNfc2luY2VfYm9vdCgpK0NBQ0hFX05FV19FWFBJUlkpOwo+ID4gIAl3
cml0ZV91bmxvY2soJmRldGFpbC0+aGFzaF9sb2NrKTsKPiA+ICsJY2xlYXJfYml0KENBQ0hF
X1BFTkRJTkcsICZoLT5mbGFncyk7Cj4gQ2xlYXJpbmcgdGhpcyBiaXQgaXMgd3JvbmcgIC0g
Y2FjaGVfZnJzaF91bmxvY2tlZCB3aWxsIGRvIHRoYXQuCgpZZXMuIE15IGZhdWx0LiBCdXQg
aW5zdGVhZCBvZiBzdHJpcHBpbmcgdGhlIGNsZWFyX2JpdCgpLCBJJ2QgYmV0dGVyIGxpa2UK
dG8gY2hhbmdlIHRoZSBmb2xsb3dpbmcgY2FjaGVfZnJlc2hfdW5sb2NrZWQoKSB0byBjYWNo
ZV9yZXZpc2l0X3JlcXVlc3QoKQoKPiAKPiA+ICAJY2FjaGVfZnJlc2hfdW5sb2NrZWQoaCwg
ZGV0YWlsKTsKPiA+ICAJcmV0dXJuIC1FTk9FTlQ7Cj4gPiAgfQo+IFNvIG1heWJlOgo+IAo+
IHN0YXRpYyBpbnQgdHJ5X3RvX25lZ2F0ZV9lbnRyeSguLi4uKQo+IHsKPiAJaW50IHJ2Owo+
IAo+IAl3cml0ZV9sb2NrKCZkZXRhaWwtPmhhc2hfbG9jayk7Cj4gCXJ2ID0gY2FjaGVfaXNf
dmFsaWQoZGV0YWlsLCBoKTsKPiAJaWYgKHJ2ID09IC1FQUdBSU4pIHsKPiAJCXNldF9iaXQo
Q0FDSEVfTkVHQVRJVkUsICZoLT5mbGFncyk7Cj4gCQljYWNoZV9mcmVzaF9sb2NrZWQoaCwg
Li4uLik7Cj4gCQlydiA9IC1FTk9FTlQ7Cj4gCX0KPiAJd3JpdGVfdW5sb2NrKCZkZXRhaWwt
Pmhhc2hfbG9jayk7Cj4gCWNhY2hlX2ZyZXNoX3VubG9ja2VkKGgsIGRldGFpbCk7CgpGb3Ig
dGhlIHNhbWUgcmVhc29uIGFzIHdyaXR0ZW4gYWJvdmUsIEknZCBiZXR0ZXIgbGlrZQpjbGVh
cl9iaXQoKSBhbmQgY2FjaGVfcmV2aXNpdF9yZXF1ZXN0KCkKCj4gCXJldHVybiBydjsKPiB9
Cj4gPz8/Pwo+IAo+IAo+ID4gQEAgLTI3NSw4ICsyNzgsNiBAQCBpbnQgY2FjaGVfY2hlY2so
c3RydWN0IGNhY2hlX2RldGFpbCAqZGV0Cj4gPiAgCQlpZiAoIXRlc3RfYW5kX3NldF9iaXQo
Q0FDSEVfUEVORElORywgJmgtPmZsYWdzKSkgewo+ID4gIAkJCXN3aXRjaCAoY2FjaGVfbWFr
ZV91cGNhbGwoZGV0YWlsLCBoKSkgewo+ID4gIAkJCWNhc2UgLUVJTlZBTDoKPiA+IC0JCQkJ
Y2xlYXJfYml0KENBQ0hFX1BFTkRJTkcsICZoLT5mbGFncyk7Cj4gPiAtCQkJCWNhY2hlX3Jl
dmlzaXRfcmVxdWVzdChoKTsKPiA+ICAJCQkJcnYgPSB0cnlfdG9fbmVnYXRlX2VudHJ5KGRl
dGFpbCwgaCk7Cj4gPiAgCQkJCWJyZWFrOwo+ID4gIAkJCWNhc2UgLUVBR0FJTjoKPiAKPiBZ
ZXMsIHRob3NlIGxpbmVzIHNob3VsZCBkZWZpbml0ZWx5IGJlIHJlbW92ZWQuCj4gCj4gU28g
bWF5YmUgdGhpcyBhZ2FpbnN0IG1haW5saW5lOgo+IAo+IGRpZmYgLS1naXQgYS9uZXQvc3Vu
cnBjL2NhY2hlLmMgYi9uZXQvc3VucnBjL2NhY2hlLmMgaW5kZXggOWFmYTQzOS4uNzI5NjY0
NCAxMDA2NDQKPiAtLS0gYS9uZXQvc3VucnBjL2NhY2hlLmMKPiArKysgYi9uZXQvc3VucnBj
L2NhY2hlLmMKPiBAQCAtMjI4LDEyICsyMjgsMTEgQEAgc3RhdGljIGludCB0cnlfdG9fbmVn
YXRlX2VudHJ5KHN0cnVjdCBjYWNoZV9kZXRhaWwgKmRldGFpbCwgc3RydWN0IGNhY2hlX2hl
YWQgKmgKPiAgCj4gIAl3cml0ZV9sb2NrKCZkZXRhaWwtPmhhc2hfbG9jayk7Cj4gIAlydiA9
IGNhY2hlX2lzX3ZhbGlkKGRldGFpbCwgaCk7Cj4gLQlpZiAocnYgIT0gLUVBR0FJTikgewo+
IC0JCXdyaXRlX3VubG9jaygmZGV0YWlsLT5oYXNoX2xvY2spOwo+IC0JCXJldHVybiBydjsK
PiArCWlmIChydiA9PSAtRUFHQUlOKSB7Cj4gKwkJc2V0X2JpdChDQUNIRV9ORUdBVElWRSwg
JmgtPmZsYWdzKTsKPiArCQljYWNoZV9mcmVzaF9sb2NrZWQoaCwgc2Vjb25kc19zaW5jZV9i
b290KCkrQ0FDSEVfTkVXX0VYUElSWSk7Cj4gKwkJcnYgPSAtRU5PRU5UOwo+ICAJfQo+IC0J
c2V0X2JpdChDQUNIRV9ORUdBVElWRSwgJmgtPmZsYWdzKTsKPiAtCWNhY2hlX2ZyZXNoX2xv
Y2tlZChoLCBzZWNvbmRzX3NpbmNlX2Jvb3QoKStDQUNIRV9ORVdfRVhQSVJZKTsKPiAgCXdy
aXRlX3VubG9jaygmZGV0YWlsLT5oYXNoX2xvY2spOwo+ICAJY2FjaGVfZnJlc2hfdW5sb2Nr
ZWQoaCwgZGV0YWlsKTsKPiAgCXJldHVybiAtRU5PRU5UOwo+IEBAIC0yNzUsMTMgKzI3NCwx
MCBAQCBpbnQgY2FjaGVfY2hlY2soc3RydWN0IGNhY2hlX2RldGFpbCAqZGV0YWlsLAo+ICAJ
CWlmICghdGVzdF9hbmRfc2V0X2JpdChDQUNIRV9QRU5ESU5HLCAmaC0+ZmxhZ3MpKSB7Cj4g
IAkJCXN3aXRjaCAoY2FjaGVfbWFrZV91cGNhbGwoZGV0YWlsLCBoKSkgewo+ICAJCQljYXNl
IC1FSU5WQUw6Cj4gLQkJCQljbGVhcl9iaXQoQ0FDSEVfUEVORElORywgJmgtPmZsYWdzKTsK
PiAtCQkJCWNhY2hlX3JldmlzaXRfcmVxdWVzdChoKTsKPiAgCQkJCXJ2ID0gdHJ5X3RvX25l
Z2F0ZV9lbnRyeShkZXRhaWwsIGgpOwo+ICAJCQkJYnJlYWs7Cj4gIAkJCWNhc2UgLUVBR0FJ
TjoKPiAtCQkJCWNsZWFyX2JpdChDQUNIRV9QRU5ESU5HLCAmaC0+ZmxhZ3MpOwo+IC0JCQkJ
Y2FjaGVfcmV2aXNpdF9yZXF1ZXN0KGgpOwo+ICsJCQkJY2FjaGVfZnJlc2hfdW5sb2NrZWQo
aCwgZGV0YWlsKTsKCkFuZCBhZ2FpbjogSSBkb24ndCBsaWtlIGl0IDstKQoKPiAgCQkJCWJy
ZWFrOwo+ICAJCQl9Cj4gIAkJfQo+IAo+IAo+IElzIHRoYXQgY29udmluY2luZz8KCk9mIGNv
dXJzZS4KCkRvIHlvdSB0aGluayBpdCB3b3VsZCBiZSBhIGdvb2QgaWRlYSB0byB1c2UgdGhl
IHNhbWUgY29kZSBzdHJ1Y3R1cmUgYXMgc3VnZ2VzdGVkIGZvciAyLjYuMzIuNjAgPwpJZiBz
bywgdGhpcyB3b3VsZCByZXN1bHQgaW4gdGhlIGZvbGxvd2luZyBwYXRjaDoKCi0tLSBhL25l
dC9zdW5ycGMvY2FjaGUuYwkyMDEzLTAyLTIwIDEzOjU2OjMwLjAwMDAwMDAwMCArMDEwMAor
KysgYi9uZXQvc3VucnBjL2NhY2hlLmMJMjAxMy0wMi0yMCAxNDowNTowOC4wMDAwMDAwMDAg
KzAxMDAKQEAgLTIyMiwyMyArMjIyLDYgQEAKIAl9CiB9Cgotc3RhdGljIGludCB0cnlfdG9f
bmVnYXRlX2VudHJ5KHN0cnVjdCBjYWNoZV9kZXRhaWwgKmRldGFpbCwgc3RydWN0IGNhY2hl
X2hlYWQgKmgpCi17Ci0JaW50IHJ2OwotCi0Jd3JpdGVfbG9jaygmZGV0YWlsLT5oYXNoX2xv
Y2spOwotCXJ2ID0gY2FjaGVfaXNfdmFsaWQoZGV0YWlsLCBoKTsKLQlpZiAocnYgIT0gLUVB
R0FJTikgewotCQl3cml0ZV91bmxvY2soJmRldGFpbC0+aGFzaF9sb2NrKTsKLQkJcmV0dXJu
IHJ2OwotCX0KLQlzZXRfYml0KENBQ0hFX05FR0FUSVZFLCAmaC0+ZmxhZ3MpOwotCWNhY2hl
X2ZyZXNoX2xvY2tlZChoLCBzZWNvbmRzX3NpbmNlX2Jvb3QoKStDQUNIRV9ORVdfRVhQSVJZ
KTsKLQl3cml0ZV91bmxvY2soJmRldGFpbC0+aGFzaF9sb2NrKTsKLQljYWNoZV9mcmVzaF91
bmxvY2tlZChoLCBkZXRhaWwpOwotCXJldHVybiAtRU5PRU5UOwotfQotCiAvKgogICogVGhp
cyBpcyB0aGUgZ2VuZXJpYyBjYWNoZSBtYW5hZ2VtZW50IHJvdXRpbmUgZm9yIGFsbAogICog
dGhlIGF1dGhlbnRpY2F0aW9uIGNhY2hlcy4KQEAgLTI3NSwxMCArMjU4LDE1IEBACiAJCWlm
ICghdGVzdF9hbmRfc2V0X2JpdChDQUNIRV9QRU5ESU5HLCAmaC0+ZmxhZ3MpKSB7CiAJCQlz
d2l0Y2ggKGNhY2hlX21ha2VfdXBjYWxsKGRldGFpbCwgaCkpIHsKIAkJCWNhc2UgLUVJTlZB
TDoKLQkJCQljbGVhcl9iaXQoQ0FDSEVfUEVORElORywgJmgtPmZsYWdzKTsKLQkJCQljYWNo
ZV9yZXZpc2l0X3JlcXVlc3QoaCk7Ci0JCQkJcnYgPSB0cnlfdG9fbmVnYXRlX2VudHJ5KGRl
dGFpbCwgaCk7Ci0JCQkJYnJlYWs7CisJCQkJd3JpdGVfbG9jaygmZGV0YWlsLT5oYXNoX2xv
Y2spOworCQkJCXJ2ID0gY2FjaGVfaXNfdmFsaWQoZGV0YWlsLCBoKTsKKwkJCQlpZiAocnYg
PT0gLUVBR0FJTikgeworCQkJCQlzZXRfYml0KENBQ0hFX05FR0FUSVZFLCAmaC0+ZmxhZ3Mp
OworCQkJCQljYWNoZV9mcmVzaF9sb2NrZWQoaCwgc2Vjb25kc19zaW5jZV9ib290KCkrQ0FD
SEVfTkVXX0VYUElSWSk7CisJCQkJCXJ2ID0gLUVOT0VOVDsKKwkJCQl9CisJCQkJd3JpdGVf
dW5sb2NrKCZkZXRhaWwtPmhhc2hfbG9jayk7CisJCQkJLyogRkFMTFRIUk9VR0ggKi8KIAkJ
CWNhc2UgLUVBR0FJTjoKIAkJCQljbGVhcl9iaXQoQ0FDSEVfUEVORElORywgJmgtPmZsYWdz
KTsKIAkJCQljYWNoZV9yZXZpc2l0X3JlcXVlc3QoaCk7CgoKCj4gCj4gVGhhbmtzIGEgbG90
IGZvciB5b3VyIHZlcnkgdGhvcm91Z2ggYW5hbHlzaXMhCj4gCj4gTmVpbEJyb3duCj4gCgpU
aGFuayB5b3UgZm9yIHRoZSBxdWljayByZXNwb25zZSEKCkJlc3QgcmVnYXJkcywKQm9kbwo=

Re: [PATCH] sunrpc.ko: RPC cache fix races

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 10498 bytes --]

On 19 Feb 2013 18:08:40 +0100 bstroesser@ts.fujitsu.com wrote:

> Second attempt using the correct FROM. Sorry for the noise.
> 
> 
> Hi,
> 
> I found a problem in sunrpc.ko on a SLES11 SP1 (2.6.32.59-0,7.1)
> and fixed it (see first patch ifor 2.6.32.60 below).
> For us the patch works fine (tested on 2.6.32.59-0.7.1).
> 
> AFAICS from the code, the problem seems to persist in current
> kernel versions also. Thus, I added the second patch for 3.7.9.
> As the setup to reproduce the problem is quite complex, I couldn't
> test the second patch yet. So consider this one as a RFC.
> 
> Best regards,
> Bodo
> 
> Please CC me, I'm not on the list.
> 
> =========================================
> From: Bodo Stroesser <bstroesser@ts.fujitsu.com>
> Date: Fri, 08 Feb 2013
> Subject: [PATCH] net: sunrpc: fix races in RPC cache
> 
> We found the problem and tested the patch on a SLES11 SP1 2.6.32.59-0.7.1
> 
> This patch applies to linux-2.6.32.60 (changed monotonic_seconds -->
> get_seconds())
> 
> Sporadically NFS3 RPC requests to the nfs server are dropped due to
> cache_check() (net/sunrpc/cache.c) returning -ETIMEDOUT for an entry
> of the "auth.unix.gid" cache.
> In this case, no NFS reply is sent to the client.
> 
> The reason for the dropped requests are races in cache_check() when
> cache_make_upcall() returns -EINVAL (because it is called for a cache
> without readers) and cache_check() therefore refreshes the cache entry 
> (rv == -EAGAIN).
> 
> There are three details that need to be changed:
>  1) cache_revisit_request() must not be called before cache_fresh_locked()
>     has updated the cache entry, as cache_revisit_request() wakes up
>     threads waiting for the cache entry to be updated.

This certainly seems correct.  It is wrong to call cache_revisit_request() so
early.

>     The explicit call to cache_revisit_request() is not needed, as
>     cache_fresh_unlocked() calls it anyway.

But cache_fresh_unlocked is only called if "rv == -EAGAIN", however we also
need to call it in the case where "age > refresh_age/2" - it must always be
called after clearing CACHE_PENDING.

Also, cache_fresh_unlocked() only calls cache_revisit_request() if
CACHE_PENDING is set, but we have just cleared it!  Some definitely something
wrong here.
(Note that I'm looking at the SLES 2.6.32 code at the moment, mainline is a
bit different).


>     (But in case of not updating the cache entry, cache_revisit_request()
>     must be called. Thus, we use a fall through in the "case").

Hmm... I don't like case fallthroughs unless they have nice big comments:
    /* FALLTHROUGH */
or similar. :-)

>  2) CACHE_PENDING must not be cleared before cache_fresh_locked() has
>     updated the cache entry, as cache_defer_req() can return without really
>     sleeping if it detects CACHE_PENDING unset.

Agreed.  So we should leave the clearing of CACHE_PENDING to
cache_fresh_unlocked().


>     (In case of not updating the cache entry again we use the fall through)
>  3) Imagine a thread that calls cache_check() and gets rv = -ENOENT from
>     cache_is_valid(). Then it sets CACHE_PENDINGs and calls
>     cache_make_upcall().
>     We assume that meanwhile get_seconds() advances to the next
>     sec. and a second thread also calls cache_check(). It gets -EAGAIN from
>     cache_is_valid() for the same cache entry. As CACHE_PENDING still is
>     set, it calls cache_defer_req() immediately and waits for a wakeup from
>     the first thread.
>     After cache_make_upcall() returned -EINVAL, the first thread does not
>     update the cache entry as it had got rv = -ENOENT, but wakes up the
>     second thread by calling cache_revisit_request().
>     Thread two wakes up, calls cache_is_valid() and again gets -EAGAIN.
>     Thus, the result of the second cache_check() is -ETIMEDOUT and the
>     NFS request is dropped.

Yep, that's not so good....


>     To solve this, the cache entry now is updated not only if rv == -EAGAIN
>     but if rv == -ENOENT also. This is a sufficient workaround, as the
>     first thread would have to stay in cache_check() between its call to
>     cache_is_valid() and clearing CACHE_PENDING for more than 60 seconds
>     to break the workaround.

Still, it isn't nice to just have a work-around.  It would be best to have a
fix.
The key problem here is that cache_is_valid() is time-sensitive.  This have
been address in mainline - cache_is_valid doesn't depend on the current time
there.


>     
> Signed-off-by: Bodo Stroesser <bstroesser@ts.fujitsu.com>
> ---
> 
> --- a/net/sunrpc/cache.c	2012-08-08 21:35:09.000000000 +0200
> +++ b/net/sunrpc/cache.c	2013-02-08 14:29:41.000000000 +0100
> @@ -233,15 +233,14 @@ int cache_check(struct cache_detail *det
>  		if (!test_and_set_bit(CACHE_PENDING, &h->flags)) {
>  			switch (cache_make_upcall(detail, h)) {
>  			case -EINVAL:
> -				clear_bit(CACHE_PENDING, &h->flags);
> -				cache_revisit_request(h);
> -				if (rv == -EAGAIN) {
> +				if (rv == -EAGAIN || rv == -ENOENT) {
>  					set_bit(CACHE_NEGATIVE, &h->flags);
>  					cache_fresh_locked(h, get_seconds()+CACHE_NEW_EXPIRY);
> +					clear_bit(CACHE_PENDING, &h->flags);
>  					cache_fresh_unlocked(h, detail);
>  					rv = -ENOENT;
> +					break;
>  				}
> -				break;
>  
>  			case -EAGAIN:
>  				clear_bit(CACHE_PENDING, &h->flags);

I agree with some of this....
Maybe:

  switch(cache_make_upcall(detail, h)) {
  case -EINVAL:
        if (rv) {
		set_bit(CACHE_NEGATIVE, &h->flags);
		cache_fresh_locked(h, get_seconds() + CACHE_NEW_EXPIRY);
		rv = -ENOENT;
	}
	/* FALLTHROUGH */
  case -EAGAIN:
	cache_fresh_unlocked(h, detail);
  }

Though it isn't good that cache_fresh_locked() is being called without
holding a lock!  Maybe we should import try_to_negate_entry() from mainline.

> =========================================
> From: Bodo Stroesser <bstroesser@ts.fujitsu.com>
> Date: Fri, 08 Feb 2013
> Subject: [PATCH] net: sunrpc: fix races in RPC cache
> 
> This patch applies to SLES 11 SP2 linux-3.0.51-0.7.9 and also to
> vanilla linux-3.7.9
> 
> It is untested and is only based on a code review after we
> analyzed the reason for NFS requests being dropped on a
> SLES11 SP1 (linux-2.6.32.59-0.7.1)
> 
> Sporadically NFS3 RPC requests to the nfs server are dropped due to
> cache_check() (net/sunrpc/cache.c) returning -ETIMEDOUT for an entry
> of the "auth.unix.gid" cache.
> In this case, no NFS reply is sent to the client.
> 
> The reason for the dropped requests are races in cache_check() when
> cache_make_upcall() returns -EINVAL (because it is called for a cache
> without readers) and cache_check() therefore refreshes the cache entry 
> (rv == -EAGAIN).
> 
> There are two details that need to be changed:
>  1) cache_revisit_request() must not be called before cache_fresh_locked()
>     has updated the cache entry, as cache_revisit_request() wakes up
>     threads waiting for the cache entry to be updated.
>     The explicit call to cache_revisit_request() is not needed, as
>     cache_fresh_unlocked() calls it anyway.
>     (But in case of not updating the cache entry, cache_revisit_request()
>     must be called).
>  2) CACHE_PENDING must not be cleared before cache_fresh_locked() has
>     updated the cache entry, as cache_wait_req() called by
>     cache_defer_req() can return without really sleeping if it detects
>     CACHE_PENDING unset.
> 
> Signed-off-by: Bodo Stroesser <bstroesser@ts.fujitsu.com>
> ---
> 
> --- a/net/sunrpc/cache.c	2013-02-08 15:56:07.000000000 +0100
> +++ b/net/sunrpc/cache.c	2013-02-08 16:04:32.000000000 +0100
> @@ -230,11 +230,14 @@ static int try_to_negate_entry(struct ca
>  	rv = cache_is_valid(detail, h);
>  	if (rv != -EAGAIN) {
>  		write_unlock(&detail->hash_lock);
> +		clear_bit(CACHE_PENDING, &h->flags);
> +		cache_revisit_request(h);

This should just be cache_fresh_unlocked(), as below.
>  		return rv;
>  	}
>  	set_bit(CACHE_NEGATIVE, &h->flags);
>  	cache_fresh_locked(h, seconds_since_boot()+CACHE_NEW_EXPIRY);
>  	write_unlock(&detail->hash_lock);
> +	clear_bit(CACHE_PENDING, &h->flags);
Clearing this bit is wrong  - cache_frsh_unlocked will do that.

>  	cache_fresh_unlocked(h, detail);
>  	return -ENOENT;
>  }
So maybe:

static int try_to_negate_entry(....)
{
	int rv;

	write_lock(&detail->hash_lock);
	rv = cache_is_valid(detail, h);
	if (rv == -EAGAIN) {
		set_bit(CACHE_NEGATIVE, &h->flags);
		cache_fresh_locked(h, ....);
		rv = -ENOENT;
	}
	write_unlock(&detail->hash_lock);
	cache_fresh_unlocked(h, detail);
	return rv;
}
????


> @@ -275,8 +278,6 @@ int cache_check(struct cache_detail *det
>  		if (!test_and_set_bit(CACHE_PENDING, &h->flags)) {
>  			switch (cache_make_upcall(detail, h)) {
>  			case -EINVAL:
> -				clear_bit(CACHE_PENDING, &h->flags);
> -				cache_revisit_request(h);
>  				rv = try_to_negate_entry(detail, h);
>  				break;
>  			case -EAGAIN:

Yes, those lines should definitely be removed.

So maybe this against mainline:

diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
index 9afa439..7296644 100644
--- a/net/sunrpc/cache.c
+++ b/net/sunrpc/cache.c
@@ -228,12 +228,11 @@ static int try_to_negate_entry(struct cache_detail *detail, struct cache_head *h
 
 	write_lock(&detail->hash_lock);
 	rv = cache_is_valid(detail, h);
-	if (rv != -EAGAIN) {
-		write_unlock(&detail->hash_lock);
-		return rv;
+	if (rv == -EAGAIN) {
+		set_bit(CACHE_NEGATIVE, &h->flags);
+		cache_fresh_locked(h, seconds_since_boot()+CACHE_NEW_EXPIRY);
+		rv = -ENOENT;
 	}
-	set_bit(CACHE_NEGATIVE, &h->flags);
-	cache_fresh_locked(h, seconds_since_boot()+CACHE_NEW_EXPIRY);
 	write_unlock(&detail->hash_lock);
 	cache_fresh_unlocked(h, detail);
 	return -ENOENT;
@@ -275,13 +274,10 @@ int cache_check(struct cache_detail *detail,
 		if (!test_and_set_bit(CACHE_PENDING, &h->flags)) {
 			switch (cache_make_upcall(detail, h)) {
 			case -EINVAL:
-				clear_bit(CACHE_PENDING, &h->flags);
-				cache_revisit_request(h);
 				rv = try_to_negate_entry(detail, h);
 				break;
 			case -EAGAIN:
-				clear_bit(CACHE_PENDING, &h->flags);
-				cache_revisit_request(h);
+				cache_fresh_unlocked(h, detail);
 				break;
 			}
 		}


Is that convincing?

Thanks a lot for your very thorough analysis!

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] sunrpc.ko: RPC cache fix races

[J. Bruce Fields]

On Tue, Feb 19, 2013 at 06:08:40PM +0100, bstroesser@ts.fujitsu.com wrote:
> Second attempt using the correct FROM. Sorry for the noise.
> 
> 
> Hi,
> 
> I found a problem in sunrpc.ko on a SLES11 SP1 (2.6.32.59-0,7.1)
> and fixed it (see first patch ifor 2.6.32.60 below).
> For us the patch works fine (tested on 2.6.32.59-0.7.1).
> 
> AFAICS from the code, the problem seems to persist in current
> kernel versions also. Thus, I added the second patch for 3.7.9.
> As the setup to reproduce the problem is quite complex, I couldn't
> test the second patch yet. So consider this one as a RFC.
> 
> Best regards,
> Bodo
> 
> Please CC me, I'm not on the list.
> 
> =========================================
> From: Bodo Stroesser <bstroesser@ts.fujitsu.com>
> Date: Fri, 08 Feb 2013
> Subject: [PATCH] net: sunrpc: fix races in RPC cache
> 
> We found the problem and tested the patch on a SLES11 SP1 2.6.32.59-0.7.1
> 
> This patch applies to linux-2.6.32.60 (changed monotonic_seconds -->
> get_seconds())

This patch may well be correct, but for stable I can't normally take
patches that aren't backports of specific upstream fixes.  That means:
first, we need to fix whatever remains to be fixed on the latest
upstream kernel.  Then, we need to identify the other upstream patches
are needed and aren't yet in 2.6.32.60.

...
> =========================================
> From: Bodo Stroesser <bstroesser@ts.fujitsu.com>
> Date: Fri, 08 Feb 2013
> Subject: [PATCH] net: sunrpc: fix races in RPC cache
> 
> This patch applies to SLES 11 SP2 linux-3.0.51-0.7.9 and also to
> vanilla linux-3.7.9
> 
> It is untested and is only based on a code review after we
> analyzed the reason for NFS requests being dropped on a
> SLES11 SP1 (linux-2.6.32.59-0.7.1)
> 
> Sporadically NFS3 RPC requests to the nfs server are dropped due to
> cache_check() (net/sunrpc/cache.c) returning -ETIMEDOUT for an entry
> of the "auth.unix.gid" cache.
> In this case, no NFS reply is sent to the client.
> 
> The reason for the dropped requests are races in cache_check() when
> cache_make_upcall() returns -EINVAL (because it is called for a cache
> without readers) and cache_check() therefore refreshes the cache entry 
> (rv == -EAGAIN).
> 
> There are two details that need to be changed:
>  1) cache_revisit_request() must not be called before cache_fresh_locked()
>     has updated the cache entry, as cache_revisit_request() wakes up
>     threads waiting for the cache entry to be updated.
>     The explicit call to cache_revisit_request() is not needed, as
>     cache_fresh_unlocked() calls it anyway.
>     (But in case of not updating the cache entry, cache_revisit_request()
>     must be called).
>  2) CACHE_PENDING must not be cleared before cache_fresh_locked() has
>     updated the cache entry, as cache_wait_req() called by
>     cache_defer_req() can return without really sleeping if it detects
>     CACHE_PENDING unset.

I think that makes sense.  Thanks for the explanation.  This is all a
bit subtle, so any additional review or testing would be appreciated....

--b.

> 
> Signed-off-by: Bodo Stroesser <bstroesser@ts.fujitsu.com>
> ---
> 
> --- a/net/sunrpc/cache.c	2013-02-08 15:56:07.000000000 +0100
> +++ b/net/sunrpc/cache.c	2013-02-08 16:04:32.000000000 +0100
> @@ -230,11 +230,14 @@ static int try_to_negate_entry(struct ca
>  	rv = cache_is_valid(detail, h);
>  	if (rv != -EAGAIN) {
>  		write_unlock(&detail->hash_lock);
> +		clear_bit(CACHE_PENDING, &h->flags);
> +		cache_revisit_request(h);
>  		return rv;
>  	}
>  	set_bit(CACHE_NEGATIVE, &h->flags);
>  	cache_fresh_locked(h, seconds_since_boot()+CACHE_NEW_EXPIRY);
>  	write_unlock(&detail->hash_lock);
> +	clear_bit(CACHE_PENDING, &h->flags);
>  	cache_fresh_unlocked(h, detail);
>  	return -ENOENT;
>  }
> @@ -275,8 +278,6 @@ int cache_check(struct cache_detail *det
>  		if (!test_and_set_bit(CACHE_PENDING, &h->flags)) {
>  			switch (cache_make_upcall(detail, h)) {
>  			case -EINVAL:
> -				clear_bit(CACHE_PENDING, &h->flags);
> -				cache_revisit_request(h);
>  				rv = try_to_negate_entry(detail, h);
>  				break;
>  			case -EAGAIN: