Re: [PATCH] ipv6: Do route updating for redirect in ndisc layer

[Daniel Borkmann <dborkman@redhat.com>]

On 09/12/2013 03:16 AM, Vlad Yasevich wrote:
> On 09/11/2013 07:17 PM, Hannes Frederic Sowa wrote:
>> [added Cc to Daniel and Vlad because of ipv6/sctp/redirect problem]
>>
>> On Wed, Sep 11, 2013 at 03:04:35PM +0800, Duan Jiong wrote:
>>> 于 2013年09月11日 06:50, Hannes Frederic Sowa 写道:
>>>> On Mon, Sep 09, 2013 at 03:09:56PM +0800, Duan Jiong wrote:
>>>>> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
>>>>> index 5c71501..61fe8e5 100644
>>>>> --- a/net/ipv6/tcp_ipv6.c
>>>>> +++ b/net/ipv6/tcp_ipv6.c
>>>>> @@ -382,14 +382,6 @@ static void tcp_v6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
>>>>>
>>>>>       np = inet6_sk(sk);
>>>>>
>>>>> -    if (type == NDISC_REDIRECT) {
>>>>> -        struct dst_entry *dst = __sk_dst_check(sk, np->dst_cookie);
>>>>> -
>>>>> -        if (dst)
>>>>> -            dst->ops->redirect(dst, sk, skb);
>>>>> -        goto out;
>>>>> -    }
>>>>> -
>>>>
>>>> You dropped the "goto out" here in case of an NDISC_REDIRECT, so this sends an
>>>> EPROTO further up the socket layer. Was this intended?
>>>>
>>>
>>> I'm sorry, i didn't notice the variable err was assigned to EPROTO.
>>> I only thought that message should be sent to the socket layer, because
>>> i found that in function sctp_v6_err().
>>>
>>> In addition, the rfc 4443 said the Redirect Message is not the ICMPv6 Error
>>> Message, so i think we shouldn't call those err_handler function, in other
>>> words we shouldn't call the icmpv6_notify().
>>>
>>> How do you think of this?
>>
>> Hm, thats hard.
>>
>> First of, when the kernel started publishing these errors it had a
>> contract with user-space we cannot break now. This includes all error
>> handling functions which call ipv6_icmp_error. So we only have to care
>> about INET6_PROTO_FINAL protocols, bbecause they mostly operate in socket
>> space (in this case these are the raw and the udp protocol and currently
>> sctp). Especially I do think it is important to report the redirects
>> to raw sockets. The other non-final protocols only need to be notified
>> for mtu reduction currently. Maybe we could stop notifying non-final
>> protocols for redirects, but I don't think this will improve things.
>>
>> Also we cannot know if the router sending the redirect discarded the
>> original packet or if it forwarded it just notifying us of a better route,
>> so we don't know if an actual error happend. So I would do the same thing
>> as IPv4 sockets, set sk_err to zero and queue up the icmp packet on the
>> socket's error queue (for udp and raw).
>>
>> Regarding notifying tcp sockets about the redirect seems wrong. It would
>> generate a poll notification and I do think it could even tear down
>> the whole connection.  I guess sctp should also stop updating sk_err
>> on redirects.  But let's Cc Daniel and Vlad about this. My guess is that
>> sctp could go into some error recovery mode because of this which would
>> be wrong.
>
> You are right.  SCTP shouldn't be setting sk_err on redirects as it
> isn't an error condition.  it should be doing exactly what tcp is doing and leaving the error handler without touching the socket.

Yep, probably something like ...

diff --git a/net/sctp/input.c b/net/sctp/input.c
index 5f20686..98b69bb 100644
--- a/net/sctp/input.c
+++ b/net/sctp/input.c
@@ -634,8 +634,7 @@ void sctp_v4_err(struct sk_buff *skb, __u32 info)
                 break;
         case ICMP_REDIRECT:
                 sctp_icmp_redirect(sk, transport, skb);
-               err = 0;
-               break;
+               /* Fall through to out_unlock. */
         default:
                 goto out_unlock;
         }
diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
index 4f52e2c..e7b2d4f 100644
--- a/net/sctp/ipv6.c
+++ b/net/sctp/ipv6.c
@@ -183,7 +183,7 @@ static void sctp_v6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
                 break;
         case NDISC_REDIRECT:
                 sctp_icmp_redirect(sk, transport, skb);
-               break;
+               goto out_unlock;
         default:
                 break;
         }

> Thanks
> -vlad
>
>>
>> So, for this patch I would leave the logic as is and not change anything
>> at the error reporting. Maybe Daniel and Vlad could check if we should
>> suppress redirect information for ipv6 in sctp, too? But this should
>> go into another patch.  Regarding the EPROTO problem in raw and udp,
>> let's see if all the problems go away if we update icmpv6_err_convert
>> to set *err to 0.
>>
>> Greetings,
>>
>>    Hannes
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>