diff mbox

tcmu: Fix possbile memory leak when recalculating the cmd base size

Message ID 1499760357-15120-1-git-send-email-lixiubo@cmss.chinamobile.com (mailing list archive)
State New, archived
Headers show

Commit Message

Xiubo Li July 11, 2017, 8:05 a.m. UTC 
From: Xiubo Li <lixiubo@cmss.chinamobile.com>

For all the entries allocated from the ring cmd area, the memory
is something like the stack, which will reserve the old data, so
the entry->req.iov_bidi_cnt maybe none zero.

To fix this, just memset all the entry memory before using it, and
also to be more readable we adjust the bidi code.

Fixed: fe25cc34795(tcmu: Recalculate the tcmu_cmd size to save cmd area
		memories)
Reported-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
Signed-off-by: Xiubo Li <lixiubo@cmss.chinamobile.com>
---
 drivers/target/target_core_user.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Xiubo Li July 11, 2017, 9:06 a.m. UTC | #1 
To Damien,

Please test this, I think this maybe helpful.


Thanks,

BRs



On 2017年07月11日 16:05, lixiubo@cmss.chinamobile.com wrote:
> From: Xiubo Li <lixiubo@cmss.chinamobile.com>
>
> For all the entries allocated from the ring cmd area, the memory
> is something like the stack, which will reserve the old data, so
> the entry->req.iov_bidi_cnt maybe none zero.
>
> To fix this, just memset all the entry memory before using it, and
> also to be more readable we adjust the bidi code.
>
> Fixed: fe25cc34795(tcmu: Recalculate the tcmu_cmd size to save cmd area
> 		memories)
> Reported-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
> Signed-off-by: Xiubo Li <lixiubo@cmss.chinamobile.com>
> ---
>   drivers/target/target_core_user.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
> index 2f1fa92..be62c86 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -840,6 +840,7 @@ static inline size_t tcmu_cmd_get_cmd_size(struct tcmu_cmd *tcmu_cmd,
>   	}
>   
>   	entry = (void *) mb + CMDR_OFF + cmd_head;
> +	memset(entry, 0, command_size);
>   	tcmu_hdr_set_op(&entry->hdr.len_op, TCMU_OP_CMD);
>   	entry->hdr.cmd_id = tcmu_cmd->cmd_id;
>   	entry->hdr.kflags = 0;
> @@ -865,8 +866,8 @@ static inline size_t tcmu_cmd_get_cmd_size(struct tcmu_cmd *tcmu_cmd,
>   	entry->req.iov_dif_cnt = 0;
>   
>   	/* Handle BIDI commands */
> +	iov_cnt = 0;
>   	if (se_cmd->se_cmd_flags & SCF_BIDI) {
> -		iov_cnt = 0;
>   		iov++;
>   		ret = scatter_data_area(udev, tcmu_cmd,
>   					se_cmd->t_bidi_data_sg,
> @@ -879,8 +880,8 @@ static inline size_t tcmu_cmd_get_cmd_size(struct tcmu_cmd *tcmu_cmd,
>   			pr_err("tcmu: alloc and scatter bidi data failed\n");
>   			return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>   		}
> -		entry->req.iov_bidi_cnt = iov_cnt;
>   	}
> +	entry->req.iov_bidi_cnt = iov_cnt;
>   
>   	/*
>   	 * Recalaulate the command's base size and size according



--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Damien Le Moal July 11, 2017, 9:24 a.m. UTC | #2 
WGl1Ym8sDQoNCldlbGwgZG9uZSAhIFRoaXMgZml4ZWQgbXkgcHJvYmxlbS4gVGhlIFpCQyB0ZXN0
IHN1aXRlIG5vdyBwYXNzZXMgYWxsIHRlc3RzIG9uDQpteSB0YXJnZXQgd2l0aG91dCBjcmFzaGlu
ZyB0aGUga2VybmVsLg0KDQpQbGVhc2Ugc2VlIHNvbWUgY29tbWVudHMvbml0cGlja3MgYmVsb3cu
DQoNCk90aGVyd2lzZSwgcGxlYXNlIGZlZWwgZnJlZSB0byBhZGQgbXkgInRlc3RlZC1ieSINCg0K
DQpPbiBUdWUsIDIwMTctMDctMTEgYXQgMTc6MDYgKzA4MDAsIFhpdWJvIExpIHdyb3RlOg0KPiBP
biAyMDE35bm0MDfmnIgxMeaXpSAxNjowNSwgbGl4aXVib0BjbXNzLmNoaW5hbW9iaWxlLmNvbSB3
cm90ZToNCj4gPiBGcm9tOiBYaXVibyBMaSA8bGl4aXVib0BjbXNzLmNoaW5hbW9iaWxlLmNvbT4N
Cj4gPiANCj4gPiBGb3IgYWxsIHRoZSBlbnRyaWVzIGFsbG9jYXRlZCBmcm9tIHRoZSByaW5nIGNt
ZCBhcmVhLCB0aGUgbWVtb3J5DQo+ID4gaXMgc29tZXRoaW5nIGxpa2UgdGhlIHN0YWNrLCB3aGlj
aCB3aWxsIHJlc2VydmUgdGhlIG9sZCBkYXRhLCBzbw0KPiA+IHRoZSBlbnRyeS0+cmVxLmlvdl9i
aWRpX2NudCBtYXliZSBub25lIHplcm8uDQo+ID4gDQo+ID4gVG8gZml4IHRoaXMsIGp1c3QgbWVt
c2V0IGFsbCB0aGUgZW50cnkgbWVtb3J5IGJlZm9yZSB1c2luZyBpdCwgYW5kDQo+ID4gYWxzbyB0
byBiZSBtb3JlIHJlYWRhYmxlIHdlIGFkanVzdCB0aGUgYmlkaSBjb2RlLg0KPiA+IA0KPiA+IEZp
eGVkOiBmZTI1Y2MzNDc5NSh0Y211OiBSZWNhbGN1bGF0ZSB0aGUgdGNtdV9jbWQgc2l6ZSB0byBz
YXZlIGNtZCBhcmVhDQo+ID4gCQltZW1vcmllcykNCj4gPiBSZXBvcnRlZC1ieTogQnJ5YW50IEcu
IEx5IDxicnlhbnRseUBsaW51eC52bmV0LmlibS5jb20+DQo+ID4gU2lnbmVkLW9mZi1ieTogWGl1
Ym8gTGkgPGxpeGl1Ym9AY21zcy5jaGluYW1vYmlsZS5jb20+DQo+ID4gLS0tDQo+ID4gwqAgZHJp
dmVycy90YXJnZXQvdGFyZ2V0X2NvcmVfdXNlci5jIHwgNSArKystLQ0KPiA+IMKgIDEgZmlsZSBj
aGFuZ2VkLCAzIGluc2VydGlvbnMoKyksIDIgZGVsZXRpb25zKC0pDQo+ID4gDQo+ID4gZGlmZiAt
LWdpdCBhL2RyaXZlcnMvdGFyZ2V0L3RhcmdldF9jb3JlX3VzZXIuYw0KPiA+IGIvZHJpdmVycy90
YXJnZXQvdGFyZ2V0X2NvcmVfdXNlci5jDQo+ID4gaW5kZXggMmYxZmE5Mi4uYmU2MmM4NiAxMDA2
NDQNCj4gPiAtLS0gYS9kcml2ZXJzL3RhcmdldC90YXJnZXRfY29yZV91c2VyLmMNCj4gPiArKysg
Yi9kcml2ZXJzL3RhcmdldC90YXJnZXRfY29yZV91c2VyLmMNCj4gPiBAQCAtODQwLDYgKzg0MCw3
IEBAIHN0YXRpYyBpbmxpbmUgc2l6ZV90IHRjbXVfY21kX2dldF9jbWRfc2l6ZShzdHJ1Y3QNCj4g
PiB0Y211X2NtZCAqdGNtdV9jbWQsDQo+ID4gwqDCoAl9DQo+ID4gwqDCoA0KPiA+IMKgwqAJZW50
cnkgPSAodm9pZCAqKSBtYiArIENNRFJfT0ZGICsgY21kX2hlYWQ7DQo+ID4gKwltZW1zZXQoZW50
cnksIDAsIGNvbW1hbmRfc2l6ZSk7DQo+ID4gwqDCoAl0Y211X2hkcl9zZXRfb3AoJmVudHJ5LT5o
ZHIubGVuX29wLCBUQ01VX09QX0NNRCk7DQo+ID4gwqDCoAllbnRyeS0+aGRyLmNtZF9pZCA9IHRj
bXVfY21kLT5jbWRfaWQ7DQo+ID4gwqDCoAllbnRyeS0+aGRyLmtmbGFncyA9IDA7DQoNClRoZSBh
ZGRlZCBtZW1zZXQgYWxsb3dzIHJlbW92aW5nIHRoZSAwIGFzc2lnbm1lbnQgaGVyZSwgYW5kIHRo
ZSBvbmUgdGhhdA0KZm9sbG93cyB0aGlzIG9uZSB0b28uDQoNCj4gPiBAQCAtODY1LDggKzg2Niw4
IEBAIHN0YXRpYyBpbmxpbmUgc2l6ZV90IHRjbXVfY21kX2dldF9jbWRfc2l6ZShzdHJ1Y3QNCj4g
PiB0Y211X2NtZCAqdGNtdV9jbWQsDQo+ID4gwqDCoAllbnRyeS0+cmVxLmlvdl9kaWZfY250ID0g
MDsNCj4gPiDCoMKgDQo+ID4gwqDCoAkvKiBIYW5kbGUgQklESSBjb21tYW5kcyAqLw0KPiA+ICsJ
aW92X2NudCA9IDA7DQo+ID4gwqDCoAlpZiAoc2VfY21kLT5zZV9jbWRfZmxhZ3MgJiBTQ0ZfQklE
SSkgew0KPiA+IC0JCWlvdl9jbnQgPSAwOw0KPiA+IMKgwqAJCWlvdisrOw0KPiA+IMKgwqAJCXJl
dCA9IHNjYXR0ZXJfZGF0YV9hcmVhKHVkZXYsIHRjbXVfY21kLA0KPiA+IMKgwqAJCQkJCXNlX2Nt
ZC0+dF9iaWRpX2RhdGFfc2csDQo+ID4gQEAgLTg3OSw4ICs4ODAsOCBAQCBzdGF0aWMgaW5saW5l
IHNpemVfdCB0Y211X2NtZF9nZXRfY21kX3NpemUoc3RydWN0DQo+ID4gdGNtdV9jbWQgKnRjbXVf
Y21kLA0KPiA+IMKgwqAJCQlwcl9lcnIoInRjbXU6IGFsbG9jIGFuZCBzY2F0dGVyIGJpZGkgZGF0
YQ0KPiA+IGZhaWxlZFxuIik7DQo+ID4gwqDCoAkJCXJldHVybiBUQ01fTE9HSUNBTF9VTklUX0NP
TU1VTklDQVRJT05fRkFJTFVSRTsNCj4gPiDCoMKgCQl9DQo+ID4gLQkJZW50cnktPnJlcS5pb3Zf
YmlkaV9jbnQgPSBpb3ZfY250Ow0KPiA+IMKgwqAJfQ0KPiA+ICsJZW50cnktPnJlcS5pb3ZfYmlk
aV9jbnQgPSBpb3ZfY250Ow0KPiA+IMKgwqANCj4gPiDCoMKgCS8qDQo+ID4gwqDCoAnCoCogUmVj
YWxhdWxhdGUgdGhlIGNvbW1hbmQncyBiYXNlIHNpemUgYW5kIHNpemUgYWNjb3JkaW5nDQoNCkZv
ciByZWZlcmVuY2UsIGhlcmUgaXMgdGhlIGFjdHVhbCBwYXRjaCBJIHVzZWQgZm9yIHRlc3Rpbmc6
DQoNCmRpZmYgLS1naXQgYS9kcml2ZXJzL3RhcmdldC90YXJnZXRfY29yZV91c2VyLmMNCmIvZHJp
dmVycy90YXJnZXQvdGFyZ2V0X2NvcmVfdXNlci5jDQppbmRleCAyZjFmYTkyLi4zYjI1ZWYzIDEw
MDY0NA0KLS0tIGEvZHJpdmVycy90YXJnZXQvdGFyZ2V0X2NvcmVfdXNlci5jDQorKysgYi9kcml2
ZXJzL3RhcmdldC90YXJnZXRfY29yZV91c2VyLmMNCkBAIC01NjMsNyArNTYzLDcgQEAgc3RhdGlj
IGludCBzY2F0dGVyX2RhdGFfYXJlYShzdHJ1Y3QgdGNtdV9kZXYgKnVkZXYsDQrCoAkJCXRvX29m
ZnNldCA9IGdldF9ibG9ja19vZmZzZXRfdXNlcih1ZGV2LCBkYmksDQrCoAkJCQkJYmxvY2tfcmVt
YWluaW5nKTsNCsKgCQkJb2Zmc2V0ID0gREFUQV9CTE9DS19TSVpFIC0gYmxvY2tfcmVtYWluaW5n
Ow0KLQkJCXRvID0gKHZvaWQgKikodW5zaWduZWQgbG9uZyl0byArIG9mZnNldDsNCisJCQl0byAr
PSBvZmZzZXQ7DQrCoA0KwqAJCQlpZiAoKmlvdl9jbnQgIT0gMCAmJg0KwqAJCQnCoMKgwqDCoHRv
X29mZnNldCA9PSBpb3ZfdGFpbCh1ZGV2LCAqaW92KSkgew0KQEAgLTYzNiw3ICs2MzYsNyBAQCBz
dGF0aWMgdm9pZCBnYXRoZXJfZGF0YV9hcmVhKHN0cnVjdCB0Y211X2RldiAqdWRldiwgc3RydWN0
DQp0Y211X2NtZCAqY21kLA0KwqAJCQljb3B5X2J5dGVzID0gbWluX3Qoc2l6ZV90LCBzZ19yZW1h
aW5pbmcsDQrCoAkJCQkJYmxvY2tfcmVtYWluaW5nKTsNCsKgCQkJb2Zmc2V0ID0gREFUQV9CTE9D
S19TSVpFIC0gYmxvY2tfcmVtYWluaW5nOw0KLQkJCWZyb20gPSAodm9pZCAqKSh1bnNpZ25lZCBs
b25nKWZyb20gKyBvZmZzZXQ7DQorCQkJZnJvbSArPSBvZmZzZXQ7DQrCoAkJCXRjbXVfZmx1c2hf
ZGNhY2hlX3JhbmdlKGZyb20sIGNvcHlfYnl0ZXMpOw0KwqAJCQltZW1jcHkodG8gKyBzZy0+bGVu
Z3RoIC0gc2dfcmVtYWluaW5nLCBmcm9tLA0KwqAJCQkJCWNvcHlfYnl0ZXMpOw0KQEAgLTg0MCwx
MCArODQwLDkgQEAgdGNtdV9xdWV1ZV9jbWRfcmluZyhzdHJ1Y3QgdGNtdV9jbWQgKnRjbXVfY21k
KQ0KwqAJfQ0KwqANCsKgCWVudHJ5ID0gKHZvaWQgKikgbWIgKyBDTURSX09GRiArIGNtZF9oZWFk
Ow0KKwltZW1zZXQoZW50cnksIDAsIGNvbW1hbmRfc2l6ZSk7DQrCoAl0Y211X2hkcl9zZXRfb3Ao
JmVudHJ5LT5oZHIubGVuX29wLCBUQ01VX09QX0NNRCk7DQrCoAllbnRyeS0+aGRyLmNtZF9pZCA9
IHRjbXVfY21kLT5jbWRfaWQ7DQotCWVudHJ5LT5oZHIua2ZsYWdzID0gMDsNCi0JZW50cnktPmhk
ci51ZmxhZ3MgPSAwOw0KwqANCsKgCS8qIEhhbmRsZSBhbGxvY2F0aW5nIHNwYWNlIGZyb20gdGhl
IGRhdGEgYXJlYSAqLw0KwqAJdGNtdV9jbWRfcmVzZXRfZGJpX2N1cih0Y211X2NtZCk7DQpAQCAt
ODYyLDExICs4NjEsMTAgQEAgdGNtdV9xdWV1ZV9jbWRfcmluZyhzdHJ1Y3QgdGNtdV9jbWQgKnRj
bXVfY21kKQ0KwqAJCXJldHVybiBUQ01fTE9HSUNBTF9VTklUX0NPTU1VTklDQVRJT05fRkFJTFVS
RTsNCsKgCX0NCsKgCWVudHJ5LT5yZXEuaW92X2NudCA9IGlvdl9jbnQ7DQotCWVudHJ5LT5yZXEu
aW92X2RpZl9jbnQgPSAwOw0KwqANCsKgCS8qIEhhbmRsZSBCSURJIGNvbW1hbmRzICovDQorCWlv
dl9jbnQgPSAwOw0KwqAJaWYgKHNlX2NtZC0+c2VfY21kX2ZsYWdzICYgU0NGX0JJREkpIHsNCi0J
CWlvdl9jbnQgPSAwOw0KwqAJCWlvdisrOw0KwqAJCXJldCA9IHNjYXR0ZXJfZGF0YV9hcmVhKHVk
ZXYsIHRjbXVfY21kLA0KwqAJCQkJCXNlX2NtZC0+dF9iaWRpX2RhdGFfc2csDQpAQCAtODc5LDgg
Kzg3Nyw4IEBAIHRjbXVfcXVldWVfY21kX3Jpbmcoc3RydWN0IHRjbXVfY21kICp0Y211X2NtZCkN
CsKgCQkJcHJfZXJyKCJ0Y211OiBhbGxvYyBhbmQgc2NhdHRlciBiaWRpIGRhdGEgZmFpbGVkXG4i
KTsNCsKgCQkJcmV0dXJuIFRDTV9MT0dJQ0FMX1VOSVRfQ09NTVVOSUNBVElPTl9GQUlMVVJFOw0K
wqAJCX0NCi0JCWVudHJ5LT5yZXEuaW92X2JpZGlfY250ID0gaW92X2NudDsNCsKgCX0NCisJZW50
cnktPnJlcS5pb3ZfYmlkaV9jbnQgPSBpb3ZfY250Ow0KwqANCsKgCS8qDQrCoAnCoCogUmVjYWxh
dWxhdGUgdGhlIGNvbW1hbmQncyBiYXNlIHNpemUgYW5kIHNpemUgYWNjb3JkaW5nDQotLcKgDQoN
CkJlc3QgcmVnYXJkcy4NCg0KLS0gDQpEYW1pZW4gTGUgTW9hbA0KV2VzdGVybiBEaWdpdGFs
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Xiubo Li July 11, 2017, 9:33 a.m. UTC | #3 
Hi Damien,

Good news.

Thanks very much for you help to test about this.


On 2017年07月11日 17:24, Damien Le Moal wrote:
> Xiubo,
>
> Well done ! This fixed my problem. The ZBC test suite now passes all tests on
> my target without crashing the kernel.
>
> Please see some comments/nitpicks below.
>
> Otherwise, please feel free to add my "tested-by"
>
>
> On Tue, 2017-07-11 at 17:06 +0800, Xiubo Li wrote:
>> On 2017年07月11日 16:05, lixiubo@cmss.chinamobile.com wrote:
>>> From: Xiubo Li <lixiubo@cmss.chinamobile.com>
>>>
>>> For all the entries allocated from the ring cmd area, the memory
>>> is something like the stack, which will reserve the old data, so
>>> the entry->req.iov_bidi_cnt maybe none zero.
>>>
>>> To fix this, just memset all the entry memory before using it, and
>>> also to be more readable we adjust the bidi code.
>>>
>>> Fixed: fe25cc34795(tcmu: Recalculate the tcmu_cmd size to save cmd area
>>> 		memories)
>>> Reported-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
>>> Signed-off-by: Xiubo Li <lixiubo@cmss.chinamobile.com>
>>> ---
>>>    drivers/target/target_core_user.c | 5 +++--
>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/target/target_core_user.c
>>> b/drivers/target/target_core_user.c
>>> index 2f1fa92..be62c86 100644
>>> --- a/drivers/target/target_core_user.c
>>> +++ b/drivers/target/target_core_user.c
>>> @@ -840,6 +840,7 @@ static inline size_t tcmu_cmd_get_cmd_size(struct
>>> tcmu_cmd *tcmu_cmd,
>>>    	}
>>>    
>>>    	entry = (void *) mb + CMDR_OFF + cmd_head;
>>> +	memset(entry, 0, command_size);
>>>    	tcmu_hdr_set_op(&entry->hdr.len_op, TCMU_OP_CMD);
>>>    	entry->hdr.cmd_id = tcmu_cmd->cmd_id;
>>>    	entry->hdr.kflags = 0;
> The added memset allows removing the 0 assignment here, and the one that
> follows this one too.
>
Yes, it is. Maybe reserving them could make the code more readable ?

Thanks,

BRs
Xiubo

>>> @@ -865,8 +866,8 @@ static inline size_t tcmu_cmd_get_cmd_size(struct
>>> tcmu_cmd *tcmu_cmd,
>>>    	entry->req.iov_dif_cnt = 0;
>>>    
>>>    	/* Handle BIDI commands */
>>> +	iov_cnt = 0;
>>>    	if (se_cmd->se_cmd_flags & SCF_BIDI) {
>>> -		iov_cnt = 0;
>>>    		iov++;
>>>    		ret = scatter_data_area(udev, tcmu_cmd,
>>>    					se_cmd->t_bidi_data_sg,
>>> @@ -879,8 +880,8 @@ static inline size_t tcmu_cmd_get_cmd_size(struct
>>> tcmu_cmd *tcmu_cmd,
>>>    			pr_err("tcmu: alloc and scatter bidi data
>>> failed\n");
>>>    			return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>>>    		}
>>> -		entry->req.iov_bidi_cnt = iov_cnt;
>>>    	}
>>> +	entry->req.iov_bidi_cnt = iov_cnt;
>>>    
>>>    	/*
>>>    	 * Recalaulate the command's base size and size according
> For reference, here is the actual patch I used for testing:
>
> diff --git a/drivers/target/target_core_user.c
> b/drivers/target/target_core_user.c
> index 2f1fa92..3b25ef3 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -563,7 +563,7 @@ static int scatter_data_area(struct tcmu_dev *udev,
>   			to_offset = get_block_offset_user(udev, dbi,
>   					block_remaining);
>   			offset = DATA_BLOCK_SIZE - block_remaining;
> -			to = (void *)(unsigned long)to + offset;
> +			to += offset;
>   
>   			if (*iov_cnt != 0 &&
>   			    to_offset == iov_tail(udev, *iov)) {
> @@ -636,7 +636,7 @@ static void gather_data_area(struct tcmu_dev *udev, struct
> tcmu_cmd *cmd,
>   			copy_bytes = min_t(size_t, sg_remaining,
>   					block_remaining);
>   			offset = DATA_BLOCK_SIZE - block_remaining;
> -			from = (void *)(unsigned long)from + offset;
> +			from += offset;
>   			tcmu_flush_dcache_range(from, copy_bytes);
>   			memcpy(to + sg->length - sg_remaining, from,
>   					copy_bytes);
> @@ -840,10 +840,9 @@ tcmu_queue_cmd_ring(struct tcmu_cmd *tcmu_cmd)
>   	}
>   
>   	entry = (void *) mb + CMDR_OFF + cmd_head;
> +	memset(entry, 0, command_size);
>   	tcmu_hdr_set_op(&entry->hdr.len_op, TCMU_OP_CMD);
>   	entry->hdr.cmd_id = tcmu_cmd->cmd_id;
> -	entry->hdr.kflags = 0;
> -	entry->hdr.uflags = 0;
>   
>   	/* Handle allocating space from the data area */
>   	tcmu_cmd_reset_dbi_cur(tcmu_cmd);
> @@ -862,11 +861,10 @@ tcmu_queue_cmd_ring(struct tcmu_cmd *tcmu_cmd)
>   		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>   	}
>   	entry->req.iov_cnt = iov_cnt;
> -	entry->req.iov_dif_cnt = 0;
>   
>   	/* Handle BIDI commands */
> +	iov_cnt = 0;
>   	if (se_cmd->se_cmd_flags & SCF_BIDI) {
> -		iov_cnt = 0;
>   		iov++;
>   		ret = scatter_data_area(udev, tcmu_cmd,
>   					se_cmd->t_bidi_data_sg,
> @@ -879,8 +877,8 @@ tcmu_queue_cmd_ring(struct tcmu_cmd *tcmu_cmd)
>   			pr_err("tcmu: alloc and scatter bidi data failed\n");
>   			return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
>   		}
> -		entry->req.iov_bidi_cnt = iov_cnt;
>   	}
> +	entry->req.iov_bidi_cnt = iov_cnt;
>   
>   	/*
>   	 * Recalaulate the command's base size and size according
> -- 
>
> Best regards.
>



--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicholas A. Bellinger July 11, 2017, 9:34 a.m. UTC | #4 
On Tue, 2017-07-11 at 09:24 +0000, Damien Le Moal wrote:
> Xiubo,
> 
> Well done ! This fixed my problem. The ZBC test suite now passes all tests on
> my target without crashing the kernel.
> 
> Please see some comments/nitpicks below.
> 
> Otherwise, please feel free to add my "tested-by"
> 

Great, thanks for the quick turn-around.
 
Xiubo, please pick-up Damien's extra bits and repost for v2.  Also,
please update the patch topic + commit log to mention it addresses a
general protection fault OOPs, so others can find it easily when looking
through git log.

Damien's OOPs would probably be the best one to include in the commit
log, since Bryant's was on POWER.  ;)

Btw, since it's a regression fix related to commit fe25cc34795, I assume
it should be CC'ed to stable for v4.12.y, right..?

--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Xiubo Li July 11, 2017, 9:37 a.m. UTC | #5 
On 2017年07月11日 17:34, Nicholas A. Bellinger wrote:
> On Tue, 2017-07-11 at 09:24 +0000, Damien Le Moal wrote:
>> Xiubo,
>>
>> Well done ! This fixed my problem. The ZBC test suite now passes all tests on
>> my target without crashing the kernel.
>>
>> Please see some comments/nitpicks below.
>>
>> Otherwise, please feel free to add my "tested-by"
>>
> Great, thanks for the quick turn-around.
>   
> Xiubo, please pick-up Damien's extra bits and repost for v2.  Also,
> please update the patch topic + commit log to mention it addresses a
> general protection fault OOPs, so others can find it easily when looking
> through git log.
>
> Damien's OOPs would probably be the best one to include in the commit
> log, since Bryant's was on POWER.  ;)
Sure, I will repost for v2 later.

> Btw, since it's a regression fix related to commit fe25cc34795, I assume
> it should be CC'ed to stable for v4.12.y, right..?
>
Yes, it is.

Thanks

BRs
Xiubo


--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
index 2f1fa92..be62c86 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -840,6 +840,7 @@  static inline size_t tcmu_cmd_get_cmd_size(struct tcmu_cmd *tcmu_cmd,
 	}
 
 	entry = (void *) mb + CMDR_OFF + cmd_head;
+	memset(entry, 0, command_size);
 	tcmu_hdr_set_op(&entry->hdr.len_op, TCMU_OP_CMD);
 	entry->hdr.cmd_id = tcmu_cmd->cmd_id;
 	entry->hdr.kflags = 0;
@@ -865,8 +866,8 @@  static inline size_t tcmu_cmd_get_cmd_size(struct tcmu_cmd *tcmu_cmd,
 	entry->req.iov_dif_cnt = 0;
 
 	/* Handle BIDI commands */
+	iov_cnt = 0;
 	if (se_cmd->se_cmd_flags & SCF_BIDI) {
-		iov_cnt = 0;
 		iov++;
 		ret = scatter_data_area(udev, tcmu_cmd,
 					se_cmd->t_bidi_data_sg,
@@ -879,8 +880,8 @@  static inline size_t tcmu_cmd_get_cmd_size(struct tcmu_cmd *tcmu_cmd,
 			pr_err("tcmu: alloc and scatter bidi data failed\n");
 			return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 		}
-		entry->req.iov_bidi_cnt = iov_cnt;
 	}
+	entry->req.iov_bidi_cnt = iov_cnt;
 
 	/*
 	 * Recalaulate the command's base size and size according