Re: [PATCH net-next] ipv6: fix reachability confirmation with proxy_ndp

Re: [PATCH net-next] ipv6: fix reachability confirmation with proxy_ndp

[Gergely Riskó]

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

On Tue, Jan 17, 2023 at 5:53 AM Jakub Kicinski <kuba@kernel.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

Re: [PATCH net-next] ipv6: fix reachability confirmation with proxy_ndp

[Paolo Abeni]

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@kernel.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

[PATCH net-next] ipv6: fix reachability confirmation with proxy_ndp

[Gergely Risko]

From: Gergely Risko <gergely.risko@gmail.com>

When proxying IPv6 NDP requests, the adverts to the initial multicast
solicits are correct and working.  On the other hand, when a
reachability confirmation is requested (on unicast), no reply is sent.

This causes the neighbor entry expiring on the sending host, which is
mostly a non-issue, as a new multicast request is sent.  There are
routers, where the multicast requests are intentionally delayed, and in
these environments the current implementation causes periodic packet
loss for the proxied endpoints.

The root causes is the erroneous decrease of the hop limit, as this
is checked in ndisc.c and no answer is generated when it's 254 instead
of the correct 255.

Signed-off-by: Gergely Risko <gergely.risko@gmail.com>
---
 net/ipv6/ip6_output.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 60fd91bb5171..c314fdde0097 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -547,7 +547,20 @@ int ip6_forward(struct sk_buff *skb)
 	    pneigh_lookup(&nd_tbl, net, &hdr->daddr, skb->dev, 0)) {
 		int proxied = ip6_forward_proxy_check(skb);
 		if (proxied > 0) {
-			hdr->hop_limit--;
+			/* It's tempting to decrease the hop limit
+			 * here by 1, as we do at the end of the
+			 * function too.
+			 *
+			 * But that would be incorrect, as proxying is
+			 * not forwarding.  The ip6_input function
+			 * will handle this packet locally, and it
+			 * depends on the hop limit being unchanged.
+			 *
+			 * One example is the NDP hop limit, that
+			 * always has to stay 255, but other would be
+			 * similar checks around RA packets, where the
+			 * user can even change the desired limit.
+			 */
 			return ip6_input(skb);
 		} else if (proxied < 0) {
 			__IP6_INC_STATS(net, idev, IPSTATS_MIB_INDISCARDS);
-- 
2.39.0