2020-04-05 12:28:00

by Qiujun Huang

[permalink] [raw]
Subject: [PATCH v3] powerpc/powernv: add NULL check after kzalloc in opal_add_one_export

Here needs a NULL check.

Issue found by coccinelle.

Signed-off-by: Qiujun Huang <[email protected]>
---
arch/powerpc/platforms/powernv/opal.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index 2b3dfd0b6cdd..908d749bcef5 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -801,16 +801,19 @@ 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);
+ if (!attr)
+ return -ENOMEM;
+
name = kstrdup(export_name, GFP_KERNEL);
if (!name) {
rc = -ENOMEM;
--
2.17.1


2020-04-05 19:09:09

by Markus Elfring

[permalink] [raw]
Subject: Re: [PATCH v3] powerpc/powernv: add NULL check after kzalloc in opal_add_one_export

> Here needs a NULL check.

I find this change description questionable
(despite of a reasonable patch subject).


> Issue found by coccinelle.

Would an information like “Generated by: scripts/coccinelle/null/kmerr.cocci”
be nicer?


> ---

Will a patch change log be helpful here?

Regards,
Markus

2020-04-06 01:14:19

by Qiujun Huang

[permalink] [raw]
Subject: Re: [PATCH v3] powerpc/powernv: add NULL check after kzalloc in opal_add_one_export

On Mon, Apr 6, 2020 at 3:06 AM Markus Elfring <[email protected]> wrote:
>
> > Here needs a NULL check.
quite obvious?
>
> I find this change description questionable
> (despite of a reasonable patch subject).
>
>
> > Issue found by coccinelle.
>
> Would an information like “Generated by: scripts/coccinelle/null/kmerr.cocci”
> be nicer?
Yeah, but I think It was enough.
>
>
> > ---
>
> Will a patch change log be helpful here?
I realized I should write some change log, and the change log was meaningless.
So I left it blank.
>
> Regards,
> Markus

2020-04-06 09:04:13

by Oliver O'Halloran

[permalink] [raw]
Subject: Re: [PATCH v3] powerpc/powernv: add NULL check after kzalloc in opal_add_one_export

On Mon, Apr 6, 2020 at 11:15 AM Qiujun Huang <[email protected]> wrote:
>
> On Mon, Apr 6, 2020 at 3:06 AM Markus Elfring <[email protected]> wrote:
> >
> > > Here needs a NULL check.
> quite obvious?
> >
> > I find this change description questionable
> > (despite of a reasonable patch subject).
> >
> >
> > > Issue found by coccinelle.
> >
> > Would an information like “Generated by: scripts/coccinelle/null/kmerr.cocci”
> > be nicer?
> Yeah, but I think It was enough.

I didn't know we had that script in the kernel tree so I think it's a
good to mention that you used it. It might even help idiots like me
who write this sort of bug.

> > Will a patch change log be helpful here?
> I realized I should write some change log, and the change log was meaningless.
> So I left it blank.

The changelog is fine IMO. The point of a changelog is to tell a
reader doing git archeology why a change happened and this is
sufficent for that.

Reviewed-by: Oliver O'Halloran <[email protected]>

2020-04-06 09:25:19

by Qiujun Huang

[permalink] [raw]
Subject: Re: [PATCH v3] powerpc/powernv: add NULL check after kzalloc in opal_add_one_export

On Mon, Apr 6, 2020 at 5:01 PM Oliver O'Halloran <[email protected]> wrote:
>
> On Mon, Apr 6, 2020 at 11:15 AM Qiujun Huang <[email protected]> wrote:
> >
> > On Mon, Apr 6, 2020 at 3:06 AM Markus Elfring <[email protected]> wrote:
> > >
> > > > Here needs a NULL check.
> > quite obvious?
> > >
> > > I find this change description questionable
> > > (despite of a reasonable patch subject).
> > >
> > >
> > > > Issue found by coccinelle.
> > >
> > > Would an information like “Generated by: scripts/coccinelle/null/kmerr.cocci”
> > > be nicer?
> > Yeah, but I think It was enough.
>
> I didn't know we had that script in the kernel tree so I think it's a
> good to mention that you used it. It might even help idiots like me
> who write this sort of bug.

Yes, I will resend the patch. Thanks :-)

>
> > > Will a patch change log be helpful here?
> > I realized I should write some change log, and the change log was meaningless.
> > So I left it blank.
>
> The changelog is fine IMO. The point of a changelog is to tell a
> reader doing git archeology why a change happened and this is
> sufficent for that.

Get that.

>
> Reviewed-by: Oliver O'Halloran <[email protected]>

2020-04-06 10:03:14

by Markus Elfring

[permalink] [raw]
Subject: Re: [PATCH v3] powerpc/powernv: add NULL check after kzalloc in opal_add_one_export

>>>> Here needs a NULL check.
>> quite obvious?

I suggest to consider another fine-tuning for the wording also around
such “obvious” programming items.


>>> I find this change description questionable
>>> (despite of a reasonable patch subject).

I got further development concerns.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=a10c9c710f9ecea87b9f4bbb837467893b4bef01#n129

* Were changes mixed for different issues according to the diff code?

* I find it safer here to split specific changes into separate update steps
for a small patch series.

* Will the addition of the desired null pointer check qualify for
the specification of the tag “Fixes”?


>>> Will a patch change log be helpful here?
>> I realized I should write some change log, and the change log was meaningless.

Will any more adjustments happen for the discussed update suggestion
after the third patch version?


> The changelog is fine IMO. The point of a changelog is to tell a
> reader doing git archeology why a change happened and this is
> sufficent for that.

We might stumble on a different understanding for the affected “change logs”.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=a10c9c710f9ecea87b9f4bbb837467893b4bef01#n751

Would you like to follow the patch evolution a bit easier?

Regards,
Markus

2020-04-06 10:41:31

by Qiujun Huang

[permalink] [raw]
Subject: Re: [PATCH v3] powerpc/powernv: add NULL check after kzalloc in opal_add_one_export

On Mon, Apr 6, 2020 at 6:02 PM Markus Elfring <[email protected]> wrote:
>
> >>>> Here needs a NULL check.
> >> quite obvious?
>
> I suggest to consider another fine-tuning for the wording also around
> such “obvious” programming items.
>
>
> >>> I find this change description questionable
> >>> (despite of a reasonable patch subject).
>
> I got further development concerns.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=a10c9c710f9ecea87b9f4bbb837467893b4bef01#n129
>
> * Were changes mixed for different issues according to the diff code?
>
> * I find it safer here to split specific changes into separate update steps
> for a small patch series.
>
> * Will the addition of the desired null pointer check qualify for
> the specification of the tag “Fixes”?
>
>
> >>> Will a patch change log be helpful here?
> >> I realized I should write some change log, and the change log was meaningless.
>
> Will any more adjustments happen for the discussed update suggestion
> after the third patch version?
>
>
> > The changelog is fine IMO. The point of a changelog is to tell a
> > reader doing git archeology why a change happened and this is
> > sufficent for that.
>
> We might stumble on a different understanding for the affected “change logs”.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=a10c9c710f9ecea87b9f4bbb837467893b4bef01#n751
>
> Would you like to follow the patch evolution a bit easier?
>
> Regards,
> Markus

Thanks for the reply.
I should study the documentation first.
BTW, happy new week