linux-kernel - Re: [PATCH] can: Use common error handling code in vxcan

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 28 Oct 2017 19:40:34 +0200
From:   Oliver Hartkopp <socketcan@...tkopp.net>
To:     SF Markus Elfring <elfring@...rs.sourceforge.net>,
        linux-can@...r.kernel.org, netdev@...r.kernel.org
Cc:     Marc Kleine-Budde <mkl@...gutronix.de>,
        Wolfgang Grandegger <wg@...ndegger.com>,
        LKML <linux-kernel@...r.kernel.org>,
        kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] can: Use common error handling code in vxcan_newlink()


On 10/28/2017 10:23 AM, SF Markus Elfring wrote:
>>> @@ -227,10 +227,8 @@ static int vxcan_newlink(struct net *net, struct net_device *dev,
>>>        netif_carrier_off(peer);
>>>          err = rtnl_configure_link(peer, ifmp);
>>> -    if (err < 0) {
>>> -        unregister_netdevice(peer);
>>> -        return err;
>>> -    }
>>> +    if (err)
>>> +        goto unregister_network_device;
>>
>> You are changing semantic in the if-statement here.
> 
> I got an other software development opinion for this implementation detail.
> 
> http://elixir.free-electrons.com/linux/v4.14-rc6/source/net/core/rtnetlink.c#L2393
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/net/core/rtnetlink.c?id=36ef71cae353f88fd6e095e2aaa3e5953af1685d#n2513
> 
> The success predicate for the function “rtnl_configure_link” is that
> the return value is zero. I would prefer to treat other values as
> an error code then.

Me not.

In rtnl_configure_link() the check is

	if (err < 0)
		return err;

And other calling sites as in linux/drivers/net/veth.c are checking for

	(err < 0)

too.

All checks done at the calling sites should be consistent.

So if you would like to change the if-statement:

1. Send a patch for vxcan.c to improve the error handling flow
2. Send a separate patch for all rtnl_configure_link() callers to unify 
the result check

Step 2 is optional ... and prepare yourself for more feedback ;-)

Regards,
Oliver