[PATCH] scsi: ses: Move a label in ses_enclosure_data_process()

[PATCH] scsi: ses: Move a label in ses_enclosure_data_process()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 28 Dec 2023 15:38:09 +0100

The kfree() function was called in up to three cases by
the ses_enclosure_data_process() function during error handling
even if the passed variable contained a null pointer.
This issue was detected by using the Coccinelle software.

* Thus move the label “simple_populate” behind this kfree() call.

* Delete an initialisation (for the variable “buf”)
  which became unnecessary with this refactoring.

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

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index d7d0c35c58b8..e98f47d8206f 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -528,7 +528,7 @@ static void ses_enclosure_data_process(struct enclosure_device *edev,
 				       int create)
 {
 	u32 result;
-	unsigned char *buf = NULL, *type_ptr, *desc_ptr, *addl_desc_ptr = NULL;
+	unsigned char *buf, *type_ptr, *desc_ptr, *addl_desc_ptr = NULL;
 	int i, j, page7_len, len, components;
 	struct ses_device *ses_dev = edev->scratch;
 	int types = ses_dev->page1_num_types;
@@ -552,8 +552,8 @@ static void ses_enclosure_data_process(struct enclosure_device *edev,
 		goto simple_populate;
 	result = ses_recv_diag(sdev, 7, buf, len);
 	if (result) {
- simple_populate:
 		kfree(buf);
+simple_populate:
 		buf = NULL;
 		desc_ptr = NULL;
 		len = 0;
--
2.43.0

Re: [PATCH] scsi: ses: Move a label in ses_enclosure_data_process()

[James Bottomley]

On Thu, 2023-12-28 at 15:48 +0100, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 28 Dec 2023 15:38:09 +0100
> 
> The kfree() function was called in up to three cases by
> the ses_enclosure_data_process() function during error handling
> even if the passed variable contained a null pointer.
> This issue was detected by using the Coccinelle software.

Why is this an issue?  The whole point of having kfree(NULL) be a nop
is so we don't have to special case the free path.  The reason we do
that is because multiple special case paths through code leads to more
complex control flows and more potential bugs.  If coccinelle suddenly
thinks this is a problem, it's coccinelle that needs fixing.

James

Re: [PATCH] scsi: ses: Move a label in ses_enclosure_data_process()

[Julia Lawall]

On Fri, 29 Dec 2023, James Bottomley wrote:

> On Thu, 2023-12-28 at 15:48 +0100, Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Thu, 28 Dec 2023 15:38:09 +0100
> >
> > The kfree() function was called in up to three cases by
> > the ses_enclosure_data_process() function during error handling
> > even if the passed variable contained a null pointer.
> > This issue was detected by using the Coccinelle software.
>
> Why is this an issue?  The whole point of having kfree(NULL) be a nop
> is so we don't have to special case the free path.  The reason we do
> that is because multiple special case paths through code leads to more
> complex control flows and more potential bugs.  If coccinelle suddenly
> thinks this is a problem, it's coccinelle that needs fixing.

Coccinelle doesn't think anything.  Markus for some reason thinks it's a
problem and uses Coccinelle to find occurrences of it.

julia

Re: scsi: ses: Move a label in ses_enclosure_data_process()

[Markus Elfring]

>> The kfree() function was called in up to three cases by
>> the ses_enclosure_data_process() function during error handling
>> even if the passed variable contained a null pointer.
>> This issue was detected by using the Coccinelle software.
>
> Why is this an issue?  The whole point of having kfree(NULL) be a nop

Such “a nop” can trigger the allocation of extra data processing resources,
can't it?


> is so we don't have to special case the free path.

A bit more development attention can hopefully connect the mentioned label
with a more appropriate jump target directly.


>                                                     The reason we do
> that is because multiple special case paths through code leads to more
> complex control flows and more potential bugs.

You probably know some advices from another information source.

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


>                                                 If coccinelle suddenly
> thinks this is a problem, it's coccinelle that needs fixing.

This software tool can help to point source code places out for further considerations.
The search patterns are evolving accordingly.

Regards,
Markus

Re: scsi: ses: Move a label in ses_enclosure_data_process()

[James Bottomley]

On Sat, 2023-12-30 at 08:04 +0100, Markus Elfring wrote:
> > > The kfree() function was called in up to three cases by
> > > the ses_enclosure_data_process() function during error handling
> > > even if the passed variable contained a null pointer.
> > > This issue was detected by using the Coccinelle software.
> > 
> > Why is this an issue?  The whole point of having kfree(NULL) be a
> > nop
> 
> Such “a nop” can trigger the allocation of extra data processing
> resources, can't it?

No.

> > is so we don't have to special case the free path.
> 
> A bit more development attention can hopefully connect the mentioned
> label with a more appropriate jump target directly.

That's making the flow more complex as I pointed out in my initial
email.

> >                                                     The reason we
> > do that is because multiple special case paths through code leads
> > to more complex control flows and more potential bugs.
> 
> You probably know some advices from another information source.
> 
> 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
> 

Yes, but it's about using staged deallocation at the end of the
function instead of in the if loops.  That's to *simplify* the exit
chain and make the error legs less error prone because the teardown
isn't repeated in if bodies.  It has no bearing on what you just tried
to do.

> >                                               If coccinelle
> > suddenly thinks this is a problem, it's coccinelle that needs
> > fixing.
> 
> This software tool can help to point source code places out for
> further considerations. The search patterns are evolving accordingly.

The pattern is wrong because kfree(NULL) exists as a teardown
simplification.

James

Re: scsi: ses: Move a label in ses_enclosure_data_process()

[Markus Elfring]

>> You probably know some advices from another information source.
>>
>> 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
>>
>
> Yes, but it's about using staged deallocation at the end of the
> function instead of in the if loops.  That's to *simplify* the exit
> chain and make the error legs less error prone because the teardown
> isn't repeated in if bodies.  It has no bearing on what you just tried
> to do.

I got the impression that there is a general conflict involved
according to different programming styles.


>>>                                               If coccinelle
>>> suddenly thinks this is a problem, it's coccinelle that needs
>>> fixing.
>>
>> This software tool can help to point source code places out for
>> further considerations. The search patterns are evolving accordingly.
>
> The pattern is wrong because kfree(NULL) exists as a teardown simplification.

It might be convenient to view in this way.

If you would dare to follow advice from goto chains in a strict way,
I imagine that you can tend to stress the attention for more useful
data processing a bit more than such a redundant function call.

Regards,
Markus

Re: scsi: ses: Move a label in ses_enclosure_data_process()

[James Bottomley]

On Sat, 2023-12-30 at 14:36 +0100, Markus Elfring wrote:
> > > You probably know some advices from another information source.
> > > 
> > > 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
> > > 
> > 
> > Yes, but it's about using staged deallocation at the end of the
> > function instead of in the if loops.  That's to *simplify* the exit
> > chain and make the error legs less error prone because the teardown
> > isn't repeated in if bodies.  It has no bearing on what you just
> > tried to do.
> 
> I got the impression that there is a general conflict involved
> according to different programming styles.

There isn't; you simply seem to have misunderstood what the above 
resource is actually saying.

> > > >                                               If coccinelle
> > > > suddenly thinks this is a problem, it's coccinelle that needs
> > > > fixing.
> > > 
> > > This software tool can help to point source code places out for
> > > further considerations. The search patterns are evolving
> > > accordingly.
> > 
> > The pattern is wrong because kfree(NULL) exists as a teardown
> > simplification.
> 
> It might be convenient to view in this way.
> 
> If you would dare to follow advice from goto chains in a strict way,
> I imagine that you can tend to stress the attention for more useful
> data processing a bit more than such a redundant function call.

It's about maintainability and simplicity.  Eliminating kfree(NULL)
doesn't simplify most code, it just makes the exit paths more complex
as I've said for the third time now.  It could possibly be done in
extremely performance critical situations for good and well documented
reason, but that's only a tiny fraction of the kernel and it certainly
doesn't apply here.

James

Re: scsi: ses: Move a label in ses_enclosure_data_process()

[Markus Elfring]

>> If you would dare to follow advice from goto chains in a strict way,
>> I imagine that you can tend to stress the attention for more useful
>> data processing a bit more than such a redundant function call.
>
> It's about maintainability and simplicity.  Eliminating kfree(NULL)
> doesn't simplify most code,

I find it easy to avoid such a call in the affected and concrete
function implementation.


>                             it just makes the exit paths more complex

Where is undesirable software complexity here in the repositioning
of the label “simple_populate” before the statement “buf = NULL;”?

Regards,
Markus

Re: scsi: ses: Move a label in ses_enclosure_data_process()

[James Bottomley]

On Sat, 2023-12-30 at 15:25 +0100, Markus Elfring wrote:
> > > If you would dare to follow advice from goto chains in a strict
> > > way, I imagine that you can tend to stress the attention for more
> > > useful data processing a bit more than such a redundant function
> > > call.
> > 
> > It's about maintainability and simplicity.  Eliminating kfree(NULL)
> > doesn't simplify most code,
> 
> I find it easy to avoid such a call in the affected and concrete
> function implementation.

I find it easy to fall down stairs nowadays; that doesn't make it a
necessary or even desirable thing to do.

> >                             it just makes the exit paths more
> > complex
> 
> Where is undesirable software complexity here in the repositioning
> of the label “simple_populate” before the statement “buf = NULL;”?

We don't just apply patches because we can: code churn is inimical to
software maintenance and backporting, so every patch has an application
cost.  The value provided by any patch has to be greater than that
cost. kfree(NULL) is an expected operation so there's little value in
avoiding it and certainly not enough to overcome the patch application
cost.

James