netdev - Re: [PATCH net-next v5] net: ip6mr: add RTM

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

Message-ID: <20220709122320.7ecc9621@kernel.org>
Date:   Sat, 9 Jul 2022 12:23:20 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     David Lamparter <equinox@...c24.net>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Nikolay Aleksandrov <razor@...ckwall.org>,
        David Ahern <dsahern@...nel.org>
Subject: Re: [PATCH net-next v5] net: ip6mr: add RTM_GETROUTE netlink op

On Sat, 9 Jul 2022 14:45:00 +0200 David Lamparter wrote:
> > > +	err = ip6mr_rtm_valid_getroute_req(in_skb, nlh, tb, extack);
> > > +	if (err < 0)
> > > +		goto errout;  
> > 
> > Can we:
> > 
> > 		return err;
> > 
> > ? I don't know where the preference for jumping to the return statement
> > came from, old compilers? someone's "gut feeling"?  
> 
> If I were forced to find a justification, I'd say having a central
> sequence of exit helps avoiding mistakes when some other resource
> acquisition is added later.  Easy to add a cleanup call to an existing
> cleanup block - easy to overlook a "return err;" that needs to be
> changed to "goto errout;".

That only works if the label's name is meaningless, if the label is
named after what it points to you have to rename the label and all the
jumps anyway. Can as well replace returns with a goto.

> But I have absolutely no stake in this at all, I'll happily edit it to
> whatever the consensus is.  This is just what the IPv4 code looks like
> after being adapted for IPv6.

Ah, I looked around other getroute implementations but not specifically
ipmr. I'd rather refactor ipmr.c as well than keep its strangeness.
The fact that we jump to the error path which tries to free the skb
without ever allocating the skb feels particularly off.