Message-ID: <90a213180cdc9792dfaf5271ea4e61fc07573f0d.camel@redhat.com>
Date:   Tue, 17 Jan 2023 12:14:44 +0100
From:   Paolo Abeni <pabeni@...hat.com>
To:     Gergely Riskó <gergely.risko@...il.com>,
        netdev@...r.kernel.org
Cc:     yoshfuji@...ux-ipv6.org, edumazet@...gle.com, davem@...emloft.net,
        dsahern@...nel.org, Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next] ipv6: fix reachability confirmation with
 proxy_ndp

Hello,

On Tue, 2023-01-17 at 11:29 +0100, Gergely Riskó wrote:
> (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)?

Thank you for the detailed explanation.

I think it's better if you repost this (for net), so we keep the
process clean. You can (and should) retain the reviewed-by tag by David
A., including it into the SoB area.

Cheers,

Paolo

Powered by blists - more mailing lists