netdev - Re: [RFC PATCH v1] tun: Cleanup error handling in tun_set

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]

Message-Id: <200908061420.20983.paul.moore@hp.com>
Date:	Thu, 6 Aug 2009 14:20:20 -0400
From:	Paul Moore <paul.moore@...com>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	netdev@...r.kernel.org, David Miller <davem@...emloft.net>,
	Herbert Xu <herbert@...dor.apana.org.au>
Subject: Re: [RFC PATCH v1] tun: Cleanup error handling in tun_set_iff()

On Wednesday 05 August 2009 07:14:06 pm Eric W. Biederman wrote:
> Paul Moore <paul.moore@...com> writes:
> > On Wednesday 05 August 2009 01:32:49 am Eric W. Biederman wrote:
> >> Paul Moore <paul.moore@...com> writes:
> >> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> >> > index 4a0db7a..b6d06fd 100644
> >> > --- a/drivers/net/tun.c
> >> > +++ b/drivers/net/tun.c
> >> > @@ -976,10 +973,11 @@ static int tun_set_iff(struct net *net, struct
> >> > file *file, struct ifreq *ifr) tun->flags = flags;
> >> >  		tun->txflt.count = 0;
> >> >
> >> > -		err = -ENOMEM;
> >> >  		sk = sk_alloc(net, AF_UNSPEC, GFP_KERNEL, &tun_proto);
> >> > -		if (!sk)
> >> > +		if (!sk) {
> >> > +			err = -ENOMEM;
> >> >  			goto err_free_dev;
> >> > +		}
> >>
> >> Trivially correct but I would argue uglier.
> >
> > My reasoning behind the change was that code related to the error
> > handling should be moved outside the common path as much as possible
> > similar to what we do now with the gotos.
>
> I don't understand.  Generating less readable and less efficient code is
> preferable?

While we can probably debate the "readability" of code all day long and get no 
where (anyone care to argue about the color of the bikeshed?) the concept of 
code efficiency should be a bit easier to quantify.  I'll admit that I'm far 
from a performance expert but here is my reasoning ...

The code currently looks something like this:

	err = -ENOMEM;
	buf = alloc(...);
	if (!buf)
		goto label;

This means that in the common case where 'alloc()' completes without error we 
are doing an extra, unnecessary assignment where we set the value in 'err'.  
Now, if we change this slightly to match what I proposed in the patch:

	buf = alloc(...);
	if (!buf) {
		err = -ENOMEM;
		goto label;
	}

We eliminate that extra assignment in the case where 'alloc()' completes 
without error, which should result in more efficient code (less instructions 
in the common case).  Am I wrong?  If that is the case I would appreciate an 
explanation ...

-- 
paul moore
linux @ hp

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html