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]

Message-ID: <10d51c0e-0bf1-ca9d-2d42-99a3d642bf2c@web.de>
Date:   Wed, 10 Apr 2019 11:46:56 +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?


>>>      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?


>>> +
>>> +    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?

Regards,
Markus