| Message ID | 0be7fee0-64f7-fa02-0337-51376677343e@users.sourceforge.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/22/16 11:33, SF Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Thu, 22 Sep 2016 10:06:50 +0200 > > The of_node_put() function was called in some cases > by the tilcdc_convert_slave_node() function during error handling > even if the passed variable contained a null pointer. > > * Adjust jump targets according to the Linux coding style convention. > > * Split a condition check for resource detection failures so that > each pointer from these function calls will be checked immediately. > > See also background information: > Topic "CWE-754: Improper check for unusual or exceptional conditions" > Link: https://cwe.mitre.org/data/definitions/754.html > I don't really agree with this patch. There is no harm in calling of_node_put() with NULL as an argument and because of that there is no point in making the function more complex and harder to maintain. Best regards, Jyri > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c | 28 +++++++++++++++++----------- > 1 file changed, 17 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c > index 6204405..6ee5865 100644 > --- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c > +++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c > @@ -209,25 +209,27 @@ void __init tilcdc_convert_slave_node(void) > return; > > lcdc = of_find_matching_node(NULL, tilcdc_of_match); > - slave = of_find_matching_node(NULL, tilcdc_slave_of_match); > + if (!of_device_is_available(lcdc)) > + goto free_table; > > - if (!slave || !of_device_is_available(lcdc)) > - goto out; > + slave = of_find_matching_node(NULL, tilcdc_slave_of_match); > + if (!slave) > + goto put_node_lcdc; > > i2c = of_parse_phandle(slave, "i2c", 0); > if (!i2c) { > pr_err("%s: Can't find i2c node trough phandle\n", __func__); > - goto out; > + goto put_node_slave; > } > > overlay = tilcdc_get_overlay(&kft); > if (!overlay) > - goto out; > + goto put_node_i2c; > > encoder = of_find_matching_node(overlay, tilcdc_tda998x_of_match); > if (!encoder) { > pr_err("%s: Failed to find tda998x node\n", __func__); > - goto out; > + goto put_node_i2c; > } > > tilcdc_copy_props(slave, encoder, tilcdc_slave_props, &kft); > @@ -238,10 +240,10 @@ void __init tilcdc_convert_slave_node(void) > continue; > if (!strncmp("i2c", (char *)prop->value, prop->length)) > if (tilcdc_prop_str_update(prop, i2c->full_name, &kft)) > - goto out; > + goto put_node_fragment; > if (!strncmp("lcdc", (char *)prop->value, prop->length)) > if (tilcdc_prop_str_update(prop, lcdc->full_name, &kft)) > - goto out; > + goto put_node_fragment; > } > > tilcdc_node_disable(slave); > @@ -252,12 +254,16 @@ void __init tilcdc_convert_slave_node(void) > else > pr_info("%s: ti,tilcdc,slave node successfully converted\n", > __func__); > -out: > - kfree_table_free(&kft); > + put_node_fragment: > + of_node_put(fragment); > + put_node_i2c: > of_node_put(i2c); > + put_node_slave: > of_node_put(slave); > + put_node_lcdc: > of_node_put(lcdc); > - of_node_put(fragment); > + free_table: > + kfree_table_free(&kft); > } > > int __init tilcdc_slave_compat_init(void) >
>> The of_node_put() function was called in some cases >> by the tilcdc_convert_slave_node() function during error handling >> even if the passed variable contained a null pointer. >> >> * Adjust jump targets according to the Linux coding style convention. >> >> * Split a condition check for resource detection failures so that >> each pointer from these function calls will be checked immediately. >> >> See also background information: >> Topic "CWE-754: Improper check for unusual or exceptional conditions" >> Link: https://cwe.mitre.org/data/definitions/754.html >> > > I don't really agree with this patch. This kind of feedback can be fine at first glance. > There is no harm in calling of_node_put() with NULL as an argument The cost of additional function calls will be eventually not noticed just because they belong to an exception handling implementation so far. > and because of that there is no point in making the function more complex There is inherent software complexity involved. > and harder to maintain. How do you think about to discuss this aspect a bit more? I suggest to reconsider this design detail if it is really acceptable for the safe implementation of such a software module. * How much will it matter in general that one function call was performed in this use case without checking its return value immediately? * Should it usually be determined quicker if a required resource could be acquired before trying the next allocation? Regards, Markus
On 09/22/16 21:38, SF Markus Elfring wrote: >>> The of_node_put() function was called in some cases >>> by the tilcdc_convert_slave_node() function during error handling >>> even if the passed variable contained a null pointer. >>> >>> * Adjust jump targets according to the Linux coding style convention. >>> >>> * Split a condition check for resource detection failures so that >>> each pointer from these function calls will be checked immediately. >>> >>> See also background information: >>> Topic "CWE-754: Improper check for unusual or exceptional conditions" >>> Link: https://cwe.mitre.org/data/definitions/754.html >>> >> >> I don't really agree with this patch. > > This kind of feedback can be fine at first glance. > > >> There is no harm in calling of_node_put() with NULL as an argument > > The cost of additional function calls will be eventually not noticed > just because they belong to an exception handling implementation so far. > > >> and because of that there is no point in making the function more complex > > There is inherent software complexity involved. > I think the "if (node)" in the of_node_put() is there on purpose, because it potentially saves the caller one extra if()-statement and keeps the caller code simpler. > >> and harder to maintain. > > How do you think about to discuss this aspect a bit more? > Keeping the goto labels in right order needs precision and can lead to subtle errors. Sometimes there is no way to avoid that, but here there is. Best regards, Jyri
> I think the "if (node)" in the of_node_put() is there on purpose, Yes, of course. Does such an implementation detail correspond to a general software design pattern? > because it potentially saves the caller one extra if()-statement This can occasionally happen. > and keeps the caller code simpler. A special view on software simplicity can also lead to questionable intermediate function implementation, can't it? > Keeping the goto labels in right order needs precision I can agree to this view. > and can lead to subtle errors. The management of jump labels is just another software development challenge as usual, isn't it? > Sometimes there is no way to avoid that, How do you think about to clarify the constraints which you imagine a bit more? > but here there is. I disagree to this conclusion. Would you like to care a bit more for efficiency and software correctness around the discussed exception handling? Regards, Markus
On 09/23/16 10:36, SF Markus Elfring wrote: >> I think the "if (node)" in the of_node_put() is there on purpose, > > Yes, of course. > > Does such an implementation detail correspond to a general software design pattern? > Yes it does. For instance standard malloc()/free() implementation [1]. > >> because it potentially saves the caller one extra if()-statement > > This can occasionally happen. > > >> and keeps the caller code simpler. > > A special view on software simplicity can also lead to questionable intermediate > function implementation, can't it? > I don't really follow. But in any case I do not see anything questionable in the current tilcdc_convert_slave_node() implementation. > >> Keeping the goto labels in right order needs precision > > I can agree to this view. > > >> and can lead to subtle errors. > > The management of jump labels is just another software development challenge > as usual, isn't it? > Yes. But usually it pays of to avoid complexity when possible. > >> Sometimes there is no way to avoid that, > > How do you think about to clarify the constraints which you imagine a bit more? > If the the of_node_put() behaviour would not be specified with null pointer as parameter, there would be such a constraint. I am beginning to have a feeling that this discussion is not going anywhere. > >> but here there is. > > I disagree to this conclusion. > > Would you like to care a bit more for efficiency and software correctness > around the discussed exception handling? > No, I would not. I think we have reached the bottom of this discussion. For the moment I have more important tasks to do. Best regards, Jyri [1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/free.html
>> A special view on software simplicity can also lead to questionable intermediate >> function implementation, can't it? > > I don't really follow. But in any case I do not see anything > questionable in the current tilcdc_convert_slave_node() implementation. I identified update candidates there like the following. 1. Delayed checking for null pointers … if (!slave || !of_device_is_available(lcdc)) … 2. Usage of a single jump label for (too many?) cases … goto out; … Can the corresponding exception handling become also a bit more efficient? >> Would you like to care a bit more for efficiency and software correctness >> around the discussed exception handling? > > No, I would not. Thanks for this information. I hope that the software situation can also be improved around this design aspect somehow. > For the moment I have more important tasks to do. I know also that various open issues are competing for your software development attention as usual. Regards, Markus
On Thu, Sep 22, 2016 at 2:38 PM, SF Markus Elfring <elfring@users.sourceforge.net> wrote: >>> The of_node_put() function was called in some cases >>> by the tilcdc_convert_slave_node() function during error handling >>> even if the passed variable contained a null pointer. >>> >>> * Adjust jump targets according to the Linux coding style convention. >>> >>> * Split a condition check for resource detection failures so that >>> each pointer from these function calls will be checked immediately. >>> >>> See also background information: >>> Topic "CWE-754: Improper check for unusual or exceptional conditions" >>> Link: https://cwe.mitre.org/data/definitions/754.html >>> >> >> I don't really agree with this patch. > > This kind of feedback can be fine at first glance. > > >> There is no harm in calling of_node_put() with NULL as an argument > > The cost of additional function calls will be eventually not noticed > just because they belong to an exception handling implementation so far. iirc, there are Coccinelle rules that find code with unnecessary null checks and removes them.. Although you probably made this complex enough that cocinelle would not find it. That is not a complement. One should not make error handling/cleanup more complex than needed. BR, -R > >> and because of that there is no point in making the function more complex > > There is inherent software complexity involved. > > >> and harder to maintain. > > How do you think about to discuss this aspect a bit more? > > > I suggest to reconsider this design detail if it is really acceptable > for the safe implementation of such a software module. > > * How much will it matter in general that one function call was performed > in this use case without checking its return value immediately? > > * Should it usually be determined quicker if a required resource > could be acquired before trying the next allocation? > > Regards, > Markus > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> iirc, there are Coccinelle rules that find code with unnecessary null > checks and removes them. This kind of software change is not needed here. I find that a corresponding return value check happens one function call too late. > Although you probably made this complex enough that cocinelle would > not find it. That is not a complement. I imagine that scripts for the semantic patch language can find more source code places where questionable disjunctions are used so far. Would you dare to split any more condition checks? > One should not make error handling/cleanup more complex than needed. I see a need to improve not only correctness there but also a bit of software efficiency. Regards, Markus
On Fri, Sep 23, 2016 at 7:19 AM, SF Markus Elfring <elfring@users.sourceforge.net> wrote: >> iirc, there are Coccinelle rules that find code with unnecessary null >> checks and removes them. > > This kind of software change is not needed here. > > I find that a corresponding return value check happens one function call > too late. > I think you misunderstand my point.. which is that additional conditional checks wrapping a function call to something that already checks for null should be removed.. not introduced. > >> Although you probably made this complex enough that cocinelle would >> not find it. That is not a complement. > > I imagine that scripts for the semantic patch language can find more > source code places where questionable disjunctions are used so far. > Would you dare to split any more condition checks? > > >> One should not make error handling/cleanup more complex than needed. > > I see a need to improve not only correctness there but also a bit of > software efficiency. If you can measure any performance difference and present some results (esp. considering that this is something that just happens when the driver is loaded), then we'll talk. Until then, please don't send this sort of patch. Thank you. BR, -R > Regards, > Markus
>> I see a need to improve not only correctness there but also a bit of >> software efficiency. > > If you can measure any performance difference and present some results > (esp. considering that this is something that just happens when the > driver is loaded), then we'll talk. Are you really interested to increase your software development attention for a quicker exception handling implementation in this function? > Until then, please don't send this sort of patch. Thank you. Will benchmark statistics change such a change rejection ever? Regards, Markus
On Fri, Sep 23, 2016 at 8:17 AM, SF Markus Elfring <elfring@users.sourceforge.net> wrote: >>> I see a need to improve not only correctness there but also a bit of >>> software efficiency. >> >> If you can measure any performance difference and present some results >> (esp. considering that this is something that just happens when the >> driver is loaded), then we'll talk. > > Are you really interested to increase your software development attention > for a quicker exception handling implementation in this function? > No one is interested in making error handling more complex for a non-existent benefit ;-) > >> Until then, please don't send this sort of patch. Thank you. > > Will benchmark statistics change such a change rejection ever? If you could demonstrate a real benefit to additional complexity for something that actually matters (ie. not something like this that runs once at bootup) I would care. I don't recommend that you waste your time, since there is approximately nothing in modesetting path that happens with a high enough frequency to benefit from such a micro-optimization. Especially not initialization code that runs once at boot up. Fixing actual bugs is useful and valuable. Premature "optimization" at the expense of extra complexity is very much not useful. BR, -R > Regards, > Markus
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c index 6204405..6ee5865 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_slave_compat.c @@ -209,25 +209,27 @@ void __init tilcdc_convert_slave_node(void) return; lcdc = of_find_matching_node(NULL, tilcdc_of_match); - slave = of_find_matching_node(NULL, tilcdc_slave_of_match); + if (!of_device_is_available(lcdc)) + goto free_table; - if (!slave || !of_device_is_available(lcdc)) - goto out; + slave = of_find_matching_node(NULL, tilcdc_slave_of_match); + if (!slave) + goto put_node_lcdc; i2c = of_parse_phandle(slave, "i2c", 0); if (!i2c) { pr_err("%s: Can't find i2c node trough phandle\n", __func__); - goto out; + goto put_node_slave; } overlay = tilcdc_get_overlay(&kft); if (!overlay) - goto out; + goto put_node_i2c; encoder = of_find_matching_node(overlay, tilcdc_tda998x_of_match); if (!encoder) { pr_err("%s: Failed to find tda998x node\n", __func__); - goto out; + goto put_node_i2c; } tilcdc_copy_props(slave, encoder, tilcdc_slave_props, &kft); @@ -238,10 +240,10 @@ void __init tilcdc_convert_slave_node(void) continue; if (!strncmp("i2c", (char *)prop->value, prop->length)) if (tilcdc_prop_str_update(prop, i2c->full_name, &kft)) - goto out; + goto put_node_fragment; if (!strncmp("lcdc", (char *)prop->value, prop->length)) if (tilcdc_prop_str_update(prop, lcdc->full_name, &kft)) - goto out; + goto put_node_fragment; } tilcdc_node_disable(slave); @@ -252,12 +254,16 @@ void __init tilcdc_convert_slave_node(void) else pr_info("%s: ti,tilcdc,slave node successfully converted\n", __func__); -out: - kfree_table_free(&kft); + put_node_fragment: + of_node_put(fragment); + put_node_i2c: of_node_put(i2c); + put_node_slave: of_node_put(slave); + put_node_lcdc: of_node_put(lcdc); - of_node_put(fragment); + free_table: + kfree_table_free(&kft); } int __init tilcdc_slave_compat_init(void)