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 PHC | |
Open Source and information security mailing list archives
| ||
|
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
Powered by blists - more mailing lists