qlcnic: using too much stack

qlcnic_pci_info structs are 128 bytes so an array of 8 uses 1024 bytes. That's a lot if you run with 4K stacks. I allocated them with kcalloc() instead.

Signed-off-by Dan Carpenter <[email protected]>

drivers/net/qlcnic/qlcnic_main.c +0 -0

index 7f27e2a..da84229 100644
@@ -473,14 +473,20 @@ qlcnic_cleanup_pci_map(struct qlcnic_adapter *adapter)

static int
qlcnic_init_pci_info(struct qlcnic_adapter *adapter) {

drivers/net/qlcnic/qlcnic_main.c +1 -1

-	struct qlcnic_pci_info pci_info[QLCNIC_MAX_PCI_FUNC];
+	struct qlcnic_pci_info *pci_info;

	int i, ret = 0, err;
	u8 pfn;

drivers/net/qlcnic/qlcnic_main.c +4 -0

+	pci_info = kcalloc(QLCNIC_MAX_PCI_FUNC, sizeof(*pci_info), GFP_KERNEL);
+	if (!pci_info)
+		return -ENOMEM;
+

	adapter->npars = kzalloc(sizeof(struct qlcnic_npar_info) *
				QLCNIC_MAX_PCI_FUNC, GFP_KERNEL);

drivers/net/qlcnic/qlcnic_main.c +4 -2

-	if (!adapter->npars)
-		return -ENOMEM;
+	if (!adapter->npars) {
+		err = -ENOMEM;
+		goto err_pci_info;
+	}

	adapter->eswitch = kzalloc(sizeof(struct qlcnic_eswitch) *
				QLCNIC_NIU_MAX_XG_PORTS, GFP_KERNEL);

drivers/net/qlcnic/qlcnic_main.c +1 -0

@@ -508,6 +514,7 @@ qlcnic_init_pci_info(struct qlcnic_adapter *adapter)
 	for (i = 0; i < QLCNIC_NIU_MAX_XG_PORTS; i++)
 		adapter->eswitch[i].flags |= QLCNIC_SWITCH_ENABLE;
 
+	kfree(pci_info);
 	return 0;
 

err_eswitch:

drivers/net/qlcnic/qlcnic_main.c +0 -0

@@ -516,6 +523,8 @@ err_eswitch:

err_npars:

	kfree(adapter->npars);
	adapter->eswitch = NULL;

drivers/net/qlcnic/qlcnic_main.c +2 -0

+err_pci_info:
+	kfree(pci_info);

	return ret;
}

drivers/net/qlcnic/qlcnic_main.c +8 -2

@@ -3362,15 +3371,21 @@ qlcnic_sysfs_read_pci_config(struct file *file, struct kobject *kobj,
 	struct device *dev = container_of(kobj, struct device, kobj);
 	struct qlcnic_adapter *adapter = dev_get_drvdata(dev);
 	struct qlcnic_pci_func_cfg pci_cfg[QLCNIC_MAX_PCI_FUNC];
-	struct qlcnic_pci_info	pci_info[QLCNIC_MAX_PCI_FUNC];
+	struct qlcnic_pci_info *pci_info;
 	int i, ret;
 
 	if (size != sizeof(pci_cfg))
 		return QL_STATUS_INVALID_PARAM;
 
+	pci_info = kcalloc(QLCNIC_MAX_PCI_FUNC, sizeof(*pci_info), GFP_KERNEL);
+	if (!pci_info)
+		return -ENOMEM;
+
 	ret = qlcnic_get_pci_info(adapter, pci_info);
-	if (ret)
+	if (ret) {
+		kfree(pci_info);
 		return ret;
+	}
 
 	for (i = 0; i < QLCNIC_MAX_PCI_FUNC ; i++) {
 		pci_cfg[i].pci_func = pci_info[i].id;

It looks fine except that I'd use kzalloc instead of kcalloc above.

thanks,
-Anirban

Quoted 3 lines · Anirban Chakraborty

It looks fine except that I'd use kzalloc instead of kcalloc above.

It's no problem to do that, and I'm already respinning the patches but I'm confused. It looks like pci_info gets initialized correctly. What am I missing?

Your patch is fine except that the preferred way is to use kzalloc over kaclloc. kzalloc does not need that extra argument that you are passing to kcalloc.

Quoted 2 lines · Anirban Chakraborty

Your patch is fine except that the preferred way is to use kzalloc over kaclloc. kzalloc does not need that extra argument that you are passing to kcalloc.

You probably meant to write "my preferred way" as the kcalloc to "kzalloc with a multiply" ratio is pretty high.

It's actually about 2.5 to 1 in favor of kcalloc.

$ grep -rw --include=*.[ch] kcalloc * | wc -l
419

$ grep -rP --include=*.[ch] "\bkzalloc\s*\(\s*\w+\s*\*\s*\w+" * | \
 grep -vP "\bkzalloc\s*\(\s*sizeof\s+\*\s*\w+\s*," | wc -l
164

(the grep -vP avoids kzalloc(sizeof *p, GFP_foo)

Actually, there might be a reason to use kzalloc in that location to match the other similar use a few lines away, but I'd prefer that the other use be converted to kcalloc.

Quoted 6 lines · Joe Perches

Quoted 2 lines · Anirban Chakraborty

Your patch is fine except that the preferred way is to use kzalloc over kaclloc. kzalloc does not need that extra argument that you are passing to kcalloc.

You probably meant to write "my preferred way"
as the kcalloc to "kzalloc with a multiply"
ratio is pretty high.

I was suggesting based on the following:
http://lwn.net/Articles/147014/

Quoted 7 lines · Joe Perches

Quoted 6 lines · Anirban Chakraborty

Quoted 2 lines

Your patch is fine except that the preferred way is to use kzalloc over kaclloc. kzalloc does not need that extra argument that you are passing to kcalloc.

You probably meant to write "my preferred way"
as the kcalloc to "kzalloc with a multiply"
ratio is pretty high.

Quoted 2 lines

I was suggesting based on the following:
http://lwn.net/Articles/147014/

Note that article suggests kzalloc for allocating
a single zeroed object.

kcalloc is used for multiple zeroed objects and
protects against oversized allocations.