| Message ID | 20180710173200.19853-4-bart.vanassche@wdc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, Jul 10, 2018 at 10:32:00AM -0700, Bart Van Assche wrote: > Make sure that channel objects continue to exist until the target > core has called the .close_session() callback function. This patch > voids that KASAN sporadically reports the following: > > BUG: KASAN: use-after-free in do_raw_spin_lock+0x1c/0x130 > Read of size 4 at addr ffff8801534b16e4 by task rmdir/14805 > CPU: 16 PID: 14805 Comm: rmdir Not tainted 4.18.0-rc2-dbg+ #5 > Call Trace: > dump_stack+0xa4/0xf5 > print_address_description+0x6f/0x270 > kasan_report+0x241/0x360 > __asan_load4+0x78/0x80 > do_raw_spin_lock+0x1c/0x130 > _raw_spin_lock_irqsave+0x52/0x60 > srpt_set_ch_state+0x27/0x70 [ib_srpt] > srpt_disconnect_ch+0x1b/0xc0 [ib_srpt] > srpt_close_session+0xa8/0x260 [ib_srpt] > target_shutdown_sessions+0x170/0x180 [target_core_mod] > core_tpg_del_initiator_node_acl+0xf3/0x200 [target_core_mod] > target_fabric_nacl_base_release+0x25/0x30 [target_core_mod] > config_item_release+0x9c/0x110 [configfs] > config_item_put+0x26/0x30 [configfs] > configfs_rmdir+0x3b8/0x510 [configfs] > vfs_rmdir+0xb3/0x1e0 > do_rmdir+0x262/0x2c0 > do_syscall_64+0x77/0x230 > entry_SYSCALL_64_after_hwframe+0x49/0xbe > > Fixes: a42d985bd5b2 ("ib_srpt: Initial SRP Target merge for v3.3-rc1") > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> > Cc: <stable@vger.kernel.org> > drivers/infiniband/ulp/srpt/ib_srpt.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c > index 325bae29e90d..705f6a992d82 100644 > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c > @@ -2152,6 +2152,7 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev, > } > > kref_init(&ch->kref); > + kref_get(&ch->kref); Oh that is ugly, can you put the 'get' closer to the the place that stores the ch pointer this kref is protecting? Perhaps it is one of the list_add's in this function? Guessing the kref created kref_init should probably belong to target_core's 'se_sess->fabric_sess_ptr' .. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 10, 2018 at 10:32:00AM -0700, Bart Van Assche wrote: > Make sure that channel objects continue to exist until the target > core has called the .close_session() callback function. This patch > voids that KASAN sporadically reports the following: > > BUG: KASAN: use-after-free in do_raw_spin_lock+0x1c/0x130 > Read of size 4 at addr ffff8801534b16e4 by task rmdir/14805 > CPU: 16 PID: 14805 Comm: rmdir Not tainted 4.18.0-rc2-dbg+ #5 > Call Trace: > dump_stack+0xa4/0xf5 > print_address_description+0x6f/0x270 > kasan_report+0x241/0x360 > __asan_load4+0x78/0x80 > do_raw_spin_lock+0x1c/0x130 > _raw_spin_lock_irqsave+0x52/0x60 > srpt_set_ch_state+0x27/0x70 [ib_srpt] > srpt_disconnect_ch+0x1b/0xc0 [ib_srpt] > srpt_close_session+0xa8/0x260 [ib_srpt] > target_shutdown_sessions+0x170/0x180 [target_core_mod] > core_tpg_del_initiator_node_acl+0xf3/0x200 [target_core_mod] > target_fabric_nacl_base_release+0x25/0x30 [target_core_mod] > config_item_release+0x9c/0x110 [configfs] > config_item_put+0x26/0x30 [configfs] > configfs_rmdir+0x3b8/0x510 [configfs] > vfs_rmdir+0xb3/0x1e0 > do_rmdir+0x262/0x2c0 > do_syscall_64+0x77/0x230 > entry_SYSCALL_64_after_hwframe+0x49/0xbe > > Fixes: a42d985bd5b2 ("ib_srpt: Initial SRP Target merge for v3.3-rc1") > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> > Cc: <stable@vger.kernel.org> > --- > drivers/infiniband/ulp/srpt/ib_srpt.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c > index 325bae29e90d..705f6a992d82 100644 > --- a/drivers/infiniband/ulp/srpt/ib_srpt.c > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c > @@ -2152,6 +2152,7 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev, > } > > kref_init(&ch->kref); > + kref_get(&ch->kref); kref_init starts the reference count at at 1, so why do you need to increment it right away? That feels like something is "odd" here, why do you start with 2 references in the same function? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
T24gVHVlLCAyMDE4LTA3LTEwIGF0IDEyOjM4IC0wNjAwLCBKYXNvbiBHdW50aG9ycGUgd3JvdGU6 DQo+IE9uIFR1ZSwgSnVsIDEwLCAyMDE4IGF0IDEwOjMyOjAwQU0gLTA3MDAsIEJhcnQgVmFuIEFz c2NoZSB3cm90ZToNCj4gPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9pbmZpbmliYW5kL3VscC9zcnB0 L2liX3NycHQuYyBiL2RyaXZlcnMvaW5maW5pYmFuZC91bHAvc3JwdC9pYl9zcnB0LmMNCj4gPiBp bmRleCAzMjViYWUyOWU5MGQuLjcwNWY2YTk5MmQ4MiAxMDA2NDQNCj4gPiArKysgYi9kcml2ZXJz L2luZmluaWJhbmQvdWxwL3NycHQvaWJfc3JwdC5jDQo+ID4gQEAgLTIxNTIsNiArMjE1Miw3IEBA IHN0YXRpYyBpbnQgc3JwdF9jbV9yZXFfcmVjdihzdHJ1Y3Qgc3JwdF9kZXZpY2UgKmNvbnN0IHNk ZXYsDQo+ID4gIAl9DQo+ID4gDQo+ID4gIAlrcmVmX2luaXQoJmNoLT5rcmVmKTsNCj4gPiArCWty ZWZfZ2V0KCZjaC0+a3JlZik7DQo+IA0KPiBPaCB0aGF0IGlzIHVnbHksIGNhbiB5b3UgcHV0IHRo ZSAnZ2V0JyBjbG9zZXIgdG8gdGhlIHRoZSBwbGFjZSB0aGF0DQo+IHN0b3JlcyB0aGUgY2ggcG9p bnRlciB0aGlzIGtyZWYgaXMgcHJvdGVjdGluZz8gUGVyaGFwcyBpdCBpcyBvbmUgb2YNCj4gdGhl IGxpc3RfYWRkJ3MgaW4gdGhpcyBmdW5jdGlvbj8NCj4gDQo+IEd1ZXNzaW5nIHRoZSBrcmVmIGNy ZWF0ZWQga3JlZl9pbml0IHNob3VsZCBwcm9iYWJseSBiZWxvbmcgdG8gdGFyZ2V0X2NvcmUncw0K PiAnc2Vfc2Vzcy0+ZmFicmljX3Nlc3NfcHRyJyAuLg0KDQpIZWxsbyBKYXNvbiwNCg0KQ2FuIHlv dSBjbGFyaWZ5IHlvdXIgc2Vjb25kIHBhcmFncmFwaD8gVGhlIGliX3NycHQgZHJpdmVyIGhhcyBi ZWVuIGltcGxlbWVudGVkDQpzdWNoIHRoYXQgc2Vzcy0+ZmFicmljX3Nlc3NfcHRyID09IGNoIGFz IGxvbmcgYXMgYSBzZXNzaW9uIGV4aXN0cy4NCg0KQW4gaWJfc3JwdCBSRE1BIGNoYW5uZWwgb2Jq ZWN0IChjaCBpbiB0aGUgYWJvdmUgY29kZSkgbXVzdCBzdGF5IGFyb3VuZCBhcyBsb25nDQphcyB0 aGUgYXNzb2NpYXRlZCB0YXJnZXQgY29yZSBzZXNzaW9uIChzZV9zZXNzKSBleGlzdHMgYW5kIGFs c28gYXMgbG9uZyBhcyB0aGUNCnRhcmdldCBjb3JlIGhhcyBub3QgeWV0IGNhbGxlZCBzcnB0X2Ns b3NlX3Nlc3Npb24oKS4gSGVuY2UgdGhlIGluaXRpYWxpemF0aW9uIG9mDQpjaC0+a3JlZiB0byAy IGp1c3QgYmVmb3JlIGFuIFJETUEgY2hhbm5lbCBpcyByZWdpc3RlcmVkIHdpdGggdGhlIHRhcmdl dCBjb3JlLg0KSGVuY2UgYWxzbyB0aGUga3JlZl9wdXQoKSBjYWxscyBpbiBzcnB0X2Nsb3NlX3Nl c3Npb24oKSBhbmQNCnNycHRfcmVsZWFzZV9jaGFubmVsX3dvcmsoKS4NCg0KQmFydC4NCg0KDQoN Cg0K -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
T24gVHVlLCAyMDE4LTA3LTEwIGF0IDIwOjUxICswMjAwLCBHcmVnIEtIIHdyb3RlOg0KPiBPbiBU dWUsIEp1bCAxMCwgMjAxOCBhdCAxMDozMjowMEFNIC0wNzAwLCBCYXJ0IFZhbiBBc3NjaGUgd3Jv dGU6DQo+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvaW5maW5pYmFuZC91bHAvc3JwdC9pYl9zcnB0 LmMgYi9kcml2ZXJzL2luZmluaWJhbmQvdWxwL3NycHQvaWJfc3JwdC5jDQo+ID4gaW5kZXggMzI1 YmFlMjllOTBkLi43MDVmNmE5OTJkODIgMTAwNjQ0DQo+ID4gLS0tIGEvZHJpdmVycy9pbmZpbmli YW5kL3VscC9zcnB0L2liX3NycHQuYw0KPiA+ICsrKyBiL2RyaXZlcnMvaW5maW5pYmFuZC91bHAv c3JwdC9pYl9zcnB0LmMNCj4gPiBAQCAtMjE1Miw2ICsyMTUyLDcgQEAgc3RhdGljIGludCBzcnB0 X2NtX3JlcV9yZWN2KHN0cnVjdCBzcnB0X2RldmljZSAqY29uc3Qgc2RldiwNCj4gPiAgCX0NCj4g PiAgDQo+ID4gIAlrcmVmX2luaXQoJmNoLT5rcmVmKTsNCj4gPiArCWtyZWZfZ2V0KCZjaC0+a3Jl Zik7DQo+IA0KPiBrcmVmX2luaXQgc3RhcnRzIHRoZSByZWZlcmVuY2UgY291bnQgYXQgYXQgMSwg c28gd2h5IGRvIHlvdSBuZWVkIHRvDQo+IGluY3JlbWVudCBpdCByaWdodCBhd2F5PyAgVGhhdCBm ZWVscyBsaWtlIHNvbWV0aGluZyBpcyAib2RkIiBoZXJlLCB3aHkNCj4gZG8geW91IHN0YXJ0IHdp dGggMiByZWZlcmVuY2VzIGluIHRoZSBzYW1lIGZ1bmN0aW9uPw0KDQpIaSBHcmVnLA0KDQpBbiBp Yl9zcnB0IFJETUEgY2hhbm5lbCBvYmplY3QgKGNoIGluIHRoZSBhYm92ZSBjb2RlKSBtdXN0IHN0 YXkgYXJvdW5kIGFzIGxvbmcNCmFzIHRoZSBhc3NvY2lhdGVkIHRhcmdldCBjb3JlIHNlc3Npb24g KHNlX3Nlc3MpIGV4aXN0cyBhbmQgYWxzbyBhcyBsb25nIGFzIHRoZQ0KdGFyZ2V0IGNvcmUgaGFz IG5vdCB5ZXQgY2FsbGVkIHNycHRfY2xvc2Vfc2Vzc2lvbigpLiBIZW5jZSB0aGUgaW5pdGlhbGl6 YXRpb24gb2YNCmNoLT5rcmVmIHRvIDIganVzdCBiZWZvcmUgYW4gUkRNQSBjaGFubmVsIGlzIHJl Z2lzdGVyZWQgd2l0aCB0aGUgdGFyZ2V0IGNvcmUuDQpIZW5jZSBhbHNvIHRoZSBrcmVmX3B1dCgp IGNhbGxzIGluIHNycHRfY2xvc2Vfc2Vzc2lvbigpIGFuZA0Kc3JwdF9yZWxlYXNlX2NoYW5uZWxf d29yaygpLiBQbGVhc2UgbGV0IG1lIGtub3cgaWYgeW91IG5lZWQgbW9yZSBpbmZvcm1hdGlvbi4N Cg0KVGhhbmtzLA0KDQpCYXJ0Lg0KDQoNCg0K -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 10, 2018 at 08:23:01PM +0000, Bart Van Assche wrote: > On Tue, 2018-07-10 at 20:51 +0200, Greg KH wrote: > > On Tue, Jul 10, 2018 at 10:32:00AM -0700, Bart Van Assche wrote: > > > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c > > > index 325bae29e90d..705f6a992d82 100644 > > > --- a/drivers/infiniband/ulp/srpt/ib_srpt.c > > > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c > > > @@ -2152,6 +2152,7 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev, > > > } > > > > > > kref_init(&ch->kref); > > > + kref_get(&ch->kref); > > > > kref_init starts the reference count at at 1, so why do you need to > > increment it right away? That feels like something is "odd" here, why > > do you start with 2 references in the same function? > > Hi Greg, > > An ib_srpt RDMA channel object (ch in the above code) must stay around as long > as the associated target core session (se_sess) exists and also as long as the > target core has not yet called srpt_close_session(). Hence the initialization of > ch->kref to 2 just before an RDMA channel is registered with the target core. Shouldn't the registration increment the reference? Starting out at "2" feels very "odd", don't you agree? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
T24gVHVlLCAyMDE4LTA3LTEwIGF0IDIyOjQ0ICswMjAwLCBncmVna2hAbGludXhmb3VuZGF0aW9u Lm9yZyB3cm90ZToNCj4gT24gVHVlLCBKdWwgMTAsIDIwMTggYXQgMDg6MjM6MDFQTSArMDAwMCwg QmFydCBWYW4gQXNzY2hlIHdyb3RlOg0KPiA+IE9uIFR1ZSwgMjAxOC0wNy0xMCBhdCAyMDo1MSAr MDIwMCwgR3JlZyBLSCB3cm90ZToNCj4gPiA+IE9uIFR1ZSwgSnVsIDEwLCAyMDE4IGF0IDEwOjMy OjAwQU0gLTA3MDAsIEJhcnQgVmFuIEFzc2NoZSB3cm90ZToNCj4gPiA+ID4gZGlmZiAtLWdpdCBh L2RyaXZlcnMvaW5maW5pYmFuZC91bHAvc3JwdC9pYl9zcnB0LmMgYi9kcml2ZXJzL2luZmluaWJh bmQvdWxwL3NycHQvaWJfc3JwdC5jDQo+ID4gPiA+IGluZGV4IDMyNWJhZTI5ZTkwZC4uNzA1ZjZh OTkyZDgyIDEwMDY0NA0KPiA+ID4gPiAtLS0gYS9kcml2ZXJzL2luZmluaWJhbmQvdWxwL3NycHQv aWJfc3JwdC5jDQo+ID4gPiA+ICsrKyBiL2RyaXZlcnMvaW5maW5pYmFuZC91bHAvc3JwdC9pYl9z cnB0LmMNCj4gPiA+ID4gQEAgLTIxNTIsNiArMjE1Miw3IEBAIHN0YXRpYyBpbnQgc3JwdF9jbV9y ZXFfcmVjdihzdHJ1Y3Qgc3JwdF9kZXZpY2UgKmNvbnN0IHNkZXYsDQo+ID4gPiA+ICAJfQ0KPiA+ ID4gPiAgDQo+ID4gPiA+ICAJa3JlZl9pbml0KCZjaC0+a3JlZik7DQo+ID4gPiA+ICsJa3JlZl9n ZXQoJmNoLT5rcmVmKTsNCj4gPiA+IA0KPiA+ID4ga3JlZl9pbml0IHN0YXJ0cyB0aGUgcmVmZXJl bmNlIGNvdW50IGF0IGF0IDEsIHNvIHdoeSBkbyB5b3UgbmVlZCB0bw0KPiA+ID4gaW5jcmVtZW50 IGl0IHJpZ2h0IGF3YXk/ICBUaGF0IGZlZWxzIGxpa2Ugc29tZXRoaW5nIGlzICJvZGQiIGhlcmUs IHdoeQ0KPiA+ID4gZG8geW91IHN0YXJ0IHdpdGggMiByZWZlcmVuY2VzIGluIHRoZSBzYW1lIGZ1 bmN0aW9uPw0KPiA+IA0KPiA+IEFuIGliX3NycHQgUkRNQSBjaGFubmVsIG9iamVjdCAoY2ggaW4g dGhlIGFib3ZlIGNvZGUpIG11c3Qgc3RheSBhcm91bmQgYXMgbG9uZw0KPiA+IGFzIHRoZSBhc3Nv Y2lhdGVkIHRhcmdldCBjb3JlIHNlc3Npb24gKHNlX3Nlc3MpIGV4aXN0cyBhbmQgYWxzbyBhcyBs b25nIGFzIHRoZQ0KPiA+IHRhcmdldCBjb3JlIGhhcyBub3QgeWV0IGNhbGxlZCBzcnB0X2Nsb3Nl X3Nlc3Npb24oKS4gSGVuY2UgdGhlIGluaXRpYWxpemF0aW9uIG9mDQo+ID4gY2gtPmtyZWYgdG8g MiBqdXN0IGJlZm9yZSBhbiBSRE1BIGNoYW5uZWwgaXMgcmVnaXN0ZXJlZCB3aXRoIHRoZSB0YXJn ZXQgY29yZS4NCj4gDQo+IFNob3VsZG4ndCB0aGUgcmVnaXN0cmF0aW9uIGluY3JlbWVudCB0aGUg cmVmZXJlbmNlPyAgU3RhcnRpbmcgb3V0IGF0ICIyIg0KPiBmZWVscyB2ZXJ5ICJvZGQiLCBkb24n dCB5b3UgYWdyZWU/DQoNCkhlbGxvIEdyZWcsDQoNClRoZSBjb2RlIHRoYXQgcmVnaXN0ZXJzIHRo ZSBzZXNzaW9uIHdpdGggdGhlIHRhcmdldCBjb3JlIGlzIGluIGFub3RoZXIgZHJpdmVyDQooU0NT SSB0YXJnZXQgY29yZSkgYW5kIHRoYXQgZHJpdmVyIGRvZXNuJ3Qga25vdyBhYm91dCB0aGUgYWJz dHJhY3Rpb25zIG1haW50YWluZWQNCmJ5IHRoZSBpYl9zcnB0IGRyaXZlci4gVGhhdCdzIHdoeSB0 aGUgYWJvdmUga3JlZl9nZXQoKSBjYWxsIGlzIGluIHRoZSBpYl9zcnB0DQpkcml2ZXIgYW5kIG5v dCBlLmcuIGluIHRoZSB0YXJnZXRfYWxsb2Nfc2Vzc2lvbigpIGZ1bmN0aW9uLg0KDQpCVFcsIHRo ZXJlIGlzIG1vcmUgY29kZSBpbiB0aGUgTGludXgga2VybmVsIHRoYXQgZm9sbG93cyB0aGUgYWJv dmUgcGF0dGVybi4NCnRhcmdldF9zdWJtaXRfY21kX21hcF9zZ2xzKCkgaW5pdGlhbGl6ZXMgdGhl IHNlX2NtZCByZWZlcmVuY2UgY291bnQgdG8gdHdvIGFzDQpmb2xsb3dzOg0KKiB0cmFuc3BvcnRf aW5pdF9zZV9jbWQoKSBpbml0aWFsaXplcyBpdCB0byBvbmUuDQoqIHRhcmdldF9nZXRfc2Vzc19j bWQoKSBpbmNyZW1lbnRzIGl0IGZyb20gb25lIHRvIHR3by4NClRoaXMgaXMgYmVjYXVzZSB0d28g Y29udGV4dHMga2VlcCBhIHJlZmVyZW5jZSB0byBzZV9jbWQgZGF0YSBzdHJ1Y3R1cmVzLA0KbmFt ZWx5IHRoZSB0YXJnZXQgY29yZSBhbmQgdGhlIHRhcmdldCBkcml2ZXIuIEEgU0NTSSB0YXJnZXQg Y29tbWFuZCBkYXRhDQpzdHJ1Y3R1cmUgKHNlX2NtZCkgbXVzdCBvbmx5IGJlIGZyZWVkIGFmdGVy IGJvdGggY29udGV4dHMgaGF2ZSBmaW5pc2hlZCB0aGVpcg0KcGFydCBvZiB0aGUgY29tbWFuZCBw cm9jZXNzaW5nLg0KDQpUaGFua3MsDQoNCkJhcnQuDQoNCg0K -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 10, 2018 at 08:19:20PM +0000, Bart Van Assche wrote: > On Tue, 2018-07-10 at 12:38 -0600, Jason Gunthorpe wrote: > > On Tue, Jul 10, 2018 at 10:32:00AM -0700, Bart Van Assche wrote: > > > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c > > > index 325bae29e90d..705f6a992d82 100644 > > > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c > > > @@ -2152,6 +2152,7 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev, > > > } > > > > > > kref_init(&ch->kref); > > > + kref_get(&ch->kref); > > > > Oh that is ugly, can you put the 'get' closer to the the place that > > stores the ch pointer this kref is protecting? Perhaps it is one of > > the list_add's in this function? > > > > Guessing the kref created kref_init should probably belong to target_core's > > 'se_sess->fabric_sess_ptr' .. > > Hello Jason, > > Can you clarify your second paragraph? The ib_srpt driver has been implemented > such that sess->fabric_sess_ptr == ch as long as a session exists. In my view that means sess->fabric_sess_ptr is the pointer that 'owns' the kref_init's value - ie the kref_init pairs with the kref_put added in your patch. The question here, is what kref_put does the 2nd kref_get pair with? When you identify that, then move the kref_get closer to the assignment to the 'owning' pointer. krefs are very tricky, I find it is best to be very rigorous with them, and place some notes about how they pair and what storage is 'owning' each kref. eg if you put something in a list, and incr the kref when doing so, then place the kref_get near the list_add, and the kref_put near the list_del. That gives somebody else a chance to understand that the list_head is holding the kref. And you have to be careful when sharing the stack kref with something else - eg if a list_add 'moves' a stack kref into a list then another thread can race and do list_del and put, even though the first thread is still accessing the memory on its stack. Often times these 'publish' actions must be last in a function, or more krefs are needed. This is a really annoying bug class with krefs I find from time to time.. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 10, 2018 at 09:02:51PM +0000, Bart Van Assche wrote: > On Tue, 2018-07-10 at 22:44 +0200, gregkh@linuxfoundation.org wrote: > > On Tue, Jul 10, 2018 at 08:23:01PM +0000, Bart Van Assche wrote: > > > On Tue, 2018-07-10 at 20:51 +0200, Greg KH wrote: > > > > On Tue, Jul 10, 2018 at 10:32:00AM -0700, Bart Van Assche wrote: > > > > > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c > > > > > index 325bae29e90d..705f6a992d82 100644 > > > > > --- a/drivers/infiniband/ulp/srpt/ib_srpt.c > > > > > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c > > > > > @@ -2152,6 +2152,7 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev, > > > > > } > > > > > > > > > > kref_init(&ch->kref); > > > > > + kref_get(&ch->kref); > > > > > > > > kref_init starts the reference count at at 1, so why do you need to > > > > increment it right away? That feels like something is "odd" here, why > > > > do you start with 2 references in the same function? > > > > > > An ib_srpt RDMA channel object (ch in the above code) must stay around as long > > > as the associated target core session (se_sess) exists and also as long as the > > > target core has not yet called srpt_close_session(). Hence the initialization of > > > ch->kref to 2 just before an RDMA channel is registered with the target core. > > > > Shouldn't the registration increment the reference? Starting out at "2" > > feels very "odd", don't you agree? > > Hello Greg, > > The code that registers the session with the target core is in another driver > (SCSI target core) and that driver doesn't know about the abstractions maintained > by the ib_srpt driver. Ok, but then that registration shouldn't be dropping a reference either, right? > That's why the above kref_get() call is in the ib_srpt > driver and not e.g. in the target_alloc_session() function. But I still do not understand why you have 2 on the count here. Why do you need that? > BTW, there is more code in the Linux kernel that follows the above pattern. > target_submit_cmd_map_sgls() initializes the se_cmd reference count to two as > follows: > * transport_init_se_cmd() initializes it to one. > * target_get_sess_cmd() increments it from one to two. That is two different areas of the code, right? That is not "initialize the count to 2 when we first create it". > This is because two contexts keep a reference to se_cmd data structures, > namely the target core and the target driver. A SCSI target command data > structure (se_cmd) must only be freed after both contexts have finished their > part of the command processing. It's your subsytem, I'm just trying to point out that this pattern you have created is very odd and is probably wrong. If you all insist it is correct, that's great, but I did warn you :) good luck! greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c index 325bae29e90d..705f6a992d82 100644 --- a/drivers/infiniband/ulp/srpt/ib_srpt.c +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c @@ -2152,6 +2152,7 @@ static int srpt_cm_req_recv(struct srpt_device *const sdev, } kref_init(&ch->kref); + kref_get(&ch->kref); ch->pkey = be16_to_cpu(pkey); ch->nexus = nexus; ch->zw_cqe.done = srpt_zerolength_write_done; @@ -3212,6 +3213,7 @@ static void srpt_close_session(struct se_session *se_sess) struct srpt_rdma_ch *ch = se_sess->fabric_sess_ptr; srpt_disconnect_ch_sync(ch); + kref_put(&ch->kref, srpt_free_ch); } /**
Make sure that channel objects continue to exist until the target core has called the .close_session() callback function. This patch voids that KASAN sporadically reports the following: BUG: KASAN: use-after-free in do_raw_spin_lock+0x1c/0x130 Read of size 4 at addr ffff8801534b16e4 by task rmdir/14805 CPU: 16 PID: 14805 Comm: rmdir Not tainted 4.18.0-rc2-dbg+ #5 Call Trace: dump_stack+0xa4/0xf5 print_address_description+0x6f/0x270 kasan_report+0x241/0x360 __asan_load4+0x78/0x80 do_raw_spin_lock+0x1c/0x130 _raw_spin_lock_irqsave+0x52/0x60 srpt_set_ch_state+0x27/0x70 [ib_srpt] srpt_disconnect_ch+0x1b/0xc0 [ib_srpt] srpt_close_session+0xa8/0x260 [ib_srpt] target_shutdown_sessions+0x170/0x180 [target_core_mod] core_tpg_del_initiator_node_acl+0xf3/0x200 [target_core_mod] target_fabric_nacl_base_release+0x25/0x30 [target_core_mod] config_item_release+0x9c/0x110 [configfs] config_item_put+0x26/0x30 [configfs] configfs_rmdir+0x3b8/0x510 [configfs] vfs_rmdir+0xb3/0x1e0 do_rmdir+0x262/0x2c0 do_syscall_64+0x77/0x230 entry_SYSCALL_64_after_hwframe+0x49/0xbe Fixes: a42d985bd5b2 ("ib_srpt: Initial SRP Target merge for v3.3-rc1") Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> Cc: <stable@vger.kernel.org> --- drivers/infiniband/ulp/srpt/ib_srpt.c | 2 ++ 1 file changed, 2 insertions(+)