netdev - Re: [PATCH net-next 10/10] ipv6: Refactor __ip6_route

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: <73cdd5f6-57ad-75a5-b9d7-75d61d784e48@gmail.com>
Date:   Wed, 10 Apr 2019 21:54:18 -0700
From:   David Ahern <dsahern@...il.com>
To:     Martin Lau <kafai@...com>
Cc:     David Ahern <dsahern@...nel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "idosch@...lanox.com" <idosch@...lanox.com>
Subject: Re: [PATCH net-next 10/10] ipv6: Refactor __ip6_route_redirect

On 4/10/19 1:45 PM, Martin Lau wrote:
> On Wed, Apr 10, 2019 at 11:45:04AM -0700, David Ahern wrote:
>>>> @@ -2420,7 +2449,7 @@ static struct rt6_info *__ip6_route_redirect(struct net *net,
>>>>  					     int flags)
>>>>  {
>>>>  	struct ip6rd_flowi *rdfl = (struct ip6rd_flowi *)fl6;
>>>> -	struct rt6_info *ret = NULL, *rt_cache;
>>>> +	struct rt6_info *ret = NULL;
>>>>  	struct fib6_info *rt;
>>> The "rt" naming is becoming hard to review.
>>> It is fortunate that the type context is kept in this diff.
>>>
>>> In this local case, the s/rt/f6i/ rename is quite doable?
>>
>> I looked into it and I feel such a mass rename just causes noise:
>> $ egrep -r 'fib6_info \*rt[,;]' net | wc -l
>>       50
>>
>> so that's 50 functions that need to be updated and then all rt
>> references within the functions. A lot of noise which is why I have not
>> sent a name change patch.
> Agree that it is a bit noisy (but obvious) change.
> I think it was a mistake to decide to postpone this name change while
> introducing fib6_info back then.  It would be easier to review if
> they were done together:
> https://marc.info/?l=linux-netdev&m=151968617902168&w=2
> 
> I think a few of us are still ok-ish because we know what
> have been going on.  For some, it will be confusing and
> it should not be suprised to see this 'struct fib6_info *rt'
> being copy-and-pasted to the future patch that makes it
> even harder to read/maintain.
> 

As I mentioned in that URL:
"My thought is to follow up with another patch that does the rename of
rt to f6i for all fib6_info. Given how large this change is already I
did not want to add extra diffs for that. If there is agreement to fold
that part in now, I can do it."

That was the theory. The reality is that it is a non-trivial change that
can not be done cleanly: 'rt' does not allow a clean search and replace
and when you consider the related changes (e.g., fib6_info *rt_last). I
am 80% through net/ipv6/route.c with ip6_fib.c to go. This is the
current diff stat:

$ git diff --stat
 include/net/if_inet6.h |   2 +-
 include/net/ip6_fib.h  |  20 +--
 net/ipv6/addrconf.c    |  84 ++++-----
 net/ipv6/route.c       | 674
++++++++++++++++++++++++++++++++++-----------------------------------
 4 files changed, 390 insertions(+), 390 deletions(-)

and that assumes I have done this right (ie., a clean compile). By the
time I am done that is going to be on the order of 500 insert / 500
delete of manual name changes. I see that as unnecessary churn with no
benefit.