scsi: megaraid: check kzalloc

PATCH Superseded Libo Chen 4 participants 12 messages May 24, 2013 – Jun 5, 2013

Subsystems (2)

SCSI Megaraid SCSI/SAS drivers

lore

Share & tools

lore

Libo Chen

author

2013-05-24 02:22 UTC #0 lore raw

PATCH 1/1 scsi: megaraid: check kzalloc

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(-)

drivers/scsi/megaraid.c +4 -0

index 846f475..195b095 100644
@@ -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;

Signature · 1 line

1.7.1

Santosh Y

2013-05-24 03:20 UTC #1 lore raw

Quoted 13 lines · Libo Chen

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(-)
drivers/scsi/megaraid.c +0 -0
index 846f475..195b095 100644

drivers/scsi/megaraid.c mega_internal_command(adapter_t *adapter, megacmd_t *mc, mega_passthru *pthru) · line 4164

Quoted 3 lines

drivers/scsi/megaraid.c +0 -0

@@ -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.

Quoted 14 lines

drivers/scsi/megaraid.c +4 -1
+       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

Signature · 1 line

~Santosh

Libo Chen

author

2013-05-24 09:30 UTC #2 lore raw

Quoted 15 lines · Santosh Y

Quoted 14 lines · Libo Chen
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(-)
drivers/scsi/megaraid.c +0 -0
index 846f475..195b095 100644
@@ -4162,6 +4162,10 @@ mega_internal_command(adapter_t *adapter, megacmd_t *mc, mega_passthru *pthru)
 

Quoted 4 lines

drivers/scsi/megaraid.c mega_internal_command(adapter_t *adapter, megacmd_t *mc, mega_passthru *pthru) · line 4164
Quoted 2 lines
drivers/scsi/megaraid.c +0 -0
        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

Quoted 18 lines

Quoted 15 lines
drivers/scsi/megaraid.c +4 -1
+       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

Libo Chen

author

2013-05-24 09:40 UTC #3 lore raw

PATCH RESEND 1/1 scsi: megaraid: check kzalloc

In reply to Libo Chen

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.

drivers/scsi/megaraid.c +4 -0

index 846f475..6b623cb 100644
@@ -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));

Signature · 1 line

1.7.1

Tomas Henzl

2013-05-29 15:03 UTC #4 lore raw

Quoted 14 lines · Libo Chen

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.
drivers/scsi/megaraid.c +0 -0
index 846f475..6b623cb 100644

drivers/scsi/megaraid.c mega_internal_command(adapter_t *adapter, megacmd_t *mc, mega_passthru *pthru) · line 4165

Quoted 5 lines

drivers/scsi/megaraid.c +2 -0

@@ -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

Quoted 5 lines

drivers/scsi/megaraid.c +2 -0

+		return -ENOMEM;
+	}
 	scmd->device = sdev;
 
 	memset(adapter->int_cdb, 0, sizeof(adapter->int_cdb));

Tomas Henzl

2013-05-29 15:12 UTC #8 lore raw

In reply to Libo Chen

Quoted 14 lines · Libo Chen

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.
drivers/scsi/megaraid.c +0 -0
index 846f475..6b623cb 100644

drivers/scsi/megaraid.c mega_internal_command(adapter_t *adapter, megacmd_t *mc, mega_passthru *pthru) · line 4165

Quoted 5 lines

drivers/scsi/megaraid.c +2 -0

@@ -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

Quoted 5 lines

drivers/scsi/megaraid.c +2 -0

+		return -ENOMEM;
+	}
 	scmd->device = sdev;
 
 	memset(adapter->int_cdb, 0, sizeof(adapter->int_cdb));

Libo Chen

author

2013-05-30 01:38 UTC #5 lore raw

In reply to Tomas Henzl

Quoted 17 lines · Tomas Henzl

Quoted 16 lines · Libo Chen
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.
drivers/scsi/megaraid.c +0 -0
index 846f475..6b623cb 100644
@@ -4161,6 +4161,10 @@ mega_internal_command(adapter_t *adapter, megacmd_t *mc, mega_passthru *pthru)
 	memset(scb, 0, sizeof(scb_t));
 

Quoted 8 lines

drivers/scsi/megaraid.c mega_internal_command(adapter_t *adapter, megacmd_t *mc, mega_passthru *pthru) · line 4165
Quoted 3 lines
drivers/scsi/megaraid.c +2 -0
 	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

Quoted 10 lines

drivers/scsi/megaraid.c mega_internal_command(adapter_t *adapter, megacmd_t *mc, mega_passthru *pthru) · line 4170
Quoted 6 lines
drivers/scsi/megaraid.c +2 -0
+		return -ENOMEM;
+	}
 	scmd->device = sdev;
 
 	memset(adapter->int_cdb, 0, sizeof(adapter->int_cdb));
.

Libo Chen

author

2013-05-30 02:33 UTC #6 lore raw

Quoted 25 lines · Libo Chen

Quoted 24 lines · Tomas Henzl
Quoted 14 lines · Libo Chen
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.
drivers/scsi/megaraid.c +0 -0
index 846f475..6b623cb 100644
drivers/scsi/megaraid.c mega_internal_command(adapter_t *adapter, megacmd_t *mc, mega_passthru *pthru) · line 4165
Quoted 5 lines
drivers/scsi/megaraid.c +2 -0
@@ -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?

Quoted 6 lines

Quoted 1 line
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

Quoted 14 lines

Libo
Quoted 10 lines
drivers/scsi/megaraid.c mega_internal_command(adapter_t *adapter, megacmd_t *mc, mega_passthru *pthru) · line 4170
Quoted 6 lines
drivers/scsi/megaraid.c +2 -0
+		return -ENOMEM;
+	}
 	scmd->device = sdev;
 
 	memset(adapter->int_cdb, 0, sizeof(adapter->int_cdb));
.

Tomas Henzl

2013-05-30 10:54 UTC #7 lore raw

Quoted 25 lines · Libo Chen

Quoted 24 lines · Libo Chen
Quoted 23 lines · Tomas Henzl
Quoted 14 lines · Libo Chen
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.
drivers/scsi/megaraid.c +0 -0
index 846f475..6b623cb 100644
drivers/scsi/megaraid.c mega_internal_command(adapter_t *adapter, megacmd_t *mc, mega_passthru *pthru) · line 4165
Quoted 5 lines
drivers/scsi/megaraid.c +2 -0
@@ -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?

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
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.

Quoted 20 lines

   mutex_lock	   kzalloc
   kzalloc     ->  mutex_lock
Quoted 10 lines
Libo
Quoted 8 lines
drivers/scsi/megaraid.c mega_internal_command(adapter_t *adapter, megacmd_t *mc, mega_passthru *pthru) · line 4170
Quoted 5 lines
drivers/scsi/megaraid.c +2 -0
+		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

Libo Chen

2013-06-04 09:35 UTC #9 lore raw

In reply to Libo Chen

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(-)

drivers/scsi/megaraid.c +6 -1

index 846f475..cc599cc 100644
@@ -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));

Signature · 1 line

1.7.1

Tomas Henzl

Acked-by

2013-06-04 12:14 UTC #10 lore raw

Quoted 6 lines · Libo Chen

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

Libo Chen

2013-06-05 01:27 UTC #11 lore raw

Quoted 11 lines · Tomas Henzl

Quoted 6 lines · Libo Chen
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

Quoted 6 lines

Cheers,
Tomas
.