Re: [PATCH v2] net: ip6: ndisc: fix incorrect forwarding of proxied ns packets

[Nils Fuhler <nils@nilsfuhler.de>]

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