linux-kernel - Re: [v2] ethernet: ti: eliminate a bit of duplicate code in gbe

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Wed, 10 Apr 2019 13:35:49 +0200
From:   Markus Elfring <Markus.Elfring@....de>
To:     Wen Yang <wen.yang99@....com.cn>, netdev@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org, Yi Wang <wang.yi59@....com.cn>,
        Wingman Kwok <w-kwok2@...com>,
        Murali Karicheri <m-karicheri2@...com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [v2] ethernet: ti: eliminate a bit of duplicate code in
 gbe_probe()

>>>>> @@ -3651,22 +3651,18 @@ static int gbe_probe(struct netcp_device *netcp_device, struct device *dev,
>>>>>      if (ret)
>>>>>          return ret;
>>>>>
>>>>> -    interfaces = of_get_child_by_name(node, "interfaces");
>>>>> -    if (!interfaces)
>>>>> -        dev_err(dev, "could not find interfaces\n");
>>>>> -
>>>>
>>>> How do you think about to skip a bit of statements as a reaction for
>>>> such a null pointer?
>>>> https://elixir.bootlin.com/linux/v5.1-rc4/source/drivers/net/ethernet/ti/netcp_ethss.c#L3562
>> …
>>> Maybe you need to look at the implementation of for_each_child_of_node() and of_node_put().
>>> NULL check before those functions is not needed.
>>
>> This information is reasonable in principle.
>>
>> Was the reference counter incremented even if a null pointer was returned
>> by such a function call?
>
> The situation you assume is an issue that the of_get_child_by_name() function needs to consider
>  and has been irrelevant to our patch.

I suggest to reconsider the software situation a bit more.


> 1, when returning NULL, the of_get_child_by_name () function needs to ensure that the resources
>  it has allocated are released;
> 2, when returning NULL, if of_get_child_by_name() can&apos;t release its resources, then the
>  outer function has no way to release these resources.
>
> If you are interested, you can check the of_get_child_by_name() function further

It seems that the corresponding software documentation can be improved also here.
https://elixir.bootlin.com/linux/v5.1-rc4/source/drivers/of/base.c#L863


> and send it to me if you find any problems.

I find the exception handling suspicious in the discussed function implementation.


>>>>>      ret = netcp_txpipe_init(&gbe_dev->tx_pipe, netcp_device,
>>>>>                  gbe_dev->dma_chan_name, gbe_dev->tx_queue_id);
>>>>> -    if (ret) {
>>>>> -        of_node_put(interfaces);
>>>>> +    if (ret)
>>>>>          return ret;
>>>>> -    }
>>>>>
>>>>>      ret = netcp_txpipe_open(&gbe_dev->tx_pipe);
>>>>> -    if (ret) {
>>>>> -        of_node_put(interfaces);
>>>>> +    if (ret)
>>>>>          return ret;
>>>>> -    }
>>
>> Does the preparation of the NetCP pipe still matter in this special use case?
…
> Please refer to my reply above.

I propose to take additional software design possibilities into account.


> We have checked the netcp_txpipe_init() and the netcp_txpipe_open() function.

I wonder if such function calls are still relevant if a questionable system
configuration would be detected before.


> However, your questions may not actually be related to our patch.

Your update suggestion triggered related adjustment ideas.



>>>> +
>>>> +    interfaces = of_get_child_by_name(node, "interfaces");
>>>> +    if (!interfaces)
>>>> +        dev_err(dev, "could not find interfaces\n");
>>>>
>>>>      /* Create network interfaces */
>>>>      INIT_LIST_HEAD(&gbe_dev->gbe_intf_head);
>>>>
>>>> Can code like the following trigger corresponding software development concerns?
>>>>
>>>> for_each_child_of_node(interfaces, interface) {
>>>> …
>>>> }
>>>> of_node_put(interfaces);
>>>>
>> …
>>>> if (!gbe_dev->num_slaves)
>>>> dev_warn(dev, "No network interface configured\n");
>
>> Is this message really required as another response then?

Is the exception handling still questionable in this function?

Regards,
Markus