[PATCH v5 0/2] Fix opal_add_one_export

[PATCH v5 0/2] Fix opal_add_one_export

[Qiujun Huang]

We should check the return value of kzalloc, as kzalloc may fail returning NULL.

---
v4->v5:
	Separate the patch into two.
v3->v4:
	Added the information about coccinelle script.
	Added change log.
	Added Oliver's Reviewed-by.
v2->v3:
	Removed redundant assignment to 'attr' and 'name'.
v1->v2:
	Just return -ENOMEM if attr is NULL.

Qiujun Huang (2):
  powerpc/powernv: Remove redundant assignments to attr and name
  powerpc/powernv: Add NULL check after kzalloc in opal_add_one_export

 arch/powerpc/platforms/powernv/opal.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

-- 
2.17.1

[PATCH v5 1/2] powerpc/powernv: Remove redundant assignments to attr and name

[Qiujun Huang]

We don't need to go to the labal of out when of_property_read_u64_array
fails, as there is nothing to do. Just return.
And we can remove the redundant assignments to attr and name.

Signed-off-by: Qiujun Huang <hqjagain@gmail.com>
Reviewed-by: Oliver O'Halloran <oohall@gmail.com>
---
 arch/powerpc/platforms/powernv/opal.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index 2b3dfd0b6cdd..71af1cbc6334 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -801,14 +801,14 @@ static ssize_t export_attr_read(struct file *fp, struct kobject *kobj,
 static int opal_add_one_export(struct kobject *parent, const char *export_name,
 			       struct device_node *np, const char *prop_name)
 {
-	struct bin_attribute *attr = NULL;
-	const char *name = NULL;
+	struct bin_attribute *attr;
+	const char *name;
 	u64 vals[2];
 	int rc;
 
 	rc = of_property_read_u64_array(np, prop_name, &vals[0], 2);
 	if (rc)
-		goto out;
+		return rc;
 
 	attr = kzalloc(sizeof(*attr), GFP_KERNEL);
 	name = kstrdup(export_name, GFP_KERNEL);
-- 
2.17.1

[PATCH v5 2/2] powerpc/powernv: Add NULL check after kzalloc in opal_add_one_export

[Qiujun Huang]

Here needs a NULL check, as kzalloc may fail returning NULL.

Issue was found by coccinelle.
Generated by: scripts/coccinelle/null/kmerr.cocci

Signed-off-by: Qiujun Huang <hqjagain@gmail.com>
Reviewed-by: Oliver O'Halloran <oohall@gmail.com>
---
 arch/powerpc/platforms/powernv/opal.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index 71af1cbc6334..908d749bcef5 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -811,6 +811,9 @@ static int opal_add_one_export(struct kobject *parent, const char *export_name,
 		return rc;
 
 	attr = kzalloc(sizeof(*attr), GFP_KERNEL);
+	if (!attr)
+		return -ENOMEM;
+
 	name = kstrdup(export_name, GFP_KERNEL);
 	if (!name) {
 		rc = -ENOMEM;
-- 
2.17.1

Re: [PATCH v5 1/2] powerpc/powernv: Remove two unnecessary variable initialisations in opal_add_one_export()

[Markus Elfring]

> And we can remove the redundant assignments to attr and name.

How do you think about a wording like the following?

   Two local variables will eventually be set to appropriate pointers
   a bit later. Thus omit their explicit initialisation at the beginning.


Regards,
Markus

Re: [PATCH v5 2/2] powerpc/powernv: Add NULL check after kzalloc in opal_add_one_export

[Markus Elfring]

> Here needs a NULL check, as kzalloc may fail returning NULL.
>
> Issue was found by coccinelle.

* Do you really try to ignore (my) specific patch review comments
  (for a moment)?
  https://lore.kernel.org/linuxppc-dev/b7d64d4a-74dd-ee21-db7b-018070f1295f@web.de/
  https://lore.kernel.org/patchwork/comment/1414845/
  https://lkml.org/lkml/2020/4/6/279

* Would you like to integrate further adjustments with a varying delay?
  (Are you waiting on nicer feedback by any software maintainers?)

Regards,
Markus

Re: [PATCH v5 1/2] powerpc/powernv: Return directly after a failed of_property_read_u64_array() in opal_add_one_export()

[Markus Elfring]

> We don't need to go to the labal of out …

Please avoid a typo for this change description.


> fails, as there is nothing to do. Just return.

I suggest to reconsider also this wording.

  Return directly after a call of the function “of_property_read_u64_array”
  failed at the beginning.


> And we can …

Do such words indicate that even this patch approach should be split
into further update steps?

Regards,
Markus