we should check kzalloc, avoid to hit oops
Signed-off-by: Libo Chen <[email protected]>
---
drivers/scsi/megaraid.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
index 846f475..195b095 100644
--- a/drivers/scsi/megaraid.c
+++ b/drivers/scsi/megaraid.c
@@ -4162,6 +4162,10 @@ mega_internal_command(adapter_t *adapter, megacmd_t *mc, mega_passthru *pthru)
sdev = kzalloc(sizeof(struct scsi_device), GFP_KERNEL);
scmd->device = sdev;
+ if (!scmd->device) {
+ scsi_free_command(GFP_KERNEL, scmd);
+ return -ENOMEM;
+ }
memset(adapter->int_cdb, 0, sizeof(adapter->int_cdb));
scmd->cmnd = adapter->int_cdb;
--
1.7.1
On Fri, May 24, 2013 at 7:52 AM, Libo Chen <[email protected]> wrote:
>
> we should check kzalloc, avoid to hit oops
>
> Signed-off-by: Libo Chen <[email protected]>
> ---
> drivers/scsi/megaraid.c | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
> index 846f475..195b095 100644
> --- a/drivers/scsi/megaraid.c
> +++ b/drivers/scsi/megaraid.c
> @@ -4162,6 +4162,10 @@ mega_internal_command(adapter_t *adapter, megacmd_t *mc, mega_passthru *pthru)
>
> sdev = kzalloc(sizeof(struct scsi_device), GFP_KERNEL);
> scmd->device = sdev;
^^^^^^
I guess assigning after the NULL check of 'sdev' is more appropriate.
> + if (!scmd->device) {
> + scsi_free_command(GFP_KERNEL, scmd);
> + return -ENOMEM;
> + }
>
> memset(adapter->int_cdb, 0, sizeof(adapter->int_cdb));
> scmd->cmnd = adapter->int_cdb;
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
~Santosh
On 2013/5/24 11:20, Santosh Y wrote:
> On Fri, May 24, 2013 at 7:52 AM, Libo Chen <[email protected]> wrote:
>>
>> we should check kzalloc, avoid to hit oops
>>
>> Signed-off-by: Libo Chen <[email protected]>
>> ---
>> drivers/scsi/megaraid.c | 4 ++++
>> 1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
>> index 846f475..195b095 100644
>> --- a/drivers/scsi/megaraid.c
>> +++ b/drivers/scsi/megaraid.c
>> @@ -4162,6 +4162,10 @@ mega_internal_command(adapter_t *adapter, megacmd_t *mc, mega_passthru *pthru)
>>
>> sdev = kzalloc(sizeof(struct scsi_device), GFP_KERNEL);
>> scmd->device = sdev;
> ^^^^^^
> I guess assigning after the NULL check of 'sdev' is more appropriate.
Ah, there is a little different. ok, I will update.
Thanks,
Libo
>
>> + if (!scmd->device) {
>> + scsi_free_command(GFP_KERNEL, scmd);
>> + return -ENOMEM;
>> + }
>>
>> memset(adapter->int_cdb, 0, sizeof(adapter->int_cdb));
>> scmd->cmnd = adapter->int_cdb;
>> --
>> 1.7.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
we should check kzalloc, avoid to hit oops
Signed-off-by: Libo Chen <[email protected]>
---
drivers/scsi/megaraid.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
instead of checking scmd->device, sdev is more appropriate.
diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
index 846f475..6b623cb 100644
--- a/drivers/scsi/megaraid.c
+++ b/drivers/scsi/megaraid.c
@@ -4161,6 +4161,10 @@ mega_internal_command(adapter_t *adapter, megacmd_t *mc, mega_passthru *pthru)
memset(scb, 0, sizeof(scb_t));
sdev = kzalloc(sizeof(struct scsi_device), GFP_KERNEL);
+ if (sdev) {
+ scsi_free_command(GFP_KERNEL, scmd);
+ return -ENOMEM;
+ }
scmd->device = sdev;
memset(adapter->int_cdb, 0, sizeof(adapter->int_cdb));
--
1.7.1
On 05/24/2013 11:40 AM, Libo Chen wrote:
> we should check kzalloc, avoid to hit oops
>
> Signed-off-by: Libo Chen <[email protected]>
> ---
> drivers/scsi/megaraid.c | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> instead of checking scmd->device, sdev is more appropriate.
>
> diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
> index 846f475..6b623cb 100644
> --- a/drivers/scsi/megaraid.c
> +++ b/drivers/scsi/megaraid.c
> @@ -4161,6 +4161,10 @@ mega_internal_command(adapter_t *adapter, megacmd_t *mc, mega_passthru *pthru)
> memset(scb, 0, sizeof(scb_t));
>
> sdev = kzalloc(sizeof(struct scsi_device), GFP_KERNEL);
> + if (sdev) {
> + scsi_free_command(GFP_KERNEL, scmd);
I think, that a mutex_unlock(&adapter->int_mtx); is also needed
Maybe just setting a rval = -ENOMEM and a jump to to some point below?
tomash
> + return -ENOMEM;
> + }
> scmd->device = sdev;
>
> memset(adapter->int_cdb, 0, sizeof(adapter->int_cdb));
On 05/24/2013 11:40 AM, Libo Chen wrote:
> we should check kzalloc, avoid to hit oops
>
> Signed-off-by: Libo Chen <[email protected]>
> ---
> drivers/scsi/megaraid.c | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> instead of checking scmd->device, sdev is more appropriate.
>
> diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
> index 846f475..6b623cb 100644
> --- a/drivers/scsi/megaraid.c
> +++ b/drivers/scsi/megaraid.c
> @@ -4161,6 +4161,10 @@ mega_internal_command(adapter_t *adapter, megacmd_t *mc, mega_passthru *pthru)
> memset(scb, 0, sizeof(scb_t));
>
> sdev = kzalloc(sizeof(struct scsi_device), GFP_KERNEL);
> + if (sdev) {
> + scsi_free_command(GFP_KERNEL, scmd);
I think, that a mutex_unlock(&adapter->int_mtx); is also needed
Maybe just setting a rval = -ENOMEM and a jump to to some point below?
tomash
> + return -ENOMEM;
> + }
> scmd->device = sdev;
>
> memset(adapter->int_cdb, 0, sizeof(adapter->int_cdb));
On 2013/5/29 23:03, Tomas Henzl wrote:
> On 05/24/2013 11:40 AM, Libo Chen wrote:
>> we should check kzalloc, avoid to hit oops
>>
>> Signed-off-by: Libo Chen <[email protected]>
>> ---
>> drivers/scsi/megaraid.c | 4 ++++
>> 1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> instead of checking scmd->device, sdev is more appropriate.
>>
>> diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
>> index 846f475..6b623cb 100644
>> --- a/drivers/scsi/megaraid.c
>> +++ b/drivers/scsi/megaraid.c
>> @@ -4161,6 +4161,10 @@ mega_internal_command(adapter_t *adapter, megacmd_t *mc, mega_passthru *pthru)
>> memset(scb, 0, sizeof(scb_t));
>>
>> sdev = kzalloc(sizeof(struct scsi_device), GFP_KERNEL);
>> + if (sdev) {
>> + scsi_free_command(GFP_KERNEL, scmd);
>
> I think, that a mutex_unlock(&adapter->int_mtx); is also needed
> Maybe just setting a rval = -ENOMEM and a jump to to some point below?
>
> tomash
thanks for catching this.
when kzalloc broken, fist unlock and then return. I will update later.
Libo
>
>> + return -ENOMEM;
>> + }
>> scmd->device = sdev;
>>
>> memset(adapter->int_cdb, 0, sizeof(adapter->int_cdb));
>
>
> .
>
On 2013/5/30 9:38, Libo Chen wrote:
> On 2013/5/29 23:03, Tomas Henzl wrote:
>> On 05/24/2013 11:40 AM, Libo Chen wrote:
>>> we should check kzalloc, avoid to hit oops
>>>
>>> Signed-off-by: Libo Chen <[email protected]>
>>> ---
>>> drivers/scsi/megaraid.c | 4 ++++
>>> 1 files changed, 4 insertions(+), 0 deletions(-)
>>>
>>> instead of checking scmd->device, sdev is more appropriate.
>>>
>>> diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
>>> index 846f475..6b623cb 100644
>>> --- a/drivers/scsi/megaraid.c
>>> +++ b/drivers/scsi/megaraid.c
>>> @@ -4161,6 +4161,10 @@ mega_internal_command(adapter_t *adapter, megacmd_t *mc, mega_passthru *pthru)
>>> memset(scb, 0, sizeof(scb_t));
>>>
>>> sdev = kzalloc(sizeof(struct scsi_device), GFP_KERNEL);
>>> + if (sdev) {
>>> + scsi_free_command(GFP_KERNEL, scmd);
>>
>> I think, that a mutex_unlock(&adapter->int_mtx); is also needed
>> Maybe just setting a rval = -ENOMEM and a jump to to some point below?
>>
>> tomash
>
> thanks for catching this.
>
> when kzalloc broken, fist unlock and then return. I will update later.
>
I think we can put kzalloc outside of mutex_lock(&adapter->int_mtx) ?
phase:
mutex_lock kzalloc
kzalloc -> mutex_lock
>
> Libo
>
>>
>>> + return -ENOMEM;
>>> + }
>>> scmd->device = sdev;
>>>
>>> memset(adapter->int_cdb, 0, sizeof(adapter->int_cdb));
>>
>>
>> .
>>
>
On 05/30/2013 04:32 AM, Libo Chen wrote:
> On 2013/5/30 9:38, Libo Chen wrote:
>> On 2013/5/29 23:03, Tomas Henzl wrote:
>>> On 05/24/2013 11:40 AM, Libo Chen wrote:
>>>> we should check kzalloc, avoid to hit oops
>>>>
>>>> Signed-off-by: Libo Chen <[email protected]>
>>>> ---
>>>> drivers/scsi/megaraid.c | 4 ++++
>>>> 1 files changed, 4 insertions(+), 0 deletions(-)
>>>>
>>>> instead of checking scmd->device, sdev is more appropriate.
>>>>
>>>> diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
>>>> index 846f475..6b623cb 100644
>>>> --- a/drivers/scsi/megaraid.c
>>>> +++ b/drivers/scsi/megaraid.c
>>>> @@ -4161,6 +4161,10 @@ mega_internal_command(adapter_t *adapter, megacmd_t *mc, mega_passthru *pthru)
>>>> memset(scb, 0, sizeof(scb_t));
>>>>
>>>> sdev = kzalloc(sizeof(struct scsi_device), GFP_KERNEL);
>>>> + if (sdev) {
>>>> + scsi_free_command(GFP_KERNEL, scmd);
>>> I think, that a mutex_unlock(&adapter->int_mtx); is also needed
>>> Maybe just setting a rval = -ENOMEM and a jump to to some point below?
>>>
>>> tomash
>> thanks for catching this.
>>
>> when kzalloc broken, fist unlock and then return. I will update later.
>>
> I think we can put kzalloc outside of mutex_lock(&adapter->int_mtx) ?
> phase:
Yes, that is also possible, depends on what you prefer.
>
> mutex_lock kzalloc
> kzalloc -> mutex_lock
>
>
>> Libo
>>
>>>> + return -ENOMEM;
>>>> + }
>>>> scmd->device = sdev;
>>>>
>>>> memset(adapter->int_cdb, 0, sizeof(adapter->int_cdb));
>>>
>>> .
>>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
we should check kzalloc, avoid to hit oops
Change from v1:
- put kzalloc outside of mutex
Signed-off-by: Libo Chen <[email protected]>
---
drivers/scsi/megaraid.c | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/drivers/scsi/megaraid.c b/drivers/scsi/megaraid.c
index 846f475..cc599cc 100644
--- a/drivers/scsi/megaraid.c
+++ b/drivers/scsi/megaraid.c
@@ -4150,6 +4150,12 @@ mega_internal_command(adapter_t *adapter, megacmd_t *mc, mega_passthru *pthru)
if (!scmd)
return -ENOMEM;
+ sdev = kzalloc(sizeof(struct scsi_device), GFP_KERNEL);
+ if (!sdev) {
+ scsi_free_command(GFP_KERNEL, scmd);
+ return -ENOMEM;
+ }
+
/*
* The internal commands share one command id and hence are
* serialized. This is so because we want to reserve maximum number of
@@ -4160,7 +4166,6 @@ mega_internal_command(adapter_t *adapter, megacmd_t *mc, mega_passthru *pthru)
scb = &adapter->int_scb;
memset(scb, 0, sizeof(scb_t));
- sdev = kzalloc(sizeof(struct scsi_device), GFP_KERNEL);
scmd->device = sdev;
memset(adapter->int_cdb, 0, sizeof(adapter->int_cdb));
--
1.7.1
On 06/04/2013 11:33 AM, Libo Chen wrote:
> we should check kzalloc, avoid to hit oops
>
> Change from v1:
> - put kzalloc outside of mutex
>
> Signed-off-by: Libo Chen <[email protected]>
Your patch looks fine to me:
Acked-by: Tomas Henzl <[email protected]>
Cheers,
Tomas
On 2013/6/4 20:14, Tomas Henzl wrote:
> On 06/04/2013 11:33 AM, Libo Chen wrote:
>> we should check kzalloc, avoid to hit oops
>>
>> Change from v1:
>> - put kzalloc outside of mutex
>>
>> Signed-off-by: Libo Chen <[email protected]>
>
> Your patch looks fine to me:
> Acked-by: Tomas Henzl <[email protected]>
>
your ack is very helpfull for me.
Thanks,
Libo
> Cheers,
> Tomas
>
>
> .
>