Re: [PATCH] clk: qcom: Fix a possible NULL pointer dereferencing

[Vivek Gautam <vivek.gautam@codeaurora.org>]

Hi,

On 01/06/2017 04:09 AM, Stephen Boyd wrote:
> On 01/05, Vivek Gautam wrote:
>> On 01/05/2017 07:50 PM, Andy Gross wrote:
>>> On Thu, Jan 05, 2017 at 02:25:25PM +0530, Vivek Gautam wrote:
>>>> Assign num_parents as 0 while registering fixed rate clocks
>>>> in _qcom_cc_register_board_clk(), to make sure the clk framework
>>>> doesn't dereference parent.
>>>>
>>>> Fixes: ee15faffef11 ("clk: qcom: common: Add API to register board clocks backwards compatibly")
>>>>
>>>> Cc: Georgi Djakov<georgi.djakov@linaro.org>
>>>> Signed-off-by: Vivek Gautam<vivek.gautam@codeaurora.org>
>>>> ---
>>>>
>>>> Based on 'clk-next'. Build tested.
>>>>
>>>>   drivers/clk/qcom/common.c | 1 +
>>>>   1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
>>>> index cfab7b400381..df004ead1bef 100644
>>>> --- a/drivers/clk/qcom/common.c
>>>> +++ b/drivers/clk/qcom/common.c
>>>> @@ -157,6 +157,7 @@ static int _qcom_cc_register_board_clk(struct device *dev, const char *path,
>>>>   		init_data.name = path;
>>>>   		init_data.ops = &clk_fixed_rate_ops;
>>>> +		init_data.num_parents = 0;
>>> It seems like there was a initializer in the declaration but it was { } instead
>>> of { 0 }.
>>>
>>> Was the original intent to make this structure initialized to 0?  If so, perhaps
>>> it should be fixed above in the initializer.
>> yes, i think we intend to initialize the clock init data to 0, and thus
>> we should do that during declaration.
>> Will modify and re-spin the patch.
>>
> What's the error exactly? Do you have some call stack/crash that
> could be put in the commit text?

I got a crash dump from clk_register, when i was working on qmp phy
driver patches. This came when clk_init_data was not initilized.
So i had to explicitly assign num_parents to 0.

> It was my understanding that GCC allows braces without anything
> inside to initialize structures to 0, so I'm confused what's
> wrong here.

You are right. My understanding, that we need to initialize a structure
with {0}, was incomplete.
We don't need this patch.

> If this is actually a problem then we have other
> places to fix this.
>
>   $ git grep "clk_init_data.*{ *}"
>   drivers/clk/bcm/clk-bcm53573-ilp.c:     struct clk_init_data init = { };
>   drivers/clk/clk-gpio.c: struct clk_init_data init = {};
>   drivers/clk/clk-qoriq.c:        struct clk_init_data init = {};
>   drivers/clk/clk-rk808.c:        struct clk_init_data init = {};
>   drivers/clk/mediatek/clk-apmixed.c:     struct clk_init_data init = {};
>   drivers/clk/mediatek/clk-gate.c:        struct clk_init_data init = {};
>   drivers/clk/mediatek/clk-pll.c: struct clk_init_data init = {};
>   drivers/clk/qcom/common.c:      struct clk_init_data init_data = { };
>   drivers/gpu/drm/msm/dsi/pll/dsi_pll_28nm_8960.c:        struct clk_init_data bytediv_init = { };
>

Regards
Vivek

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project