NFS: init client before declaration

PATCH In review Stanislav Kinsbursky 4 participants 13 messages May 22, 2012 – May 23, 2012

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Stanislav Kinsbursky

author

2012-05-22 12:40 UTC #0 lore raw

PATCH 1/1 NFS: init client before declaration

Client have to be initialized prior to adding it to per-net clients list, because otherwise there are races, shown below:

CPU#0 CPU#1
_____ _____

nfs_get_client
nfs_alloc_client
list_add(..., nfs_client_list)

					rpc_fill_super
					rpc_pipefs_event
					nfs_get_client_for_event
					__rpc_pipefs_event
					(clp->cl_rpcclient is uninitialized)
					BUG()

init_client
clp->cl_rpcclient = ...

Signed-off-by Stanislav Kinsbursky <[email protected]>

---
 fs/nfs/client.c |   22 ++++++++++++----------
 1 files changed, 12 insertions(+), 10 deletions(-)

fs/nfs/client.c +12 ~~-10~~

index ae29d4f..9bf4702 100644
@@ -525,7 +525,7 @@ nfs_get_client(const struct nfs_client_initdata *cl_init,
 		cl_init->hostname ?: "", cl_init->rpc_ops->version);
 
 	/* see if the client already exists */
-	do {
+	while (1) {
 		spin_lock(&nn->nfs_client_lock);
 
 		clp = nfs_match_client(cl_init);

@@ -537,10 +537,18 @@ nfs_get_client(const struct nfs_client_initdata *cl_init,
 		spin_unlock(&nn->nfs_client_lock);
 
 		new = nfs_alloc_client(cl_init);
-	} while (!IS_ERR(new));
+		if (IS_ERR(new)) {
+			dprintk("--> nfs_get_client() = %ld [failed]\n", PTR_ERR(new));
+			return new;
+		}
 
-	dprintk("--> nfs_get_client() = %ld [failed]\n", PTR_ERR(new));
-	return new;
+		error = cl_init->rpc_ops->init_client(new, timeparms, ip_addr,
+						      authflavour, noresvport);
+		if (error < 0) {
+			nfs_put_client(new);
+			return ERR_PTR(error);
+		}
+	}
 
 	/* install a new client and return with it unready */
 install_client:

@@ -548,12 +556,6 @@ install_client:
 	list_add(&clp->cl_share_link, &nn->nfs_client_list);
 	spin_unlock(&nn->nfs_client_lock);
 
-	error = cl_init->rpc_ops->init_client(clp, timeparms, ip_addr,
-					      authflavour, noresvport);
-	if (error < 0) {
-		nfs_put_client(clp);
-		return ERR_PTR(error);
-	}
 	dprintk("--> nfs_get_client() = %p [new]\n", clp);
 	return clp;

Chuck Lever

2012-05-22 13:47 UTC #1 lore raw

Quoted 15 lines · Stanislav Kinsbursky

Client have to be initialized prior to adding it to per-net clients list, because otherwise there are races, shown below:
CPU#0 CPU#1
_____ _____
nfs_get_client
nfs_alloc_client
list_add(..., nfs_client_list)
					rpc_fill_super
					rpc_pipefs_event
					nfs_get_client_for_event
					__rpc_pipefs_event
					(clp->cl_rpcclient is uninitialized)
					BUG()

Quoted 5 lines

init_client
clp->cl_rpcclient = ...
Signed-off-by Stanislav Kinsbursky <[email protected]>

This patch collides pretty hard with the server trunking detection work. If you agree this needs to be fixed, the best thing we can do, I guess, is take this patch and drop patch 11, 12, and 13 from my recent patch set, and I'll try to rework for 3.6.

Quoted 57 lines

---
fs/nfs/client.c | 22 ++++++++++++----------

1 files changed, 12 insertions(+), 10 deletions(-)

fs/nfs/client.c +12 -4

index ae29d4f..9bf4702 100644
@@ -525,7 +525,7 @@ nfs_get_client(const struct nfs_client_initdata *cl_init,
 		cl_init->hostname ?: "", cl_init->rpc_ops->version);
 
 	/* see if the client already exists */
-	do {
+	while (1) {
 		spin_lock(&nn->nfs_client_lock);
 
 		clp = nfs_match_client(cl_init);

@@ -537,10 +537,18 @@ nfs_get_client(const struct nfs_client_initdata *cl_init,
 		spin_unlock(&nn->nfs_client_lock);
 
 		new = nfs_alloc_client(cl_init);
-	} while (!IS_ERR(new));
+		if (IS_ERR(new)) {
+			dprintk("--> nfs_get_client() = %ld [failed]\n", PTR_ERR(new));
+			return new;
+		}
 
-	dprintk("--> nfs_get_client() = %ld [failed]\n", PTR_ERR(new));
-	return new;
+		error = cl_init->rpc_ops->init_client(new, timeparms, ip_addr,
+						      authflavour, noresvport);
+		if (error < 0) {
+			nfs_put_client(new);
+			return ERR_PTR(error);
+		}
+	}
 
 	/* install a new client and return with it unready */

install_client:

fs/nfs/client.c +0 -7

@@ -548,12 +556,6 @@ install_client:
 	list_add(&clp->cl_share_link, &nn->nfs_client_list);
 	spin_unlock(&nn->nfs_client_lock);
 
-	error = cl_init->rpc_ops->init_client(clp, timeparms, ip_addr,
-					      authflavour, noresvport);
-	if (error < 0) {
-		nfs_put_client(clp);
-		return ERR_PTR(error);
-	}
 	dprintk("--> nfs_get_client() = %p [new]\n", clp);
 	return clp;
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Myklebust, Trond

2012-05-22 14:29 UTC #2 lore raw

In reply to Stanislav Kinsbursky

T24gVHVlLCAyMDEyLTA1LTIyIGF0IDE2OjQwICswNDAwLCBTdGFuaXNsYXYgS2luc2J1cnNreSB3 cm90ZToNCj4gQ2xpZW50IGhhdmUgdG8gYmUgaW5pdGlhbGl6ZWQgcHJpb3IgdG8gYWRkaW5nIGl0 IHRvIHBlci1uZXQgY2xpZW50cyBsaXN0LA0KPiBiZWNhdXNlIG90aGVyd2lzZSB0aGVyZSBhcmUg cmFjZXMsIHNob3duIGJlbG93Og0KPiANCj4gQ1BVIzAJCQkJCUNQVSMxDQo+IF9fX19fCQkJCQlf X19fXw0KPiANCj4gbmZzX2dldF9jbGllbnQNCj4gbmZzX2FsbG9jX2NsaWVudA0KPiBsaXN0X2Fk ZCguLi4sIG5mc19jbGllbnRfbGlzdCkNCj4gCQkJCQlycGNfZmlsbF9zdXBlcg0KPiAJCQkJCXJw Y19waXBlZnNfZXZlbnQNCj4gCQkJCQluZnNfZ2V0X2NsaWVudF9mb3JfZXZlbnQNCj4gCQkJCQlf X3JwY19waXBlZnNfZXZlbnQNCj4gCQkJCQkoY2xwLT5jbF9ycGNjbGllbnQgaXMgdW5pbml0aWFs aXplZCkNCj4gCQkJCQlCVUcoKQ0KPiBpbml0X2NsaWVudA0KPiBjbHAtPmNsX3JwY2NsaWVudCA9 IC4uLg0KPiANCg0KV2h5IG5vdCBzaW1wbHkgY2hhbmdlIG5mc19nZXRfY2xpZW50X2Zvcl9ldmVu dCgpIHNvIHRoYXQgaXQgZG9lc24ndA0KdG91Y2ggbmZzX2NsaWVudHMgdGhhdCBoYXZlIGNscC0+ Y2xfY29uc19zdGF0ZSE9TkZTX0NTX1JFQURZPw0KDQpUaGF0IHNob3VsZCBlbnN1cmUgdGhhdCBp dCBkb2Vzbid0IHRvdWNoIG5mc19jbGllbnRzIHRoYXQgZmFpbGVkIHRvDQppbml0aWFsaXNlIGFu ZC9vciBhcmUgc3RpbGwgaW4gdGhlIHByb2Nlc3Mgb2YgYmVpbmcgaW5pdGlhbGlzZWQuDQoNCkNo ZWVycw0KICBUcm9uZA0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBt YWludGFpbmVyDQoNCk5ldEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRh cHAuY29tDQoNCg==

Myklebust, Trond

2012-05-22 15:00 UTC #3 lore raw

T24gVHVlLCAyMDEyLTA1LTIyIGF0IDEwOjI5IC0wNDAwLCBUcm9uZCBNeWtsZWJ1c3Qgd3JvdGU6 DQo+IE9uIFR1ZSwgMjAxMi0wNS0yMiBhdCAxNjo0MCArMDQwMCwgU3RhbmlzbGF2IEtpbnNidXJz a3kgd3JvdGU6DQo+ID4gQ2xpZW50IGhhdmUgdG8gYmUgaW5pdGlhbGl6ZWQgcHJpb3IgdG8gYWRk aW5nIGl0IHRvIHBlci1uZXQgY2xpZW50cyBsaXN0LA0KPiA+IGJlY2F1c2Ugb3RoZXJ3aXNlIHRo ZXJlIGFyZSByYWNlcywgc2hvd24gYmVsb3c6DQo+ID4gDQo+ID4gQ1BVIzAJCQkJCUNQVSMxDQo+ ID4gX19fX18JCQkJCV9fX19fDQo+ID4gDQo+ID4gbmZzX2dldF9jbGllbnQNCj4gPiBuZnNfYWxs b2NfY2xpZW50DQo+ID4gbGlzdF9hZGQoLi4uLCBuZnNfY2xpZW50X2xpc3QpDQo+ID4gCQkJCQly cGNfZmlsbF9zdXBlcg0KPiA+IAkJCQkJcnBjX3BpcGVmc19ldmVudA0KPiA+IAkJCQkJbmZzX2dl dF9jbGllbnRfZm9yX2V2ZW50DQo+ID4gCQkJCQlfX3JwY19waXBlZnNfZXZlbnQNCj4gPiAJCQkJ CShjbHAtPmNsX3JwY2NsaWVudCBpcyB1bmluaXRpYWxpemVkKQ0KPiA+IAkJCQkJQlVHKCkNCj4g PiBpbml0X2NsaWVudA0KPiA+IGNscC0+Y2xfcnBjY2xpZW50ID0gLi4uDQo+ID4gDQo+IA0KPiBX aHkgbm90IHNpbXBseSBjaGFuZ2UgbmZzX2dldF9jbGllbnRfZm9yX2V2ZW50KCkgc28gdGhhdCBp dCBkb2Vzbid0DQo+IHRvdWNoIG5mc19jbGllbnRzIHRoYXQgaGF2ZSBjbHAtPmNsX2NvbnNfc3Rh dGUhPU5GU19DU19SRUFEWT8NCj4gDQo+IFRoYXQgc2hvdWxkIGVuc3VyZSB0aGF0IGl0IGRvZXNu J3QgdG91Y2ggbmZzX2NsaWVudHMgdGhhdCBmYWlsZWQgdG8NCj4gaW5pdGlhbGlzZSBhbmQvb3Ig YXJlIHN0aWxsIGluIHRoZSBwcm9jZXNzIG9mIGJlaW5nIGluaXRpYWxpc2VkLg0KDQouLi5hY3R1 YWxseSwgY29tZSB0byB0aGluayBvZiBpdC4gV2h5IG5vdCBqdXN0IGFkZCBhIGhlbHBlciBmdW5j dGlvbg0KImJvb2wgbmZzX2NsaWVudF9hY3RpdmUoY29uc3Qgc3RydWN0IG5mc19jbGllbnQgKmNs cCkiIHRvDQpmcy9uZnMvY2xpZW50LmMgdGhhdCBkb2VzIGEgY2FsbCB0bw0KCXdhaXRfZXZlbnRf a2lsbGFibGUobmZzX2NsaWVudF9hY3RpdmVfd3EsIGNscC0+Y2xfY29uc19zdGF0ZSA8IE5GU19D U19JTklUSU5HKTsNCmFuZCBjaGVja3MgdGhlIHJlc3VsdGluZyB2YWx1ZSBvZiBjbHAtPmNsX2Nv bnNfc3RhdGU/DQoNCi0tIA0KVHJvbmQgTXlrbGVidXN0DQpMaW51eCBORlMgY2xpZW50IG1haW50 YWluZXINCg0KTmV0QXBwDQpUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbQ0Kd3d3Lm5ldGFwcC5j b20NCg0K

Stanislav Kinsbursky

author

2012-05-22 15:29 UTC #4 lore raw

Quoted 24 lines · Trond

Quoted 23 lines · Trond Myklebust
Quoted 9 lines · Stanislav Kinsbursky
Client have to be initialized prior to adding it to per-net clients list, because otherwise there are races, shown below:
CPU#0 CPU#1
_____ _____
nfs_get_client
nfs_alloc_client
list_add(..., nfs_client_list)
Quoted 9 lines
					rpc_fill_super
					rpc_pipefs_event
					nfs_get_client_for_event
					__rpc_pipefs_event
					(clp->cl_rpcclient is uninitialized)
					BUG()
init_client
clp->cl_rpcclient = ...
Why not simply change nfs_get_client_for_event() so that it doesn't touch nfs_clients that have clp->cl_cons_state!=NFS_CS_READY?

Quoted 9 lines

Quoted 2 lines
That should ensure that it doesn't touch nfs_clients that failed to initialise and/or are still in the process of being initialised.
...actually, come to think of it. Why not just add a helper function "bool nfs_client_active(const struct nfs_client *clp)" to fs/nfs/client.c that does a call to
	wait_event_killable(nfs_client_active_wq, clp->cl_cons_state<  NFS_CS_INITING);
and checks the resulting value of clp->cl_cons_state?

Sorry, but I don't understand the idea... Where are you proposing to call this function? In __rpc_pipefs_event() prior to dentries creatios?

Signature · 2 lines

Best regards,
Stanislav Kinsbursky

Myklebust, Trond

2012-05-22 15:51 UTC #5 lore raw

T24gVHVlLCAyMDEyLTA1LTIyIGF0IDE5OjI5ICswNDAwLCBTdGFuaXNsYXYgS2luc2J1cnNreSB3 cm90ZToNCj4gT24gMjIuMDUuMjAxMiAxOTowMCwgTXlrbGVidXN0LCBUcm9uZCB3cm90ZToNCj4g PiBPbiBUdWUsIDIwMTItMDUtMjIgYXQgMTA6MjkgLTA0MDAsIFRyb25kIE15a2xlYnVzdCB3cm90 ZToNCj4gPj4gT24gVHVlLCAyMDEyLTA1LTIyIGF0IDE2OjQwICswNDAwLCBTdGFuaXNsYXYgS2lu c2J1cnNreSB3cm90ZToNCj4gPj4+IENsaWVudCBoYXZlIHRvIGJlIGluaXRpYWxpemVkIHByaW9y IHRvIGFkZGluZyBpdCB0byBwZXItbmV0IGNsaWVudHMgbGlzdCwNCj4gPj4+IGJlY2F1c2Ugb3Ro ZXJ3aXNlIHRoZXJlIGFyZSByYWNlcywgc2hvd24gYmVsb3c6DQo+ID4+Pg0KPiA+Pj4gQ1BVIzAJ CQkJCUNQVSMxDQo+ID4+PiBfX19fXwkJCQkJX19fX18NCj4gPj4+DQo+ID4+PiBuZnNfZ2V0X2Ns aWVudA0KPiA+Pj4gbmZzX2FsbG9jX2NsaWVudA0KPiA+Pj4gbGlzdF9hZGQoLi4uLCBuZnNfY2xp ZW50X2xpc3QpDQo+ID4+PiAJCQkJCXJwY19maWxsX3N1cGVyDQo+ID4+PiAJCQkJCXJwY19waXBl ZnNfZXZlbnQNCj4gPj4+IAkJCQkJbmZzX2dldF9jbGllbnRfZm9yX2V2ZW50DQo+ID4+PiAJCQkJ CV9fcnBjX3BpcGVmc19ldmVudA0KPiA+Pj4gCQkJCQkoY2xwLT5jbF9ycGNjbGllbnQgaXMgdW5p bml0aWFsaXplZCkNCj4gPj4+IAkJCQkJQlVHKCkNCj4gPj4+IGluaXRfY2xpZW50DQo+ID4+PiBj bHAtPmNsX3JwY2NsaWVudCA9IC4uLg0KPiA+Pj4NCj4gPj4NCj4gPj4gV2h5IG5vdCBzaW1wbHkg Y2hhbmdlIG5mc19nZXRfY2xpZW50X2Zvcl9ldmVudCgpIHNvIHRoYXQgaXQgZG9lc24ndA0KPiA+ PiB0b3VjaCBuZnNfY2xpZW50cyB0aGF0IGhhdmUgY2xwLT5jbF9jb25zX3N0YXRlIT1ORlNfQ1Nf UkVBRFk/DQo+ID4+DQo+ID4+IFRoYXQgc2hvdWxkIGVuc3VyZSB0aGF0IGl0IGRvZXNuJ3QgdG91 Y2ggbmZzX2NsaWVudHMgdGhhdCBmYWlsZWQgdG8NCj4gPj4gaW5pdGlhbGlzZSBhbmQvb3IgYXJl IHN0aWxsIGluIHRoZSBwcm9jZXNzIG9mIGJlaW5nIGluaXRpYWxpc2VkLg0KPiA+DQo+ID4gLi4u YWN0dWFsbHksIGNvbWUgdG8gdGhpbmsgb2YgaXQuIFdoeSBub3QganVzdCBhZGQgYSBoZWxwZXIg ZnVuY3Rpb24NCj4gPiAiYm9vbCBuZnNfY2xpZW50X2FjdGl2ZShjb25zdCBzdHJ1Y3QgbmZzX2Ns aWVudCAqY2xwKSIgdG8NCj4gPiBmcy9uZnMvY2xpZW50LmMgdGhhdCBkb2VzIGEgY2FsbCB0bw0K PiA+IAl3YWl0X2V2ZW50X2tpbGxhYmxlKG5mc19jbGllbnRfYWN0aXZlX3dxLCBjbHAtPmNsX2Nv bnNfc3RhdGU8ICBORlNfQ1NfSU5JVElORyk7DQo+ID4gYW5kIGNoZWNrcyB0aGUgcmVzdWx0aW5n IHZhbHVlIG9mIGNscC0+Y2xfY29uc19zdGF0ZT8NCj4gPg0KPiANCj4gU29ycnksIGJ1dCBJIGRv bid0IHVuZGVyc3RhbmQgdGhlIGlkZWEuLi4NCj4gV2hlcmUgYXJlIHlvdSBwcm9wb3NpbmcgdG8g Y2FsbCB0aGlzIGZ1bmN0aW9uPw0KPiBJbiBfX3JwY19waXBlZnNfZXZlbnQoKSBwcmlvciB0byBk ZW50cmllcyBjcmVhdGlvcz8NCg0KU2VlIGJlbG93Og0KDQo4PC0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0NCkZyb20gZjViOTBkZjYzODFhMjAzOTVkOWY4OGExOTllOWU1MmY0NDI2NzQ1NyBNb24g U2VwIDE3IDAwOjAwOjAwIDIwMDENCkZyb206IFRyb25kIE15a2xlYnVzdCA8VHJvbmQuTXlrbGVi dXN0QG5ldGFwcC5jb20+DQpEYXRlOiBUdWUsIDIyIE1heSAyMDEyIDExOjQ5OjU1IC0wNDAwDQpT dWJqZWN0OiBbUEFUQ0hdIE5GU3Y0OiBGaXggYSByYWNlIGluIHRoZSBuZXQgbmFtZXNwYWNlIG1v dW50IG5vdGlmaWNhdGlvbg0KDQpTaW5jZSB0aGUgc3RydWN0IG5mc19jbGllbnQgZ2V0cyBhZGRl ZCB0byB0aGUgZ2xvYmFsIG5mc19jbGllbnRfbGlzdA0KYmVmb3JlIGl0IGlzIGluaXRpYWxpc2Vk LCBpdCBpcyBwb3NzaWJsZSB0aGF0IHJwY19waXBlZnNfZXZlbnQgY2FuDQplbmQgdXAgdHJ5aW5n IHRvIGNyZWF0ZSBpZG1hcHBlciBlbnRyaWVzIGZvciBzdWNoIGEgdGhpbmcuDQoNClRoZSBzb2x1 dGlvbiBpcyB0byBoYXZlIHRoZSBtb3VudCBub3RpZmljYXRpb24gd2FpdCBmb3IgdGhlDQpuZnNf Y2xpZW50IGluaXRpYWxpc2F0aW9uIHRvIGNvbXBsZXRlLg0KDQpSZXBvcnRlZC1ieTogU3Rhbmlz bGF2IEtpbnNidXJza3kgPHNraW5zYnVyc2t5QHBhcmFsbGVscy5jb20+DQpTaWduZWQtb2ZmLWJ5 OiBUcm9uZCBNeWtsZWJ1c3QgPFRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tPg0KLS0tDQogZnMv bmZzL2NsaWVudC5jICAgfCAgIDE0ICsrKysrKysrKysrKysrDQogZnMvbmZzL2lkbWFwLmMgICAg fCAgICAzICsrLQ0KIGZzL25mcy9pbnRlcm5hbC5oIHwgICAgMSArDQogMyBmaWxlcyBjaGFuZ2Vk LCAxNyBpbnNlcnRpb25zKCspLCAxIGRlbGV0aW9ucygtKQ0KDQpkaWZmIC0tZ2l0IGEvZnMvbmZz L2NsaWVudC5jIGIvZnMvbmZzL2NsaWVudC5jDQppbmRleCA2MGY3ZTRlLi4zZmE0NGVmIDEwMDY0 NA0KLS0tIGEvZnMvbmZzL2NsaWVudC5jDQorKysgYi9mcy9uZnMvY2xpZW50LmMNCkBAIC01OTIs NiArNTkyLDIwIEBAIHZvaWQgbmZzX21hcmtfY2xpZW50X3JlYWR5KHN0cnVjdCBuZnNfY2xpZW50 ICpjbHAsIGludCBzdGF0ZSkNCiAJd2FrZV91cF9hbGwoJm5mc19jbGllbnRfYWN0aXZlX3dxKTsN CiB9DQogDQorc3RhdGljIGJvb2wgbmZzX2NsaWVudF9yZWFkeShzdHJ1Y3QgbmZzX2NsaWVudCAq Y2xwKQ0KK3sNCisJcmV0dXJuIGNscC0+Y2xfY29uc19zdGF0ZSA8PSBORlNfQ1NfUkVBRFk7DQor fQ0KKw0KK2ludCBuZnNfd2FpdF9jbGllbnRfcmVhZHkoc3RydWN0IG5mc19jbGllbnQgKmNscCkN Cit7DQorCWlmICh3YWl0X2V2ZW50X2tpbGxhYmxlKG5mc19jbGllbnRfYWN0aXZlX3dxLCBuZnNf Y2xpZW50X3JlYWR5KGNscCkpIDwgMCkNCisJCXJldHVybiAtRVJFU1RBUlRTWVM7DQorCWlmIChj bHAtPmNsX2NvbnNfc3RhdGUgPCAwKQ0KKwkJcmV0dXJuIGNscC0+Y2xfY29uc19zdGF0ZTsNCisJ cmV0dXJuIDA7DQorfQ0KKw0KIC8qDQogICogV2l0aCBzZXNzaW9ucywgdGhlIGNsaWVudCBpcyBu b3QgbWFya2VkIHJlYWR5IHVudGlsIGFmdGVyIGENCiAgKiBzdWNjZXNzZnVsIEVYQ0hBTkdFX0lE IGFuZCBDUkVBVEVfU0VTU0lPTi4NCmRpZmYgLS1naXQgYS9mcy9uZnMvaWRtYXAuYyBiL2ZzL25m cy9pZG1hcC5jDQppbmRleCAzZThlZGJlLi42Nzk2MmM4IDEwMDY0NA0KLS0tIGEvZnMvbmZzL2lk bWFwLmMNCisrKyBiL2ZzL25mcy9pZG1hcC5jDQpAQCAtNTU4LDcgKzU1OCw4IEBAIHN0YXRpYyBp bnQgcnBjX3BpcGVmc19ldmVudChzdHJ1Y3Qgbm90aWZpZXJfYmxvY2sgKm5iLCB1bnNpZ25lZCBs b25nIGV2ZW50LA0KIAkJcmV0dXJuIDA7DQogDQogCXdoaWxlICgoY2xwID0gbmZzX2dldF9jbGll bnRfZm9yX2V2ZW50KHNiLT5zX2ZzX2luZm8sIGV2ZW50KSkpIHsNCi0JCWVycm9yID0gX19ycGNf cGlwZWZzX2V2ZW50KGNscCwgZXZlbnQsIHNiKTsNCisJCWlmIChuZnNfd2FpdF9jbGllbnRfcmVh ZHkoY2xwKSA9PSAwKQ0KKwkJCWVycm9yID0gX19ycGNfcGlwZWZzX2V2ZW50KGNscCwgZXZlbnQs IHNiKTsNCiAJCW5mc19wdXRfY2xpZW50KGNscCk7DQogCQlpZiAoZXJyb3IpDQogCQkJYnJlYWs7 DQpkaWZmIC0tZ2l0IGEvZnMvbmZzL2ludGVybmFsLmggYi9mcy9uZnMvaW50ZXJuYWwuaA0KaW5k ZXggYjc3N2JkYS4uM2JlMDBhMCAxMDA2NDQNCi0tLSBhL2ZzL25mcy9pbnRlcm5hbC5oDQorKysg Yi9mcy9uZnMvaW50ZXJuYWwuaA0KQEAgLTE2OCw2ICsxNjgsNyBAQCBleHRlcm4gc3RydWN0IG5m c19zZXJ2ZXIgKm5mc19jbG9uZV9zZXJ2ZXIoc3RydWN0IG5mc19zZXJ2ZXIgKiwNCiAJCQkJCSAg IHN0cnVjdCBuZnNfZmF0dHIgKiwNCiAJCQkJCSAgIHJwY19hdXRoZmxhdm9yX3QpOw0KIGV4dGVy biB2b2lkIG5mc19tYXJrX2NsaWVudF9yZWFkeShzdHJ1Y3QgbmZzX2NsaWVudCAqY2xwLCBpbnQg c3RhdGUpOw0KK2V4dGVybiBpbnQgbmZzX3dhaXRfY2xpZW50X3JlYWR5KHN0cnVjdCBuZnNfY2xp ZW50ICpjbHApOw0KIGV4dGVybiBpbnQgbmZzNF9jaGVja19jbGllbnRfcmVhZHkoc3RydWN0IG5m c19jbGllbnQgKmNscCk7DQogZXh0ZXJuIHN0cnVjdCBuZnNfY2xpZW50ICpuZnM0X3NldF9kc19j bGllbnQoc3RydWN0IG5mc19jbGllbnQqIG1kc19jbHAsDQogCQkJCQkgICAgIGNvbnN0IHN0cnVj dCBzb2NrYWRkciAqZHNfYWRkciwNCi0tIA0KMS43LjcuNg0KDQoNCi0tIA0KVHJvbmQgTXlrbGVi dXN0DQpMaW51eCBORlMgY2xpZW50IG1haW50YWluZXINCg0KTmV0QXBwDQpUcm9uZC5NeWtsZWJ1 c3RAbmV0YXBwLmNvbQ0Kd3d3Lm5ldGFwcC5jb20NCg0K

Stanislav Kinsbursky

author

2012-05-22 16:18 UTC #6 lore raw

Quoted 70 lines · Trond

Quoted 35 lines · Stanislav Kinsbursky
Quoted 24 lines · Trond
Quoted 23 lines · Trond Myklebust
Quoted 9 lines · Stanislav Kinsbursky
Client have to be initialized prior to adding it to per-net clients list, because otherwise there are races, shown below:
CPU#0 CPU#1
_____ _____
nfs_get_client
nfs_alloc_client
list_add(..., nfs_client_list)
Quoted 9 lines
					rpc_fill_super
					rpc_pipefs_event
					nfs_get_client_for_event
					__rpc_pipefs_event
					(clp->cl_rpcclient is uninitialized)
					BUG()
init_client
clp->cl_rpcclient = ...
Why not simply change nfs_get_client_for_event() so that it doesn't touch nfs_clients that have clp->cl_cons_state!=NFS_CS_READY?
Quoted 9 lines
Quoted 2 lines
That should ensure that it doesn't touch nfs_clients that failed to initialise and/or are still in the process of being initialised.
...actually, come to think of it. Why not just add a helper function "bool nfs_client_active(const struct nfs_client *clp)" to fs/nfs/client.c that does a call to
	wait_event_killable(nfs_client_active_wq, clp->cl_cons_state<   NFS_CS_INITING);
and checks the resulting value of clp->cl_cons_state?
Quoted 3 lines
Sorry, but I don't understand the idea... Where are you proposing to call this function? In __rpc_pipefs_event() prior to dentries creatios?
See below:
8<----------------------------------------------------------------------------------
From f5b90df6381a20395d9f88a199e9e52f44267457 Mon Sep 17 00:00:00 2001
From: Trond Myklebust<[email protected]>
Date: Tue, 22 May 2012 11:49:55 -0400
Subject: [PATCH] NFSv4: Fix a race in the net namespace mount notification
Since the struct nfs_client gets added to the global nfs_client_list before it is initialised, it is possible that rpc_pipefs_event can end up trying to create idmapper entries for such a thing.
The solution is to have the mount notification wait for the nfs_client initialisation to complete.
Reported-by Stanislav Kinsbursky<[email protected]>
Signed-off-by Trond Myklebust<[email protected]>
---
  fs/nfs/client.c   |   14 ++++++++++++++
  fs/nfs/idmap.c    |    3 ++-
  fs/nfs/internal.h |    1 +
  3 files changed, 17 insertions(+), 1 deletions(-)
fs/nfs/client.c +0 -0
index 60f7e4e..3fa44ef 100644
@@ -592,6 +592,20 @@ void nfs_mark_client_ready(struct nfs_client *clp, int state)
  	wake_up_all(&nfs_client_active_wq);
  }

fs/nfs/client.c void nfs_mark_client_ready(struct nfs_client *clp, int state) · line 603

Quoted 10 lines

fs/nfs/client.c +9 -0

 
+static bool nfs_client_ready(struct nfs_client *clp)
+{
+	return clp->cl_cons_state<= NFS_CS_READY;
+}
+
+int nfs_wait_client_ready(struct nfs_client *clp)
+{
+	if (wait_event_killable(nfs_client_active_wq, nfs_client_ready(clp))<  0)
+		return -ERESTARTSYS;

Ok, I see...
BTW, caller of this function is pipefs mount operation call... And when this mount call waits for NFS clients - it look a bit odd to me...

Quoted 13 lines

fs/nfs/client.c +5 -0

+	if (clp->cl_cons_state<  0)
+		return clp->cl_cons_state;
+	return 0;
+}
+
  /*
   * With sessions, the client is not marked ready until after a
   * successful EXCHANGE_ID and CREATE_SESSION.

fs/nfs/idmap.c +0 -0

index 3e8edbe..67962c8 100644

fs/nfs/idmap.c static int rpc_pipefs_event(struct notifier_block *nb, unsigned long event, · line 562

Quoted 6 lines

fs/nfs/idmap.c +2 -1

@@ -558,7 +558,8 @@ static int rpc_pipefs_event(struct notifier_block *nb, unsigned long event,
  		return 0;
 
  	while ((clp = nfs_get_client_for_event(sb->s_fs_info, event))) {
-		error = __rpc_pipefs_event(clp, event, sb);
+		if (nfs_wait_client_ready(clp) == 0)
+			error = __rpc_pipefs_event(clp, event, sb);

We have another problem here.
nfs4_init_client() will try to create pipe dentries prior to set of NFS_CS_READY to the client. And dentries will be created since semaphore is dropped and per-net superblock variable is initialized already. But __rpc_pipefs_event() relays on the fact, that no dentries present.
Looks like the problem was introduced by me in aad9487c...
So maybe we should not call "continue" instead "__rpc_pipefs_event()", when client becomes ready?
Looks like this will allow us to handle such races.

Quoted 15 lines

fs/nfs/idmap.c +0 -0

  		nfs_put_client(clp);
  		if (error)
  			break;

fs/nfs/internal.h +1 -0

index b777bda..3be00a0 100644
@@ -168,6 +168,7 @@ extern struct nfs_server *nfs_clone_server(struct nfs_server *,
  					   struct nfs_fattr *,
  					   rpc_authflavor_t);
  extern void nfs_mark_client_ready(struct nfs_client *clp, int state);
+extern int nfs_wait_client_ready(struct nfs_client *clp);
  extern int nfs4_check_client_ready(struct nfs_client *clp);
  extern struct nfs_client *nfs4_set_ds_client(struct nfs_client* mds_clp,
  					     const struct sockaddr *ds_addr,

Signature · 2 lines

Best regards,
Stanislav Kinsbursky

Myklebust, Trond

2012-05-22 16:43 UTC #7 lore raw

T24gVHVlLCAyMDEyLTA1LTIyIGF0IDIwOjE4ICswNDAwLCBTdGFuaXNsYXYgS2luc2J1cnNreSB3 cm90ZToNCj4gT24gMjIuMDUuMjAxMiAxOTo1MSwgTXlrbGVidXN0LCBUcm9uZCB3cm90ZToNCj4g PiBPbiBUdWUsIDIwMTItMDUtMjIgYXQgMTk6MjkgKzA0MDAsIFN0YW5pc2xhdiBLaW5zYnVyc2t5 IHdyb3RlOg0KPiA+PiBPbiAyMi4wNS4yMDEyIDE5OjAwLCBNeWtsZWJ1c3QsIFRyb25kIHdyb3Rl Og0KPiA+Pj4gT24gVHVlLCAyMDEyLTA1LTIyIGF0IDEwOjI5IC0wNDAwLCBUcm9uZCBNeWtsZWJ1 c3Qgd3JvdGU6DQo+ID4+Pj4gT24gVHVlLCAyMDEyLTA1LTIyIGF0IDE2OjQwICswNDAwLCBTdGFu aXNsYXYgS2luc2J1cnNreSB3cm90ZToNCj4gPj4+Pj4gQ2xpZW50IGhhdmUgdG8gYmUgaW5pdGlh bGl6ZWQgcHJpb3IgdG8gYWRkaW5nIGl0IHRvIHBlci1uZXQgY2xpZW50cyBsaXN0LA0KPiA+Pj4+ PiBiZWNhdXNlIG90aGVyd2lzZSB0aGVyZSBhcmUgcmFjZXMsIHNob3duIGJlbG93Og0KPiA+Pj4+ Pg0KPiA+Pj4+PiBDUFUjMAkJCQkJQ1BVIzENCj4gPj4+Pj4gX19fX18JCQkJCV9fX19fDQo+ID4+ Pj4+DQo+ID4+Pj4+IG5mc19nZXRfY2xpZW50DQo+ID4+Pj4+IG5mc19hbGxvY19jbGllbnQNCj4g Pj4+Pj4gbGlzdF9hZGQoLi4uLCBuZnNfY2xpZW50X2xpc3QpDQo+ID4+Pj4+IAkJCQkJcnBjX2Zp bGxfc3VwZXINCj4gPj4+Pj4gCQkJCQlycGNfcGlwZWZzX2V2ZW50DQo+ID4+Pj4+IAkJCQkJbmZz X2dldF9jbGllbnRfZm9yX2V2ZW50DQo+ID4+Pj4+IAkJCQkJX19ycGNfcGlwZWZzX2V2ZW50DQo+ ID4+Pj4+IAkJCQkJKGNscC0+Y2xfcnBjY2xpZW50IGlzIHVuaW5pdGlhbGl6ZWQpDQo+ID4+Pj4+ IAkJCQkJQlVHKCkNCj4gPj4+Pj4gaW5pdF9jbGllbnQNCj4gPj4+Pj4gY2xwLT5jbF9ycGNjbGll bnQgPSAuLi4NCj4gPj4+Pj4NCj4gPj4+Pg0KPiA+Pj4+IFdoeSBub3Qgc2ltcGx5IGNoYW5nZSBu ZnNfZ2V0X2NsaWVudF9mb3JfZXZlbnQoKSBzbyB0aGF0IGl0IGRvZXNuJ3QNCj4gPj4+PiB0b3Vj aCBuZnNfY2xpZW50cyB0aGF0IGhhdmUgY2xwLT5jbF9jb25zX3N0YXRlIT1ORlNfQ1NfUkVBRFk/ DQo+ID4+Pj4NCj4gPj4+PiBUaGF0IHNob3VsZCBlbnN1cmUgdGhhdCBpdCBkb2Vzbid0IHRvdWNo IG5mc19jbGllbnRzIHRoYXQgZmFpbGVkIHRvDQo+ID4+Pj4gaW5pdGlhbGlzZSBhbmQvb3IgYXJl IHN0aWxsIGluIHRoZSBwcm9jZXNzIG9mIGJlaW5nIGluaXRpYWxpc2VkLg0KPiA+Pj4NCj4gPj4+ IC4uLmFjdHVhbGx5LCBjb21lIHRvIHRoaW5rIG9mIGl0LiBXaHkgbm90IGp1c3QgYWRkIGEgaGVs cGVyIGZ1bmN0aW9uDQo+ID4+PiAiYm9vbCBuZnNfY2xpZW50X2FjdGl2ZShjb25zdCBzdHJ1Y3Qg bmZzX2NsaWVudCAqY2xwKSIgdG8NCj4gPj4+IGZzL25mcy9jbGllbnQuYyB0aGF0IGRvZXMgYSBj YWxsIHRvDQo+ID4+PiAJd2FpdF9ldmVudF9raWxsYWJsZShuZnNfY2xpZW50X2FjdGl2ZV93cSwg Y2xwLT5jbF9jb25zX3N0YXRlPCAgIE5GU19DU19JTklUSU5HKTsNCj4gPj4+IGFuZCBjaGVja3Mg dGhlIHJlc3VsdGluZyB2YWx1ZSBvZiBjbHAtPmNsX2NvbnNfc3RhdGU/DQo+ID4+Pg0KPiA+Pg0K PiA+PiBTb3JyeSwgYnV0IEkgZG9uJ3QgdW5kZXJzdGFuZCB0aGUgaWRlYS4uLg0KPiA+PiBXaGVy ZSBhcmUgeW91IHByb3Bvc2luZyB0byBjYWxsIHRoaXMgZnVuY3Rpb24/DQo+ID4+IEluIF9fcnBj X3BpcGVmc19ldmVudCgpIHByaW9yIHRvIGRlbnRyaWVzIGNyZWF0aW9zPw0KPiA+DQo+ID4gU2Vl IGJlbG93Og0KPiA+DQo+ID4gODwtLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tDQo+ID4gIEZyb20g ZjViOTBkZjYzODFhMjAzOTVkOWY4OGExOTllOWU1MmY0NDI2NzQ1NyBNb24gU2VwIDE3IDAwOjAw OjAwIDIwMDENCj4gPiBGcm9tOiBUcm9uZCBNeWtsZWJ1c3Q8VHJvbmQuTXlrbGVidXN0QG5ldGFw cC5jb20+DQo+ID4gRGF0ZTogVHVlLCAyMiBNYXkgMjAxMiAxMTo0OTo1NSAtMDQwMA0KPiA+IFN1 YmplY3Q6IFtQQVRDSF0gTkZTdjQ6IEZpeCBhIHJhY2UgaW4gdGhlIG5ldCBuYW1lc3BhY2UgbW91 bnQgbm90aWZpY2F0aW9uDQo+ID4NCj4gPiBTaW5jZSB0aGUgc3RydWN0IG5mc19jbGllbnQgZ2V0 cyBhZGRlZCB0byB0aGUgZ2xvYmFsIG5mc19jbGllbnRfbGlzdA0KPiA+IGJlZm9yZSBpdCBpcyBp bml0aWFsaXNlZCwgaXQgaXMgcG9zc2libGUgdGhhdCBycGNfcGlwZWZzX2V2ZW50IGNhbg0KPiA+ IGVuZCB1cCB0cnlpbmcgdG8gY3JlYXRlIGlkbWFwcGVyIGVudHJpZXMgZm9yIHN1Y2ggYSB0aGlu Zy4NCj4gPg0KPiA+IFRoZSBzb2x1dGlvbiBpcyB0byBoYXZlIHRoZSBtb3VudCBub3RpZmljYXRp b24gd2FpdCBmb3IgdGhlDQo+ID4gbmZzX2NsaWVudCBpbml0aWFsaXNhdGlvbiB0byBjb21wbGV0 ZS4NCj4gPg0KPiA+IFJlcG9ydGVkLWJ5OiBTdGFuaXNsYXYgS2luc2J1cnNreTxza2luc2J1cnNr eUBwYXJhbGxlbHMuY29tPg0KPiA+IFNpZ25lZC1vZmYtYnk6IFRyb25kIE15a2xlYnVzdDxUcm9u ZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbT4NCj4gPiAtLS0NCj4gPiAgIGZzL25mcy9jbGllbnQuYyAg IHwgICAxNCArKysrKysrKysrKysrKw0KPiA+ICAgZnMvbmZzL2lkbWFwLmMgICAgfCAgICAzICsr LQ0KPiA+ICAgZnMvbmZzL2ludGVybmFsLmggfCAgICAxICsNCj4gPiAgIDMgZmlsZXMgY2hhbmdl ZCwgMTcgaW5zZXJ0aW9ucygrKSwgMSBkZWxldGlvbnMoLSkNCj4gPg0KPiA+IGRpZmYgLS1naXQg YS9mcy9uZnMvY2xpZW50LmMgYi9mcy9uZnMvY2xpZW50LmMNCj4gPiBpbmRleCA2MGY3ZTRlLi4z ZmE0NGVmIDEwMDY0NA0KPiA+IC0tLSBhL2ZzL25mcy9jbGllbnQuYw0KPiA+ICsrKyBiL2ZzL25m cy9jbGllbnQuYw0KPiA+IEBAIC01OTIsNiArNTkyLDIwIEBAIHZvaWQgbmZzX21hcmtfY2xpZW50 X3JlYWR5KHN0cnVjdCBuZnNfY2xpZW50ICpjbHAsIGludCBzdGF0ZSkNCj4gPiAgIAl3YWtlX3Vw X2FsbCgmbmZzX2NsaWVudF9hY3RpdmVfd3EpOw0KPiA+ICAgfQ0KPiA+DQo+ID4gK3N0YXRpYyBi b29sIG5mc19jbGllbnRfcmVhZHkoc3RydWN0IG5mc19jbGllbnQgKmNscCkNCj4gPiArew0KPiA+ ICsJcmV0dXJuIGNscC0+Y2xfY29uc19zdGF0ZTw9IE5GU19DU19SRUFEWTsNCj4gPiArfQ0KPiA+ ICsNCj4gPiAraW50IG5mc193YWl0X2NsaWVudF9yZWFkeShzdHJ1Y3QgbmZzX2NsaWVudCAqY2xw KQ0KPiA+ICt7DQo+ID4gKwlpZiAod2FpdF9ldmVudF9raWxsYWJsZShuZnNfY2xpZW50X2FjdGl2 ZV93cSwgbmZzX2NsaWVudF9yZWFkeShjbHApKTwgIDApDQo+ID4gKwkJcmV0dXJuIC1FUkVTVEFS VFNZUzsNCj4gDQo+IE9rLCBJIHNlZS4uLg0KPiBCVFcsIGNhbGxlciBvZiB0aGlzIGZ1bmN0aW9u IGlzIHBpcGVmcyBtb3VudCBvcGVyYXRpb24gY2FsbC4uLiBBbmQgd2hlbiB0aGlzIA0KPiBtb3Vu dCBjYWxsIHdhaXRzIGZvciBORlMgY2xpZW50cyAtIGl0IGxvb2sgYSBiaXQgb2RkIHRvIG1lLi4u DQo+IA0KPiANCj4gPiArCWlmIChjbHAtPmNsX2NvbnNfc3RhdGU8ICAwKQ0KPiA+ICsJCXJldHVy biBjbHAtPmNsX2NvbnNfc3RhdGU7DQo+ID4gKwlyZXR1cm4gMDsNCj4gPiArfQ0KPiA+ICsNCj4g PiAgIC8qDQo+ID4gICAgKiBXaXRoIHNlc3Npb25zLCB0aGUgY2xpZW50IGlzIG5vdCBtYXJrZWQg cmVhZHkgdW50aWwgYWZ0ZXIgYQ0KPiA+ICAgICogc3VjY2Vzc2Z1bCBFWENIQU5HRV9JRCBhbmQg Q1JFQVRFX1NFU1NJT04uDQo+ID4gZGlmZiAtLWdpdCBhL2ZzL25mcy9pZG1hcC5jIGIvZnMvbmZz L2lkbWFwLmMNCj4gPiBpbmRleCAzZThlZGJlLi42Nzk2MmM4IDEwMDY0NA0KPiA+IC0tLSBhL2Zz L25mcy9pZG1hcC5jDQo+ID4gKysrIGIvZnMvbmZzL2lkbWFwLmMNCj4gPiBAQCAtNTU4LDcgKzU1 OCw4IEBAIHN0YXRpYyBpbnQgcnBjX3BpcGVmc19ldmVudChzdHJ1Y3Qgbm90aWZpZXJfYmxvY2sg Km5iLCB1bnNpZ25lZCBsb25nIGV2ZW50LA0KPiA+ICAgCQlyZXR1cm4gMDsNCj4gPg0KPiA+ICAg CXdoaWxlICgoY2xwID0gbmZzX2dldF9jbGllbnRfZm9yX2V2ZW50KHNiLT5zX2ZzX2luZm8sIGV2 ZW50KSkpIHsNCj4gPiAtCQllcnJvciA9IF9fcnBjX3BpcGVmc19ldmVudChjbHAsIGV2ZW50LCBz Yik7DQo+ID4gKwkJaWYgKG5mc193YWl0X2NsaWVudF9yZWFkeShjbHApID09IDApDQo+ID4gKwkJ CWVycm9yID0gX19ycGNfcGlwZWZzX2V2ZW50KGNscCwgZXZlbnQsIHNiKTsNCj4gDQo+IA0KPiBX ZSBoYXZlIGFub3RoZXIgcHJvYmxlbSBoZXJlLg0KPiBuZnM0X2luaXRfY2xpZW50KCkgd2lsbCB0 cnkgdG8gY3JlYXRlIHBpcGUgZGVudHJpZXMgcHJpb3IgdG8gc2V0IG9mIE5GU19DU19SRUFEWSAN Cj4gdG8gdGhlIGNsaWVudC4gQW5kIGRlbnRyaWVzIHdpbGwgYmUgY3JlYXRlZCBzaW5jZSBzZW1h cGhvcmUgaXMgZHJvcHBlZCBhbmQgDQo+IHBlci1uZXQgc3VwZXJibG9jayB2YXJpYWJsZSBpcyBp bml0aWFsaXplZCBhbHJlYWR5Lg0KPiBCdXQgX19ycGNfcGlwZWZzX2V2ZW50KCkgcmVsYXlzIG9u IHRoZSBmYWN0LCB0aGF0IG5vIGRlbnRyaWVzIHByZXNlbnQuDQo+IExvb2tzIGxpa2UgdGhlIHBy b2JsZW0gd2FzIGludHJvZHVjZWQgYnkgbWUgaW4gYWFkOTQ4N2MuLi4NCj4gU28gbWF5YmUgd2Ug c2hvdWxkIG5vdCBjYWxsICJjb250aW51ZSIgaW5zdGVhZCAiX19ycGNfcGlwZWZzX2V2ZW50KCki LCB3aGVuIA0KPiBjbGllbnQgYmVjb21lcyByZWFkeT8NCj4gTG9va3MgbGlrZSB0aGlzIHdpbGwg YWxsb3cgdXMgdG8gaGFuZGxlIHN1Y2ggcmFjZXMuDQoNCkxldCBtZSByZXdvcmsgdGhpcyBwYXRj aCBhIGJpdC4uLg0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWlu dGFpbmVyDQoNCk5ldEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAu Y29tDQoNCg==

Myklebust, Trond

2012-05-22 20:32 UTC #8 lore raw

T24gVHVlLCAyMDEyLTA1LTIyIGF0IDEyOjQzIC0wNDAwLCBUcm9uZCBNeWtsZWJ1c3Qgd3JvdGU6 DQo+IE9uIFR1ZSwgMjAxMi0wNS0yMiBhdCAyMDoxOCArMDQwMCwgU3RhbmlzbGF2IEtpbnNidXJz a3kgd3JvdGU6DQo+ID4gV2UgaGF2ZSBhbm90aGVyIHByb2JsZW0gaGVyZS4NCj4gPiBuZnM0X2lu aXRfY2xpZW50KCkgd2lsbCB0cnkgdG8gY3JlYXRlIHBpcGUgZGVudHJpZXMgcHJpb3IgdG8gc2V0 IG9mIE5GU19DU19SRUFEWSANCj4gPiB0byB0aGUgY2xpZW50LiBBbmQgZGVudHJpZXMgd2lsbCBi ZSBjcmVhdGVkIHNpbmNlIHNlbWFwaG9yZSBpcyBkcm9wcGVkIGFuZCANCj4gPiBwZXItbmV0IHN1 cGVyYmxvY2sgdmFyaWFibGUgaXMgaW5pdGlhbGl6ZWQgYWxyZWFkeS4NCj4gPiBCdXQgX19ycGNf cGlwZWZzX2V2ZW50KCkgcmVsYXlzIG9uIHRoZSBmYWN0LCB0aGF0IG5vIGRlbnRyaWVzIHByZXNl bnQuDQo+ID4gTG9va3MgbGlrZSB0aGUgcHJvYmxlbSB3YXMgaW50cm9kdWNlZCBieSBtZSBpbiBh YWQ5NDg3Yy4uLg0KPiA+IFNvIG1heWJlIHdlIHNob3VsZCBub3QgY2FsbCAiY29udGludWUiIGlu c3RlYWQgIl9fcnBjX3BpcGVmc19ldmVudCgpIiwgd2hlbiANCj4gPiBjbGllbnQgYmVjb21lcyBy ZWFkeT8NCj4gPiBMb29rcyBsaWtlIHRoaXMgd2lsbCBhbGxvdyB1cyB0byBoYW5kbGUgc3VjaCBy YWNlcy4NCj4gDQo+IExldCBtZSByZXdvcmsgdGhpcyBwYXRjaCBhIGJpdC4uLg0KDQpUaGUgZm9s bG93aW5nIGlzIHVnbHksIGJ1dCBpdCBzaG91bGQgYmUgZGVtb25zdHJhYmx5IGNvcnJlY3QsIGFu ZCB3aWxsDQplbnN1cmUgdGhhdCBfX3JwY19waXBlZnNfZXZlbnQoKSB3aWxsIG9ubHkgYmUgY2Fs bGVkIGZvciBmdWxseQ0KaW5pdGlhbGlzZWQgbmZzX2NsaWVudHMuLi4NCg0KODwtLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0NCkZyb20gOTBjM2I5ZmU5ZmFlYWUzMmM4ZjYyOWU4YjZjYmY1ZjUwYmI5YjI5NSBNb24gU2Vw IDE3IDAwOjAwOjAwIDIwMDENCkZyb206IFRyb25kIE15a2xlYnVzdCA8VHJvbmQuTXlrbGVidXN0 QG5ldGFwcC5jb20+DQpEYXRlOiBUdWUsIDIyIE1heSAyMDEyIDE2OjIyOjUwIC0wNDAwDQpTdWJq ZWN0OiBbUEFUQ0ggMS8yXSBORlN2NDogRml4IGEgcmFjZSBpbiB0aGUgbmV0IG5hbWVzcGFjZSBt b3VudA0KIG5vdGlmaWNhdGlvbg0KDQpTaW5jZSB0aGUgc3RydWN0IG5mc19jbGllbnQgZ2V0cyBh ZGRlZCB0byB0aGUgZ2xvYmFsIG5mc19jbGllbnRfbGlzdA0KYmVmb3JlIGl0IGlzIGluaXRpYWxp c2VkLCBpdCBpcyBwb3NzaWJsZSB0aGF0IHJwY19waXBlZnNfZXZlbnQgY2FuDQplbmQgdXAgdHJ5 aW5nIHRvIGNyZWF0ZSBpZG1hcHBlciBlbnRyaWVzIG9uIHN1Y2ggYSB0aGluZy4NCg0KVGhlIHNv bHV0aW9uIGlzIHRvIGhhdmUgdGhlIG1vdW50IG5vdGlmaWNhdGlvbiB3YWl0IGZvciB0aGUNCmlu aXRpYWxpc2F0aW9uIG9mIGVhY2ggbmZzX2NsaWVudCB0byBjb21wbGV0ZSwgYW5kIHRoZW4gdG8N CnNraXAgYW55IGVudHJpZXMgZm9yIHdoaWNoIHRoZSBpdCBmYWlsZWQuDQoNClJlcG9ydGVkLWJ5 OiBTdGFuaXNsYXYgS2luc2J1cnNreSA8c2tpbnNidXJza3lAcGFyYWxsZWxzLmNvbT4NClNpZ25l ZC1vZmYtYnk6IFRyb25kIE15a2xlYnVzdCA8VHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20+DQot LS0NCiBmcy9uZnMvY2xpZW50LmMgICB8ICAgMTAgKysrKysrKysrKw0KIGZzL25mcy9pZG1hcC5j ICAgIHwgICAxNSArKysrKysrKysrKysrKysNCiBmcy9uZnMvaW50ZXJuYWwuaCB8ICAgIDEgKw0K IDMgZmlsZXMgY2hhbmdlZCwgMjYgaW5zZXJ0aW9ucygrKSwgMCBkZWxldGlvbnMoLSkNCg0KZGlm ZiAtLWdpdCBhL2ZzL25mcy9jbGllbnQuYyBiL2ZzL25mcy9jbGllbnQuYw0KaW5kZXggNjBmN2U0 ZS4uZDNjODU1MyAxMDA2NDQNCi0tLSBhL2ZzL25mcy9jbGllbnQuYw0KKysrIGIvZnMvbmZzL2Ns aWVudC5jDQpAQCAtNTgzLDYgKzU4MywxNiBAQCBmb3VuZF9jbGllbnQ6DQogCXJldHVybiBjbHA7 DQogfQ0KIA0KK3N0YXRpYyBib29sIG5mc19jbGllbnRfcmVhZHkoY29uc3Qgc3RydWN0IG5mc19j bGllbnQgKmNscCkNCit7DQorCXJldHVybiBjbHAtPmNsX2NvbnNfc3RhdGUgPD0gTkZTX0NTX1JF QURZOw0KK30NCisNCitpbnQgbmZzX3dhaXRfY2xpZW50X3JlYWR5KGNvbnN0IHN0cnVjdCBuZnNf Y2xpZW50ICpjbHApDQorew0KKwlyZXR1cm4gd2FpdF9ldmVudF9raWxsYWJsZShuZnNfY2xpZW50 X2FjdGl2ZV93cSwgbmZzX2NsaWVudF9yZWFkeShjbHApKTsNCit9DQorDQogLyoNCiAgKiBNYXJr IGEgc2VydmVyIGFzIHJlYWR5IG9yIGZhaWxlZA0KICAqLw0KZGlmZiAtLWdpdCBhL2ZzL25mcy9p ZG1hcC5jIGIvZnMvbmZzL2lkbWFwLmMNCmluZGV4IDNlOGVkYmUuLmMwNzUzYzUgMTAwNjQ0DQot LS0gYS9mcy9uZnMvaWRtYXAuYw0KKysrIGIvZnMvbmZzL2lkbWFwLmMNCkBAIC01MzAsOSArNTMw LDI0IEBAIHN0YXRpYyBzdHJ1Y3QgbmZzX2NsaWVudCAqbmZzX2dldF9jbGllbnRfZm9yX2V2ZW50 KHN0cnVjdCBuZXQgKm5ldCwgaW50IGV2ZW50KQ0KIAlzdHJ1Y3QgbmZzX25ldCAqbm4gPSBuZXRf Z2VuZXJpYyhuZXQsIG5mc19uZXRfaWQpOw0KIAlzdHJ1Y3QgZGVudHJ5ICpjbF9kZW50cnk7DQog CXN0cnVjdCBuZnNfY2xpZW50ICpjbHA7DQorCWludCBlcnI7DQogDQorcmVzdGFydDoNCiAJc3Bp bl9sb2NrKCZubi0+bmZzX2NsaWVudF9sb2NrKTsNCiAJbGlzdF9mb3JfZWFjaF9lbnRyeShjbHAs ICZubi0+bmZzX2NsaWVudF9saXN0LCBjbF9zaGFyZV9saW5rKSB7DQorCQkvKiBXYWl0IGZvciBp bml0aWFsaXNhdGlvbiB0byBmaW5pc2ggKi8NCisJCWlmIChjbHAtPmNsX2NvbnNfc3RhdGUgPiBO RlNfQ1NfUkVBRFkpIHsNCisJCQlhdG9taWNfaW5jKCZjbHAtPmNsX2NvdW50KTsNCisJCQlzcGlu X3VubG9jaygmbm4tPm5mc19jbGllbnRfbG9jayk7DQorCQkJZXJyID0gbmZzX3dhaXRfY2xpZW50 X3JlYWR5KGNscCk7DQorCQkJbmZzX3B1dF9jbGllbnQoY2xwKTsNCisJCQlpZiAoZXJyKQ0KKwkJ CQlyZXR1cm4gTlVMTDsNCisJCQlnb3RvIHJlc3RhcnQ7DQorCQl9DQorCQkvKiBTa2lwIG5mc19j bGllbnRzIHRoYXQgZmFpbGVkIHRvIGluaXRpYWxpc2UgKi8NCisJCWlmIChjbHAtPmNsX2NvbnNf c3RhdGUgPCAwKQ0KKwkJCWNvbnRpbnVlOw0KIAkJaWYgKGNscC0+cnBjX29wcyAhPSAmbmZzX3Y0 X2NsaWVudG9wcykNCiAJCQljb250aW51ZTsNCiAJCWNsX2RlbnRyeSA9IGNscC0+Y2xfaWRtYXAt PmlkbWFwX3BpcGUtPmRlbnRyeTsNCmRpZmYgLS1naXQgYS9mcy9uZnMvaW50ZXJuYWwuaCBiL2Zz L25mcy9pbnRlcm5hbC5oDQppbmRleCBiNzc3YmRhLi4zZWU0MDQwIDEwMDY0NA0KLS0tIGEvZnMv bmZzL2ludGVybmFsLmgNCisrKyBiL2ZzL25mcy9pbnRlcm5hbC5oDQpAQCAtMTY4LDYgKzE2OCw3 IEBAIGV4dGVybiBzdHJ1Y3QgbmZzX3NlcnZlciAqbmZzX2Nsb25lX3NlcnZlcihzdHJ1Y3QgbmZz X3NlcnZlciAqLA0KIAkJCQkJICAgc3RydWN0IG5mc19mYXR0ciAqLA0KIAkJCQkJICAgcnBjX2F1 dGhmbGF2b3JfdCk7DQogZXh0ZXJuIHZvaWQgbmZzX21hcmtfY2xpZW50X3JlYWR5KHN0cnVjdCBu ZnNfY2xpZW50ICpjbHAsIGludCBzdGF0ZSk7DQorZXh0ZXJuIGludCBuZnNfd2FpdF9jbGllbnRf cmVhZHkoY29uc3Qgc3RydWN0IG5mc19jbGllbnQgKmNscCk7DQogZXh0ZXJuIGludCBuZnM0X2No ZWNrX2NsaWVudF9yZWFkeShzdHJ1Y3QgbmZzX2NsaWVudCAqY2xwKTsNCiBleHRlcm4gc3RydWN0 IG5mc19jbGllbnQgKm5mczRfc2V0X2RzX2NsaWVudChzdHJ1Y3QgbmZzX2NsaWVudCogbWRzX2Ns cCwNCiAJCQkJCSAgICAgY29uc3Qgc3RydWN0IHNvY2thZGRyICpkc19hZGRyLA0KLS0gDQoxLjcu Ny42DQoNCg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRhaW5l cg0KDQpOZXRBcHANClRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tDQp3d3cubmV0YXBwLmNvbQ0K DQo=

Stanislav Kinsbursky

author

2012-05-23 11:30 UTC #9 lore raw

Quoted 70 lines · Trond

Quoted 11 lines · Trond Myklebust
Quoted 9 lines · Stanislav Kinsbursky
We have another problem here.
nfs4_init_client() will try to create pipe dentries prior to set of NFS_CS_READY to the client. And dentries will be created since semaphore is dropped and per-net superblock variable is initialized already.
But __rpc_pipefs_event() relays on the fact, that no dentries present. Looks like the problem was introduced by me in aad9487c...
So maybe we should not call "continue" instead "__rpc_pipefs_event()", when client becomes ready?
Looks like this will allow us to handle such races.
Quoted 1 line
Let me rework this patch a bit...
The following is ugly, but it should be demonstrably correct, and will ensure that __rpc_pipefs_event() will only be called for fully initialised nfs_clients...
8<---------------------------------------------------------------------
From 90c3b9fe9faeae32c8f629e8b6cbf5f50bb9b295 Mon Sep 17 00:00:00 2001
From: Trond Myklebust<[email protected]>
Date: Tue, 22 May 2012 16:22:50 -0400
Subject: [PATCH 1/2] NFSv4: Fix a race in the net namespace mount
notification
Since the struct nfs_client gets added to the global nfs_client_list before it is initialised, it is possible that rpc_pipefs_event can end up trying to create idmapper entries on such a thing.
The solution is to have the mount notification wait for the initialisation of each nfs_client to complete, and then to skip any entries for which the it failed.
Reported-by Stanislav Kinsbursky<[email protected]>
Signed-off-by Trond Myklebust<[email protected]>
---
  fs/nfs/client.c   |   10 ++++++++++
  fs/nfs/idmap.c    |   15 +++++++++++++++
  fs/nfs/internal.h |    1 +
  3 files changed, 26 insertions(+), 0 deletions(-)
fs/nfs/client.c +10 -0
index 60f7e4e..d3c8553 100644
@@ -583,6 +583,16 @@ found_client:
  	return clp;
  }
 
+static bool nfs_client_ready(const struct nfs_client *clp)
+{
+	return clp->cl_cons_state<= NFS_CS_READY;
+}
+
+int nfs_wait_client_ready(const struct nfs_client *clp)
+{
+	return wait_event_killable(nfs_client_active_wq, nfs_client_ready(clp));
+}
+
  /*
   * Mark a server as ready or failed
   */
fs/nfs/idmap.c +0 -0
index 3e8edbe..c0753c5 100644
@@ -530,9 +530,24 @@ static struct nfs_client *nfs_get_client_for_event(struct net *net, int event)
  	struct nfs_net *nn = net_generic(net, nfs_net_id);
  	struct dentry *cl_dentry;
  	struct nfs_client *clp;

fs/nfs/idmap.c static struct nfs_client *nfs_get_client_for_event(struct net *net, int event) · line 542

Quoted 10 lines

fs/nfs/idmap.c +7 -0

+	int err;
 
+restart:
  	spin_lock(&nn->nfs_client_lock);
  	list_for_each_entry(clp,&nn->nfs_client_list, cl_share_link) {
+		/* Wait for initialisation to finish */
+		if (clp->cl_cons_state>  NFS_CS_READY) {
+			atomic_inc(&clp->cl_count);
+			spin_unlock(&nn->nfs_client_lock);
+			err = nfs_wait_client_ready(clp);

What about NFSv4.1 ?
It's clients NFS_CS_READY status depends on session establishing RPC calls... Which in turn can hung up pipefs mount call...

Moreover, looks like pipefs dentries creation have to be synchronized by nfs_client_lock somehow... Otherwise because of races we can get a client without pipe dentry....

Quoted 23 lines

fs/nfs/idmap.c +8 -0

+			nfs_put_client(clp);
+			if (err)
+				return NULL;
+			goto restart;
+		}
+		/* Skip nfs_clients that failed to initialise */
+		if (clp->cl_cons_state<  0)
+			continue;
  		if (clp->rpc_ops !=&nfs_v4_clientops)
  			continue;
  		cl_dentry = clp->cl_idmap->idmap_pipe->dentry;

fs/nfs/internal.h +1 -0

index b777bda..3ee4040 100644
@@ -168,6 +168,7 @@ extern struct nfs_server *nfs_clone_server(struct nfs_server *,
  					   struct nfs_fattr *,
  					   rpc_authflavor_t);
  extern void nfs_mark_client_ready(struct nfs_client *clp, int state);
+extern int nfs_wait_client_ready(const struct nfs_client *clp);
  extern int nfs4_check_client_ready(struct nfs_client *clp);
  extern struct nfs_client *nfs4_set_ds_client(struct nfs_client* mds_clp,
  					     const struct sockaddr *ds_addr,

Signature · 2 lines

Best regards,
Stanislav Kinsbursky

Kinsbursky Stanislav

2012-05-23 12:09 UTC #10 lore raw

Quoted 3 lines · Stanislav Kinsbursky

Moreover, looks like pipefs dentries creation have to be synchronized by nfs_client_lock somehow... Otherwise because of races we can get a client without pipe dentry....

Taking this back.

Signature · 2 lines

Best regards,
Stanislav Kinsbursky

Myklebust, Trond

2012-05-23 13:57 UTC #11 lore raw

In reply to Stanislav Kinsbursky

T24gV2VkLCAyMDEyLTA1LTIzIGF0IDE1OjMwICswNDAwLCBTdGFuaXNsYXYgS2luc2J1cnNreSB3 cm90ZToNCj4gT24gMjMuMDUuMjAxMiAwMDozMiwgTXlrbGVidXN0LCBUcm9uZCB3cm90ZToNCj4g PiBPbiBUdWUsIDIwMTItMDUtMjIgYXQgMTI6NDMgLTA0MDAsIFRyb25kIE15a2xlYnVzdCB3cm90 ZToNCj4gPj4gT24gVHVlLCAyMDEyLTA1LTIyIGF0IDIwOjE4ICswNDAwLCBTdGFuaXNsYXYgS2lu c2J1cnNreSB3cm90ZToNCj4gPj4+IFdlIGhhdmUgYW5vdGhlciBwcm9ibGVtIGhlcmUuDQo+ID4+ PiBuZnM0X2luaXRfY2xpZW50KCkgd2lsbCB0cnkgdG8gY3JlYXRlIHBpcGUgZGVudHJpZXMgcHJp b3IgdG8gc2V0IG9mIE5GU19DU19SRUFEWQ0KPiA+Pj4gdG8gdGhlIGNsaWVudC4gQW5kIGRlbnRy aWVzIHdpbGwgYmUgY3JlYXRlZCBzaW5jZSBzZW1hcGhvcmUgaXMgZHJvcHBlZCBhbmQNCj4gPj4+ IHBlci1uZXQgc3VwZXJibG9jayB2YXJpYWJsZSBpcyBpbml0aWFsaXplZCBhbHJlYWR5Lg0KPiA+ Pj4gQnV0IF9fcnBjX3BpcGVmc19ldmVudCgpIHJlbGF5cyBvbiB0aGUgZmFjdCwgdGhhdCBubyBk ZW50cmllcyBwcmVzZW50Lg0KPiA+Pj4gTG9va3MgbGlrZSB0aGUgcHJvYmxlbSB3YXMgaW50cm9k dWNlZCBieSBtZSBpbiBhYWQ5NDg3Yy4uLg0KPiA+Pj4gU28gbWF5YmUgd2Ugc2hvdWxkIG5vdCBj YWxsICJjb250aW51ZSIgaW5zdGVhZCAiX19ycGNfcGlwZWZzX2V2ZW50KCkiLCB3aGVuDQo+ID4+ PiBjbGllbnQgYmVjb21lcyByZWFkeT8NCj4gPj4+IExvb2tzIGxpa2UgdGhpcyB3aWxsIGFsbG93 IHVzIHRvIGhhbmRsZSBzdWNoIHJhY2VzLg0KPiA+Pg0KPiA+PiBMZXQgbWUgcmV3b3JrIHRoaXMg cGF0Y2ggYSBiaXQuLi4NCj4gPg0KPiA+IFRoZSBmb2xsb3dpbmcgaXMgdWdseSwgYnV0IGl0IHNo b3VsZCBiZSBkZW1vbnN0cmFibHkgY29ycmVjdCwgYW5kIHdpbGwNCj4gPiBlbnN1cmUgdGhhdCBf X3JwY19waXBlZnNfZXZlbnQoKSB3aWxsIG9ubHkgYmUgY2FsbGVkIGZvciBmdWxseQ0KPiA+IGlu aXRpYWxpc2VkIG5mc19jbGllbnRzLi4uDQo+ID4NCj4gPiA4PC0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQ0KPiA+ICBG cm9tIDkwYzNiOWZlOWZhZWFlMzJjOGY2MjllOGI2Y2JmNWY1MGJiOWIyOTUgTW9uIFNlcCAxNyAw MDowMDowMCAyMDAxDQo+ID4gRnJvbTogVHJvbmQgTXlrbGVidXN0PFRyb25kLk15a2xlYnVzdEBu ZXRhcHAuY29tPg0KPiA+IERhdGU6IFR1ZSwgMjIgTWF5IDIwMTIgMTY6MjI6NTAgLTA0MDANCj4g PiBTdWJqZWN0OiBbUEFUQ0ggMS8yXSBORlN2NDogRml4IGEgcmFjZSBpbiB0aGUgbmV0IG5hbWVz cGFjZSBtb3VudA0KPiA+ICAgbm90aWZpY2F0aW9uDQo+ID4NCj4gPiBTaW5jZSB0aGUgc3RydWN0 IG5mc19jbGllbnQgZ2V0cyBhZGRlZCB0byB0aGUgZ2xvYmFsIG5mc19jbGllbnRfbGlzdA0KPiA+ IGJlZm9yZSBpdCBpcyBpbml0aWFsaXNlZCwgaXQgaXMgcG9zc2libGUgdGhhdCBycGNfcGlwZWZz X2V2ZW50IGNhbg0KPiA+IGVuZCB1cCB0cnlpbmcgdG8gY3JlYXRlIGlkbWFwcGVyIGVudHJpZXMg b24gc3VjaCBhIHRoaW5nLg0KPiA+DQo+ID4gVGhlIHNvbHV0aW9uIGlzIHRvIGhhdmUgdGhlIG1v dW50IG5vdGlmaWNhdGlvbiB3YWl0IGZvciB0aGUNCj4gPiBpbml0aWFsaXNhdGlvbiBvZiBlYWNo IG5mc19jbGllbnQgdG8gY29tcGxldGUsIGFuZCB0aGVuIHRvDQo+ID4gc2tpcCBhbnkgZW50cmll cyBmb3Igd2hpY2ggdGhlIGl0IGZhaWxlZC4NCj4gPg0KPiA+IFJlcG9ydGVkLWJ5OiBTdGFuaXNs YXYgS2luc2J1cnNreTxza2luc2J1cnNreUBwYXJhbGxlbHMuY29tPg0KPiA+IFNpZ25lZC1vZmYt Ynk6IFRyb25kIE15a2xlYnVzdDxUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbT4NCj4gPiAtLS0N Cj4gPiAgIGZzL25mcy9jbGllbnQuYyAgIHwgICAxMCArKysrKysrKysrDQo+ID4gICBmcy9uZnMv aWRtYXAuYyAgICB8ICAgMTUgKysrKysrKysrKysrKysrDQo+ID4gICBmcy9uZnMvaW50ZXJuYWwu aCB8ICAgIDEgKw0KPiA+ICAgMyBmaWxlcyBjaGFuZ2VkLCAyNiBpbnNlcnRpb25zKCspLCAwIGRl bGV0aW9ucygtKQ0KPiA+DQo+ID4gZGlmZiAtLWdpdCBhL2ZzL25mcy9jbGllbnQuYyBiL2ZzL25m cy9jbGllbnQuYw0KPiA+IGluZGV4IDYwZjdlNGUuLmQzYzg1NTMgMTAwNjQ0DQo+ID4gLS0tIGEv ZnMvbmZzL2NsaWVudC5jDQo+ID4gKysrIGIvZnMvbmZzL2NsaWVudC5jDQo+ID4gQEAgLTU4Myw2 ICs1ODMsMTYgQEAgZm91bmRfY2xpZW50Og0KPiA+ICAgCXJldHVybiBjbHA7DQo+ID4gICB9DQo+ ID4NCj4gPiArc3RhdGljIGJvb2wgbmZzX2NsaWVudF9yZWFkeShjb25zdCBzdHJ1Y3QgbmZzX2Ns aWVudCAqY2xwKQ0KPiA+ICt7DQo+ID4gKwlyZXR1cm4gY2xwLT5jbF9jb25zX3N0YXRlPD0gTkZT X0NTX1JFQURZOw0KPiA+ICt9DQo+ID4gKw0KPiA+ICtpbnQgbmZzX3dhaXRfY2xpZW50X3JlYWR5 KGNvbnN0IHN0cnVjdCBuZnNfY2xpZW50ICpjbHApDQo+ID4gK3sNCj4gPiArCXJldHVybiB3YWl0 X2V2ZW50X2tpbGxhYmxlKG5mc19jbGllbnRfYWN0aXZlX3dxLCBuZnNfY2xpZW50X3JlYWR5KGNs cCkpOw0KPiA+ICt9DQo+ID4gKw0KPiA+ICAgLyoNCj4gPiAgICAqIE1hcmsgYSBzZXJ2ZXIgYXMg cmVhZHkgb3IgZmFpbGVkDQo+ID4gICAgKi8NCj4gPiBkaWZmIC0tZ2l0IGEvZnMvbmZzL2lkbWFw LmMgYi9mcy9uZnMvaWRtYXAuYw0KPiA+IGluZGV4IDNlOGVkYmUuLmMwNzUzYzUgMTAwNjQ0DQo+ ID4gLS0tIGEvZnMvbmZzL2lkbWFwLmMNCj4gPiArKysgYi9mcy9uZnMvaWRtYXAuYw0KPiA+IEBA IC01MzAsOSArNTMwLDI0IEBAIHN0YXRpYyBzdHJ1Y3QgbmZzX2NsaWVudCAqbmZzX2dldF9jbGll bnRfZm9yX2V2ZW50KHN0cnVjdCBuZXQgKm5ldCwgaW50IGV2ZW50KQ0KPiA+ICAgCXN0cnVjdCBu ZnNfbmV0ICpubiA9IG5ldF9nZW5lcmljKG5ldCwgbmZzX25ldF9pZCk7DQo+ID4gICAJc3RydWN0 IGRlbnRyeSAqY2xfZGVudHJ5Ow0KPiA+ICAgCXN0cnVjdCBuZnNfY2xpZW50ICpjbHA7DQo+ID4g KwlpbnQgZXJyOw0KPiA+DQo+ID4gK3Jlc3RhcnQ6DQo+ID4gICAJc3Bpbl9sb2NrKCZubi0+bmZz X2NsaWVudF9sb2NrKTsNCj4gPiAgIAlsaXN0X2Zvcl9lYWNoX2VudHJ5KGNscCwmbm4tPm5mc19j bGllbnRfbGlzdCwgY2xfc2hhcmVfbGluaykgew0KPiA+ICsJCS8qIFdhaXQgZm9yIGluaXRpYWxp c2F0aW9uIHRvIGZpbmlzaCAqLw0KPiA+ICsJCWlmIChjbHAtPmNsX2NvbnNfc3RhdGU+ICBORlNf Q1NfUkVBRFkpIHsNCj4gPiArCQkJYXRvbWljX2luYygmY2xwLT5jbF9jb3VudCk7DQo+ID4gKwkJ CXNwaW5fdW5sb2NrKCZubi0+bmZzX2NsaWVudF9sb2NrKTsNCj4gPiArCQkJZXJyID0gbmZzX3dh aXRfY2xpZW50X3JlYWR5KGNscCk7DQo+IA0KPiANCj4gV2hhdCBhYm91dCBORlN2NC4xID8NCj4g SXQncyBjbGllbnRzIE5GU19DU19SRUFEWSBzdGF0dXMgZGVwZW5kcyBvbiBzZXNzaW9uIGVzdGFi bGlzaGluZyBSUEMgY2FsbHMuLi4NCj4gV2hpY2ggaW4gdHVybiBjYW4gaHVuZyB1cCBwaXBlZnMg bW91bnQgY2FsbC4uLg0KDQpUaGUgYWx0ZXJuYXRpdmUgdGhlbiBpcyB0byB3YWl0IGZvciBjbF9j b25zX3N0YXRlICE9IE5GU19DU19JTklUSU5HLg0KVGhhdCBzaG91bGRuJ3QgcmVxdWlyZSBhbnkg dXBjYWxscywgYW5kIHNvIHNob3VsZG4ndCBiZSBhYmxlIHRvDQpkZWFkbG9jay4NCg0KLS0gDQpU cm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRhaW5lcg0KDQpOZXRBcHANClRy b25kLk15a2xlYnVzdEBuZXRhcHAuY29tDQp3d3cubmV0YXBwLmNvbQ0KDQo=

Stanislav Kinsbursky

author

2012-05-22 15:03 UTC #12 lore raw

In reply to Myklebust, Trond

Quoted 20 lines · Trond

Quoted 9 lines · Stanislav Kinsbursky
Client have to be initialized prior to adding it to per-net clients list, because otherwise there are races, shown below:
CPU#0 CPU#1
_____ _____
nfs_get_client
nfs_alloc_client
list_add(..., nfs_client_list)
Quoted 9 lines
					rpc_fill_super
					rpc_pipefs_event
					nfs_get_client_for_event
					__rpc_pipefs_event
					(clp->cl_rpcclient is uninitialized)
					BUG()
init_client
clp->cl_rpcclient = ...

Quoted 6 lines

Why not simply change nfs_get_client_for_event() so that it doesn't touch nfs_clients that have clp->cl_cons_state!=NFS_CS_READY?
That should ensure that it doesn't touch nfs_clients that failed to initialise and/or are still in the process of being initialised.

It looks like in this case we will have another races:

CPU#0 CPU#1
_____ _____

nfs4_init_client
nfs_idmap_new
nfs_idmap_register
rpc_get_sb_net (fail - no pipefs)
rpc_fill_super rpc_pipefs_event nfs_get_client_for_event (skip client - NFS_CS_READY is not set)
nfs_mark_client_ready(NFS_CS_READY)

And we are having client without idmap pipe...

Quoted 3 lines

Cheers
Trond

Signature · 2 lines

Best regards,
Stanislav Kinsbursky