linux-kernel - Re: [PATCH] of: Fix issue where code would fall through to error case.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <583C4D83.6040800@gmail.com>
Date:   Mon, 28 Nov 2016 07:30:11 -0800
From:   Frank Rowand <frowand.list@...il.com>
To:     Rob Herring <robh+dt@...nel.org>,
        Moritz Fischer <moritz.fischer.private@...il.com>
Cc:     Moritz Fischer <moritz.fischer@...us.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Pantelis Antoniou <pantelis.antoniou@...sulko.com>,
        moritz@...e-entropy.org,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH] of: Fix issue where code would fall through to error
 case.

On 11/26/16 13:39, Frank Rowand wrote:
> On 11/23/16 13:58, Rob Herring wrote:
>> On Thu, Nov 17, 2016 at 6:10 PM, Moritz Fischer
>> <moritz.fischer.private@...il.com> wrote:
>>> On Thu, Nov 17, 2016 at 4:02 PM, Frank Rowand <frowand.list@...il.com> wrote:
>>>> On 11/17/16 15:40, Frank Rowand wrote:
>>>>> On 11/17/16 15:25, Moritz Fischer wrote:
>>>>>> No longer fall through into the error case that prints out
>>>>>> an error if no error (err = 0) occurred.
>>>>>>
>>>>>> Fixes d9181b20a83(of: Add back an error message, restructured)
>>>>>> Signed-off-by: Moritz Fischer <moritz.fischer@...us.com>
>>>>>> ---
>>>>>>  drivers/of/resolver.c | 6 +++++-
>>>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
>>>>>> index 783bd09..785076d 100644
>>>>>> --- a/drivers/of/resolver.c
>>>>>> +++ b/drivers/of/resolver.c
>>>>>> @@ -358,9 +358,13 @@ int of_resolve_phandles(struct device_node *overlay)
>>>>>>
>>>>>>              err = update_usages_of_a_phandle_reference(overlay, prop, phandle);
>>>>>>              if (err)
>>>>>> -                    break;
>>>>>> +                    goto err_out;
>>>>>>      }
>>>>>>
>>>>>> +    of_node_put(tree_symbols);
>>>>>> +
>>>>>> +    return 0;
>>>>>> +
>>>>>>  err_out:
>>>>>>      pr_err("overlay phandle fixup failed: %d\n", err);
>>>>>>  out:
>>>>>
>>>>> Thanks for catching that.
>>>>>
>>>>> Rob, please apply.
>>>>>
>>>>> Reviewed-by: Frank Rowand <frank.rowand@...sony.com>
>>>>>
>>>>> -Frank
>>>>
>>>> On second thought, isn't the common pattern when clean up is needed for
>>>> both the no-error path and the error path something like:
>>>>
>>>>
>>>>         out:
>>>>                 of_node_put(tree_symbols);
>>>>                 return err;
>>>>
>>>>         err_out:
>>>>                 pr_err("overlay phandle fixup failed: %d\n", err);
>>>>                 goto out;
>>>>         }
>>>>
>>>>
>>>> I don't have a strong opinion, whatever Rob wants to take is fine with me.
>>>
>>> Same here. I tried to avoid the jumping back part, but if that's the
>>> common pattern,
>>> I can submit a v2 doing that instead.
>>
>> Both are ugly. Just do:
>>
>> if (err)
>>   pr_err(...);
>>
>> Rob
> 
> Agreed.  Thanks for the touch of sanity Rob.
> 
> -Frank

I succumbed to looking only at the few lines of code above and not the
fuller context of the file that the patch applies to.

The proposed patch was fixing the problem that a normal completion
of the for loop was falling through into the err_out label.  So what
looks cleaner ("if (err) pr_err(...)") is actually not correct.

-Frank