| Message ID | 20240815151809.16820-2-nils@nilsfuhler.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] net: ip6: ndisc: fix incorrect forwarding of proxied ns packets | expand |
On 8/15/24 17:18, Nils Fuhler wrote: > When enabling proxy_ndp per interface instead of globally, neighbor > solicitation packets sent to proxied global unicast addresses are > forwarded instead of generating a neighbor advertisement. When > proxy_ndp is enabled globally, these packets generate na responses as > expected. > > This patch fixes this behaviour. When an ns packet is sent to a > proxied unicast address, it generates an na response regardless > whether proxy_ndp is enabled per interface or globally. > > Signed-off-by: Nils Fuhler <nils@nilsfuhler.de> I have mixed feeling WRT this patch. It looks like a fix, but it's changing an established behaviour that is there since a lot of time. I think it could go via the net-next tree, without fixes tag to avoid stable backports. As such I guess it deserves a self-test script validating the new behavior. > --- > v1 -> v2: ensure that idev is not NULL > > net/ipv6/ip6_output.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c > index ab504d31f0cd..0356c8189e21 100644 > --- a/net/ipv6/ip6_output.c > +++ b/net/ipv6/ip6_output.c > @@ -551,8 +551,8 @@ int ip6_forward(struct sk_buff *skb) > return -ETIMEDOUT; > } > > - /* XXX: idev->cnf.proxy_ndp? */ > - if (READ_ONCE(net->ipv6.devconf_all->proxy_ndp) && > + if ((READ_ONCE(net->ipv6.devconf_all->proxy_ndp) || > + (idev && READ_ONCE(idev->cnf.proxy_ndp))) && > pneigh_lookup(&nd_tbl, net, &hdr->daddr, skb->dev, 0)) { > int proxied = ip6_forward_proxy_check(skb); > if (proxied > 0) { Note that there is similar chunk in ndisc_recv_na() that also ignores idev->cnf.proxy_ndp, why don't you need to such function, too? Thanks, Paolo
On 20/08/2024 15:13, Paolo Abeni wrote: > > > On 8/15/24 17:18, Nils Fuhler wrote: >> When enabling proxy_ndp per interface instead of globally, neighbor >> solicitation packets sent to proxied global unicast addresses are >> forwarded instead of generating a neighbor advertisement. When >> proxy_ndp is enabled globally, these packets generate na responses as >> expected. >> >> This patch fixes this behaviour. When an ns packet is sent to a >> proxied unicast address, it generates an na response regardless >> whether proxy_ndp is enabled per interface or globally. >> >> Signed-off-by: Nils Fuhler <nils@nilsfuhler.de> > > I have mixed feeling WRT this patch. It looks like a fix, but it's changing an established behaviour that is there since a lot of time. > > I think it could go via the net-next tree, without fixes > tag to avoid stable backports. As such I guess it deserves a self-test script validating the new behavior. > That is probably the best option. Although I'm not sure whether it would really break something. The forwarded packets have a hoplimit of 254 and are therefore not valid ndisc packets anymore. >> --- >> v1 -> v2: ensure that idev is not NULL >> >> net/ipv6/ip6_output.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c >> index ab504d31f0cd..0356c8189e21 100644 >> --- a/net/ipv6/ip6_output.c >> +++ b/net/ipv6/ip6_output.c >> @@ -551,8 +551,8 @@ int ip6_forward(struct sk_buff *skb) >> return -ETIMEDOUT; >> } >> - /* XXX: idev->cnf.proxy_ndp? */ >> - if (READ_ONCE(net->ipv6.devconf_all->proxy_ndp) && >> + if ((READ_ONCE(net->ipv6.devconf_all->proxy_ndp) || >> + (idev && READ_ONCE(idev->cnf.proxy_ndp))) && >> pneigh_lookup(&nd_tbl, net, &hdr->daddr, skb->dev, 0)) { >> int proxied = ip6_forward_proxy_check(skb); >> if (proxied > 0) { > > Note that there is similar chunk in ndisc_recv_na() that also ignores idev->cnf.proxy_ndp, why don't you need to such function, too? I have noticed the chunk in ndisc_recv_na() and did some quick testing but I was not able to get an obviously wrong behavior out of it. I have to admit, though, that I am not sure if I understand the condition correctly. At the start, it checks that the lladdr of the received na packet is equal to the lladdr of the reciving interface. That can only happen, when the interface receives its own packet, right? Is there a valid case where that can happen? Or am I missing something? Greetings, Nils
What is the status of this? The proposed fix seems to work. > > I have mixed feeling WRT this patch. It looks like a fix, but it's changing an established behaviour that is there since a lot of time. > > > > I think it could go via the net-next tree, without fixes > > tag to avoid stable backports. As such I guess it deserves a self-test script validating the new behavior. > > > That is probably the best option. > Although I'm not sure whether it would really break something. The > forwarded packets have a hoplimit of 254 and are therefore not valid > ndisc packets anymore. I also can't imagine that anything depends on invalid packets being created?
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c index ab504d31f0cd..0356c8189e21 100644 --- a/net/ipv6/ip6_output.c +++ b/net/ipv6/ip6_output.c @@ -551,8 +551,8 @@ int ip6_forward(struct sk_buff *skb) return -ETIMEDOUT; } - /* XXX: idev->cnf.proxy_ndp? */ - if (READ_ONCE(net->ipv6.devconf_all->proxy_ndp) && + if ((READ_ONCE(net->ipv6.devconf_all->proxy_ndp) || + (idev && READ_ONCE(idev->cnf.proxy_ndp))) && pneigh_lookup(&nd_tbl, net, &hdr->daddr, skb->dev, 0)) { int proxied = ip6_forward_proxy_check(skb); if (proxied > 0) {
When enabling proxy_ndp per interface instead of globally, neighbor solicitation packets sent to proxied global unicast addresses are forwarded instead of generating a neighbor advertisement. When proxy_ndp is enabled globally, these packets generate na responses as expected. This patch fixes this behaviour. When an ns packet is sent to a proxied unicast address, it generates an na response regardless whether proxy_ndp is enabled per interface or globally. Signed-off-by: Nils Fuhler <nils@nilsfuhler.de> --- v1 -> v2: ensure that idev is not NULL net/ipv6/ip6_output.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)