| Message ID | 4616e325-e313-4078-9788-dd1e6e51b9e0@web.de |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: ses: Move a label in ses_enclosure_data_process() | expand |
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
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
>> 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
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
>> 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
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
>> 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
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
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;