[PATCH 0/3] drm: property: Adjustments for three function implementations

[PATCH 0/3] drm: property: Adjustments for three function implementations

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Dec 2023 10:26:36 +0100

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (3):
  One function call less in drm_property_create() after error detection
  Delete an unnecessary initialisation in drm_property_create()
  Improve four size determinations

 drivers/gpu/drm/drm_property.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

--
2.43.0

[PATCH 1/3] drm: property: One function call less in drm_property_create() after error detection

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Dec 2023 08:44:37 +0100

The kfree() function was called in one case by the
drm_property_create() function during error handling
even if the passed data structure member contained a null pointer.
This issue was detected by using the Coccinelle software.

Thus use another label.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/drm_property.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
index 596272149a35..3440f4560e6e 100644
--- a/drivers/gpu/drm/drm_property.c
+++ b/drivers/gpu/drm/drm_property.c
@@ -117,7 +117,7 @@ struct drm_property *drm_property_create(struct drm_device *dev,
 		property->values = kcalloc(num_values, sizeof(uint64_t),
 					   GFP_KERNEL);
 		if (!property->values)
-			goto fail;
+			goto free_property;
 	}

 	ret = drm_mode_object_add(dev, &property->base, DRM_MODE_OBJECT_PROPERTY);
@@ -135,6 +135,7 @@ struct drm_property *drm_property_create(struct drm_device *dev,
 	return property;
 fail:
 	kfree(property->values);
+free_property:
 	kfree(property);
 	return NULL;
 }
--
2.43.0

Re: [PATCH 1/3] drm: property: One function call less in drm_property_create() after error detection

[Michel Dänzer]

On 2023-12-26 10:38, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 26 Dec 2023 08:44:37 +0100
> 
> The kfree() function was called in one case by the
> drm_property_create() function during error handling
> even if the passed data structure member contained a null pointer.
> This issue was detected by using the Coccinelle software.
> 
> Thus use another label.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/gpu/drm/drm_property.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
> index 596272149a35..3440f4560e6e 100644
> --- a/drivers/gpu/drm/drm_property.c
> +++ b/drivers/gpu/drm/drm_property.c
> @@ -117,7 +117,7 @@ struct drm_property *drm_property_create(struct drm_device *dev,
>  		property->values = kcalloc(num_values, sizeof(uint64_t),
>  					   GFP_KERNEL);
>  		if (!property->values)
> -			goto fail;
> +			goto free_property;
>  	}
> 
>  	ret = drm_mode_object_add(dev, &property->base, DRM_MODE_OBJECT_PROPERTY);
> @@ -135,6 +135,7 @@ struct drm_property *drm_property_create(struct drm_device *dev,
>  	return property;
>  fail:
>  	kfree(property->values);
> +free_property:
>  	kfree(property);
>  	return NULL;
>  }
> --
> 2.43.0
> 

This change is pointless at best, kfree(NULL) works fine.


Out of curiosity, what exactly did Coccinelle report?


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer

Re: [PATCH 1/3] drm: property: One function call less in drm_property_create() after error detection

[Markus Elfring]

>> The kfree() function was called in one case by the
>> drm_property_create() function during error handling
>> even if the passed data structure member contained a null pointer.
>> This issue was detected by using the Coccinelle software.
>>
>> Thus use another label.
…
>> +++ b/drivers/gpu/drm/drm_property.c
>> @@ -117,7 +117,7 @@ struct drm_property *drm_property_create(struct drm_device *dev,
>>  		property->values = kcalloc(num_values, sizeof(uint64_t),
>>  					   GFP_KERNEL);
>>  		if (!property->values)
>> -			goto fail;
>> +			goto free_property;
>>  	}
>>
>>  	ret = drm_mode_object_add(dev, &property->base, DRM_MODE_OBJECT_PROPERTY);
>> @@ -135,6 +135,7 @@ struct drm_property *drm_property_create(struct drm_device *dev,
>>  	return property;
>>  fail:
>>  	kfree(property->values);
>> +free_property:
>>  	kfree(property);
>>  	return NULL;
>>  }
…
> This change is pointless at best, kfree(NULL) works fine.

* Would you interpret such a special function call as redundant?

* Do you find advices applicable from another information source
  also for this function implementation?
  https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources


> Out of curiosity, what exactly did Coccinelle report?

Some SmPL scripts from my own selection tend to point questionable implementation details out.

Regards,
Markus

Re: [PATCH 1/3] drm: property: One function call less in drm_property_create() after error detection

[Michel Dänzer]

On 2024-01-03 17:24, Markus Elfring wrote:
> 
>> Out of curiosity, what exactly did Coccinelle report?
> 
> Some SmPL scripts from my own selection tend to point questionable implementation details out.

That doesn't answer my question.

Without seeing the actual Coccinelle report, I'll assume that it didn't actually call for this change.


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer

Re: [1/3] drm: property: One function call less in drm_property_create() after error detection

[Markus Elfring]

>>> Out of curiosity, what exactly did Coccinelle report?
>>
>> Some SmPL scripts from my own selection tend to point questionable implementation details out.
>
> That doesn't answer my question.

It should.


> Without seeing the actual Coccinelle report,

There is no “official” report according to the discussed patch which is triggered
by known advices for the application of labels in goto chains.


> I'll assume that it didn't actually call for this change.

Software development opinions are evolving accordingly.

Regards,
Markus

Re: [1/3] drm: property: One function call less in drm_property_create() after error detection

[Michel Dänzer]

On 2024-01-03 19:08, Markus Elfring wrote:
> 
>> Without seeing the actual Coccinelle report,
> 
> There is no “official” report according to the discussed patch which is triggered
> by known advices for the application of labels in goto chains.

The commit log says:

 This issue was detected by using the Coccinelle software.

Either that's inaccurate then, or you should be able to provide the corresponding output from Coccinelle.


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer

Re: [1/3] drm: property: One function call less in drm_property_create() after error detection

[Markus Elfring]

> The commit log says:
>
>  This issue was detected by using the Coccinelle software.
>
> Either that's inaccurate then,

No.


> or you should be able to provide the corresponding output from Coccinelle.

Do you find data (like the following) more helpful for the adjustment
of affected implementation details?


Markus_Elfring@Sonne:…/Projekte/Linux/next-analyses> LANG=C git status && spatch …/Projekte/Coccinelle/janitor/show_jumps_to_kfree_with_null_pointer.cocci drivers/gpu/drm/drm_property.c
HEAD detached at next-20240104
…
@@ -114,9 +114,6 @@ struct drm_property *drm_property_create
        property->dev = dev;

        if (num_values) {
-               property->values = kcalloc(num_values, sizeof(uint64_t),
-                                          GFP_KERNEL);
-               if (!property->values)
                        goto fail;
        }

@@ -133,8 +130,6 @@ struct drm_property *drm_property_create
        list_add_tail(&property->head, &dev->mode_config.property_list);

        return property;
-fail:
-       kfree(property->values);
        kfree(property);
        return NULL;
 }


How do you think about to extend the application of script variants
for the semantic patch language?

Regards,
Markus

Re: [PATCH 1/3] drm: property: One function call less in drm_property_create() after error detection

[Dan Carpenter]

On Wed, Jan 03, 2024 at 06:18:13PM +0100, Michel Dänzer wrote:
> On 2024-01-03 17:24, Markus Elfring wrote:
> > 
> >> Out of curiosity, what exactly did Coccinelle report?
> > 
> > Some SmPL scripts from my own selection tend to point questionable implementation details out.
> 
> That doesn't answer my question.
> 
> Without seeing the actual Coccinelle report, I'll assume that it didn't actually call for this change.

This isn't one of the Coccinelle scripts which ship with the kernel,
it's something that Markus wrote himself.

regards,
dan carpenter

[PATCH 2/3] drm: property: Delete an unnecessary initialisation in drm_property_create()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Dec 2023 08:46:12 +0100

The variable “property” will eventually be set to an appropriate pointer
a bit later. Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/drm_property.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
index 3440f4560e6e..ea365f00e890 100644
--- a/drivers/gpu/drm/drm_property.c
+++ b/drivers/gpu/drm/drm_property.c
@@ -98,7 +98,7 @@ struct drm_property *drm_property_create(struct drm_device *dev,
 					 u32 flags, const char *name,
 					 int num_values)
 {
-	struct drm_property *property = NULL;
+	struct drm_property *property;
 	int ret;

 	if (WARN_ON(!drm_property_flags_valid(flags)))
--
2.43.0

[PATCH 3/3] drm: property: Improve four size determinations

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 26 Dec 2023 09:45:36 +0100

Replace the specification of data structures by pointer dereferences
as the parameter for the operator “sizeof” to make the corresponding size
determination a bit safer according to the Linux coding style convention.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/drm_property.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
index ea365f00e890..1fe54cbf1c83 100644
--- a/drivers/gpu/drm/drm_property.c
+++ b/drivers/gpu/drm/drm_property.c
@@ -107,7 +107,7 @@ struct drm_property *drm_property_create(struct drm_device *dev,
 	if (WARN_ON(strlen(name) >= DRM_PROP_NAME_LEN))
 		return NULL;

-	property = kzalloc(sizeof(struct drm_property), GFP_KERNEL);
+	property = kzalloc(sizeof(*property), GFP_KERNEL);
 	if (!property)
 		return NULL;

@@ -418,7 +418,7 @@ int drm_property_add_enum(struct drm_property *property,
 	if (WARN_ON(index >= property->num_values))
 		return -EINVAL;

-	prop_enum = kzalloc(sizeof(struct drm_property_enum), GFP_KERNEL);
+	prop_enum = kzalloc(sizeof(*prop_enum), GFP_KERNEL);
 	if (!prop_enum)
 		return -ENOMEM;

@@ -560,10 +560,10 @@ drm_property_create_blob(struct drm_device *dev, size_t length,
 	struct drm_property_blob *blob;
 	int ret;

-	if (!length || length > INT_MAX - sizeof(struct drm_property_blob))
+	if (!length || length > INT_MAX - sizeof(*blob))
 		return ERR_PTR(-EINVAL);

-	blob = kvzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL);
+	blob = kvzalloc(sizeof(*blob) + length, GFP_KERNEL);
 	if (!blob)
 		return ERR_PTR(-ENOMEM);

--
2.43.0

Re: [PATCH 3/3] drm: property: Improve four size determinations

[Simon Ser]

The whole series is:

Reviewed-by: Simon Ser <contact@emersion.fr>