[PATCH] powerpc/fsl-rio: Fix fsl_rio_setup error paths and use-after-unmap

[PATCH] powerpc/fsl-rio: Fix fsl_rio_setup error paths and use-after-unmap

[Scott Wood]

Several of the error paths from fsl_rio_setup are missing error
messages.

Worse, fsl_rio_setup initializes several global pointers and does not
NULL them out after freeing/unmapping on error.  This caused
fsl_rio_mcheck_exception() to crash when accessing rio_regs_win which
was non-NULL but had been unmapped.

Signed-off-by: Scott Wood <scottwood@freescale.com>
Cc: Liu Gang <Gang.Liu@freescale.com>
---
Liu Gang, are you sure all of these error conditions are fatal?  Why
does the rio driver fail if rmu is not present (e.g.  on t4240)?
---
 arch/powerpc/sysdev/fsl_rio.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/sysdev/fsl_rio.c b/arch/powerpc/sysdev/fsl_rio.c
index cf2b084..c04b718 100644
--- a/arch/powerpc/sysdev/fsl_rio.c
+++ b/arch/powerpc/sysdev/fsl_rio.c
@@ -391,8 +391,10 @@ int fsl_rio_setup(struct platform_device *dev)
 	ops->get_inb_message = fsl_get_inb_message;
 
 	rmu_node = of_parse_phandle(dev->dev.of_node, "fsl,srio-rmu-handle", 0);
-	if (!rmu_node)
+	if (!rmu_node) {
+		dev_err(&dev->dev, "No valid fsl,srio-rmu-handle property\n");
 		goto err_rmu;
+	}
 	rc = of_address_to_resource(rmu_node, 0, &rmu_regs);
 	if (rc) {
 		dev_err(&dev->dev, "Can't get %s property 'reg'\n",
@@ -413,6 +415,7 @@ int fsl_rio_setup(struct platform_device *dev)
 	/*set up doobell node*/
 	np = of_find_compatible_node(NULL, NULL, "fsl,srio-dbell-unit");
 	if (!np) {
+		dev_err(&dev->dev, "No fsl,srio-dbell-unit node\n");
 		rc = -ENODEV;
 		goto err_dbell;
 	}
@@ -441,6 +444,7 @@ int fsl_rio_setup(struct platform_device *dev)
 	/*set up port write node*/
 	np = of_find_compatible_node(NULL, NULL, "fsl,srio-port-write-unit");
 	if (!np) {
+		dev_err(&dev->dev, "No fsl,srio-port-write-unit node\n");
 		rc = -ENODEV;
 		goto err_pw;
 	}
@@ -633,14 +637,18 @@ int fsl_rio_setup(struct platform_device *dev)
 	return 0;
 err:
 	kfree(pw);
+	pw = NULL;
 err_pw:
 	kfree(dbell);
+	dbell = NULL;
 err_dbell:
 	iounmap(rmu_regs_win);
+	rmu_regs_win = NULL;
 err_rmu:
 	kfree(ops);
 err_ops:
 	iounmap(rio_regs_win);
+	rio_regs_win = NULL;
 err_rio_regs:
 	return rc;
 }
-- 
1.9.1

RE: [PATCH] powerpc/fsl-rio: Fix fsl_rio_setup error paths and use-after-unmap

[Gang.Liu]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, April 29, 2014 9:32 AM
> To: linuxppc-dev@lists.ozlabs.org
> Cc: Wood Scott-B07421; Liu Gang-B34182
> Subject: [PATCH] powerpc/fsl-rio: Fix fsl_rio_setup error paths and use-
> after-unmap
>=20
> Several of the error paths from fsl_rio_setup are missing error messages.
>=20
> Worse, fsl_rio_setup initializes several global pointers and does not
> NULL them out after freeing/unmapping on error.  This caused
> fsl_rio_mcheck_exception() to crash when accessing rio_regs_win which was
> non-NULL but had been unmapped.
>=20
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> Cc: Liu Gang <Gang.Liu@freescale.com>
> ---
> Liu Gang, are you sure all of these error conditions are fatal?  Why does
> the rio driver fail if rmu is not present (e.g.  on t4240)?

Hi Scott, I think the errors you modified in the patch are serious and shou=
ld
be fixed. Thanks very much!
And in fact, the rio driver can be used just for the submodule of the SRIO:=
 RMU.
It should be used with arch/powerpc/sysdev/fsl_rmu.c and there should have =
the
RMU module.
The fsl_rio.c implements some basic and needed works to support the RMU run=
ning well.

In addition, could you please help to apply the following patch:

http://patchwork.ozlabs.org/patch/337810/

Thanks!
Liu Gang  =20
> ---
>  arch/powerpc/sysdev/fsl_rio.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)

Re: [PATCH] powerpc/fsl-rio: Fix fsl_rio_setup error paths and use-after-unmap

[Scott Wood]

On Mon, 2014-04-28 at 23:04 -0500, Liu Gang-B34182 wrote:
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Tuesday, April 29, 2014 9:32 AM
> > To: linuxppc-dev@lists.ozlabs.org
> > Cc: Wood Scott-B07421; Liu Gang-B34182
> > Subject: [PATCH] powerpc/fsl-rio: Fix fsl_rio_setup error paths and use-
> > after-unmap
> > 
> > Several of the error paths from fsl_rio_setup are missing error messages.
> > 
> > Worse, fsl_rio_setup initializes several global pointers and does not
> > NULL them out after freeing/unmapping on error.  This caused
> > fsl_rio_mcheck_exception() to crash when accessing rio_regs_win which was
> > non-NULL but had been unmapped.
> > 
> > Signed-off-by: Scott Wood <scottwood@freescale.com>
> > Cc: Liu Gang <Gang.Liu@freescale.com>
> > ---
> > Liu Gang, are you sure all of these error conditions are fatal?  Why does
> > the rio driver fail if rmu is not present (e.g.  on t4240)?
> 
> Hi Scott, I think the errors you modified in the patch are serious and should
> be fixed. Thanks very much!
> And in fact, the rio driver can be used just for the submodule of the SRIO: RMU.
> It should be used with arch/powerpc/sysdev/fsl_rmu.c and there should have the
> RMU module.
> The fsl_rio.c implements some basic and needed works to support the RMU running well.

I don't quite follow -- is it expected that rio can work without rmu, or
not?  As is, fsl_rio_setup() will error out if it doesn't find an
fsl,srio-rmu-handle property.

> In addition, could you please help to apply the following patch:
> 
> http://patchwork.ozlabs.org/patch/337810/

Yes, I'll apply it when I do my next batch of patch applications.

-Scott

RE: [PATCH] powerpc/fsl-rio: Fix fsl_rio_setup error paths and use-after-unmap

[Gang.Liu]

DQo+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+IEZyb206IFdvb2QgU2NvdHQtQjA3NDIx
DQo+IFNlbnQ6IFdlZG5lc2RheSwgQXByaWwgMzAsIDIwMTQgMToxMyBBTQ0KPiBUbzogTGl1IEdh
bmctQjM0MTgyDQo+IENjOiBsaW51eHBwYy1kZXZAbGlzdHMub3psYWJzLm9yZw0KPiBTdWJqZWN0
OiBSZTogW1BBVENIXSBwb3dlcnBjL2ZzbC1yaW86IEZpeCBmc2xfcmlvX3NldHVwIGVycm9yIHBh
dGhzIGFuZA0KPiB1c2UtYWZ0ZXItdW5tYXANCj4gDQo+IE9uIE1vbiwgMjAxNC0wNC0yOCBhdCAy
MzowNCAtMDUwMCwgTGl1IEdhbmctQjM0MTgyIHdyb3RlOg0KPiA+ID4gLS0tLS1PcmlnaW5hbCBN
ZXNzYWdlLS0tLS0NCj4gPiA+IEZyb206IFdvb2QgU2NvdHQtQjA3NDIxDQo+ID4gPiBTZW50OiBU
dWVzZGF5LCBBcHJpbCAyOSwgMjAxNCA5OjMyIEFNDQo+ID4gPiBUbzogbGludXhwcGMtZGV2QGxp
c3RzLm96bGFicy5vcmcNCj4gPiA+IENjOiBXb29kIFNjb3R0LUIwNzQyMTsgTGl1IEdhbmctQjM0
MTgyDQo+ID4gPiBTdWJqZWN0OiBbUEFUQ0hdIHBvd2VycGMvZnNsLXJpbzogRml4IGZzbF9yaW9f
c2V0dXAgZXJyb3IgcGF0aHMgYW5kDQo+ID4gPiB1c2UtIGFmdGVyLXVubWFwDQo+ID4gPg0KPiA+
ID4gU2V2ZXJhbCBvZiB0aGUgZXJyb3IgcGF0aHMgZnJvbSBmc2xfcmlvX3NldHVwIGFyZSBtaXNz
aW5nIGVycm9yDQo+IG1lc3NhZ2VzLg0KPiA+ID4NCj4gPiA+IFdvcnNlLCBmc2xfcmlvX3NldHVw
IGluaXRpYWxpemVzIHNldmVyYWwgZ2xvYmFsIHBvaW50ZXJzIGFuZCBkb2VzDQo+ID4gPiBub3Qg
TlVMTCB0aGVtIG91dCBhZnRlciBmcmVlaW5nL3VubWFwcGluZyBvbiBlcnJvci4gIFRoaXMgY2F1
c2VkDQo+ID4gPiBmc2xfcmlvX21jaGVja19leGNlcHRpb24oKSB0byBjcmFzaCB3aGVuIGFjY2Vz
c2luZyByaW9fcmVnc193aW4NCj4gPiA+IHdoaWNoIHdhcyBub24tTlVMTCBidXQgaGFkIGJlZW4g
dW5tYXBwZWQuDQo+ID4gPg0KPiA+ID4gU2lnbmVkLW9mZi1ieTogU2NvdHQgV29vZCA8c2NvdHR3
b29kQGZyZWVzY2FsZS5jb20+DQo+ID4gPiBDYzogTGl1IEdhbmcgPEdhbmcuTGl1QGZyZWVzY2Fs
ZS5jb20+DQo+ID4gPiAtLS0NCj4gPiA+IExpdSBHYW5nLCBhcmUgeW91IHN1cmUgYWxsIG9mIHRo
ZXNlIGVycm9yIGNvbmRpdGlvbnMgYXJlIGZhdGFsPyAgV2h5DQo+ID4gPiBkb2VzIHRoZSByaW8g
ZHJpdmVyIGZhaWwgaWYgcm11IGlzIG5vdCBwcmVzZW50IChlLmcuICBvbiB0NDI0MCk/DQo+ID4N
Cj4gPiBIaSBTY290dCwgSSB0aGluayB0aGUgZXJyb3JzIHlvdSBtb2RpZmllZCBpbiB0aGUgcGF0
Y2ggYXJlIHNlcmlvdXMgYW5kDQo+ID4gc2hvdWxkIGJlIGZpeGVkLiBUaGFua3MgdmVyeSBtdWNo
IQ0KPiA+IEFuZCBpbiBmYWN0LCB0aGUgcmlvIGRyaXZlciBjYW4gYmUgdXNlZCBqdXN0IGZvciB0
aGUgc3VibW9kdWxlIG9mIHRoZQ0KPiBTUklPOiBSTVUuDQo+ID4gSXQgc2hvdWxkIGJlIHVzZWQg
d2l0aCBhcmNoL3Bvd2VycGMvc3lzZGV2L2ZzbF9ybXUuYyBhbmQgdGhlcmUgc2hvdWxkDQo+ID4g
aGF2ZSB0aGUgUk1VIG1vZHVsZS4NCj4gPiBUaGUgZnNsX3Jpby5jIGltcGxlbWVudHMgc29tZSBi
YXNpYyBhbmQgbmVlZGVkIHdvcmtzIHRvIHN1cHBvcnQgdGhlIFJNVQ0KPiBydW5uaW5nIHdlbGwu
DQo+IA0KPiBJIGRvbid0IHF1aXRlIGZvbGxvdyAtLSBpcyBpdCBleHBlY3RlZCB0aGF0IHJpbyBj
YW4gd29yayB3aXRob3V0IHJtdSwgb3INCj4gbm90PyAgQXMgaXMsIGZzbF9yaW9fc2V0dXAoKSB3
aWxsIGVycm9yIG91dCBpZiBpdCBkb2Vzbid0IGZpbmQgYW4NCj4gZnNsLHNyaW8tcm11LWhhbmRs
ZSBwcm9wZXJ0eS4NCg0KZnNsX3Jpb19zZXR1cCgpIGRvZXNuJ3QgZXhwZWN0IHRoYXQgcmlvIGNh
biB3b3JrIHdpdGhvdXQgcm11LiBBbGwgdGhlIHJpbw0KZHJpdmVycyBqdXN0IGhhcyBvbmUgcHVy
cG9zZSwgaXQncyBybXUuIEJ1dCBybXUgaXMgYSBzdWJtb2R1bGUgb2YgdGhlIHJpbywNCnNvIHRo
ZSBkcml2ZXIgc2hvdWxkIHBhcnNlIHJpbyBpbiBkdGIgYW5kIGZpbmlzaCBzb21lIGluaXRpYWwg
d29ya3MgZmlyc3QsDQphbmQgdGhlbiB0byBzZXR1cCBybXUuDQpUaGF0J3Mgd2h5IHRoZSBkcml2
ZXJzIGNhbm5vdCBqdXN0IGhhdmUgYSBzdWNoIGFzIHJtdV9zZXR1cCgpIHRvIHBhcnNlDQpmc2ws
c3Jpby1ybXUtaGFuZGxlIHByb3BlcnR5Lg0KDQpUaGFua3MsDQpMaXUgR2FuZw0KIA0K