Message-ID: <CAMhZOOx3fnS6VtU5iKcyN7jAULG5ghRmTJYu8FaOYfkjpo2-XQ@mail.gmail.com>
Date:   Tue, 17 Jan 2023 11:29:23 +0100
From:   Gergely Riskó <gergely.risko@...il.com>
To:     netdev@...r.kernel.org
Cc:     yoshfuji@...ux-ipv6.org, edumazet@...gle.com, davem@...emloft.net,
        dsahern@...nel.org, pabeni@...hat.com,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next] ipv6: fix reachability confirmation with proxy_ndp

(resent to the mailing list too without stupid default Gmail HTML, sorry)

On Tue, Jan 17, 2023 at 5:53 AM Jakub Kicinski <kuba@...nel.org> wrote:
> Is this a regression? Did it use to work at any point in time?

Yes, this is a regression.

Here is the proof from 2020: https://github.com/lxc/lxd/issues/6668

Here people discuss that the multicast part works after setting
/proc/sys/net/ipv6/conf/eth0/proxy_ndp, but the unicast part only
works if they set conf/all/proxy_ndp.

So there was a point in time when the unicast worked. :)

The inconsistency regarding all/eth0 with multicast/unicast, I plan to
address in a separate patch, but I still need to do my homework
regarding that (more code reading), to make sure that I'm
understanding the context and the original intention enough.

The regression was introduced by Kangmin Park, as kindly pointed out
by David Ahern.  In my opinion they ran into this regression, because
they were just looking for places, where finalization statements were
not properly replicated in front of return statements.  At least there
is no proper explanation in the regression introducing code.

That's why this time I added a big comment in the hopes of protecting
ourselves in the future.

> We need to decide whether it needs to be sent for 6.1 and stable.
> If not we should soften the language (specifically remove the "fix"
> from the subject) otherwise the always-eager stable team will pull
> it into stable anyway :|

I tried to read up on all the requirements as much as possible, but
yes, I'm a first time committer, and it shows :(

I was thinking that this is non-urgent, as it's been like this since
2021, and people although complain on the internet, but not enough to
actually do something about it, and it looked like that the logic is
"next is free", "6.1 needs to have reasons", so I decided to chicken
out and go for next.  If in the current form the patch can be sent for
6.1 and stable, I would prefer that, as I currently have to compile my
own kernel with this patch in my production system and it is a
regression after all... :)

> Also - you haven't CCed the mailing list, you need to do so for
> the patch to be applied.

Yes, another noob mistake, the first point where as a new person I
found out about this, is when I sent to the mailing list ONLY, and in
the CI/CD I got the error message that I should CC the maintainers.
The proper list of email addresses to copy-paste to the git send-email
command is not published before you make a mistake.  At that point if
I send the mailing list again, then the patch duplicates in patchwork,
so I decided to just mail the maintainers separately, probably I
should have came up with a better way...

My next patch will be all better.  Do you prefer to reject this and
then I can resend the whole thing the proper way (fixing typos/grammar
too in the comments and commit message)?

Cheers,
Gergely

Powered by blists - more mailing lists