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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 11 Sep 2013 21:16:52 -0400
From:	Vlad Yasevich <vyasevic@...hat.com>
To:	Duan Jiong <duanj.fnst@...fujitsu.com>, davem@...emloft.net,
	netdev@...r.kernel.org, dborkman@...hat.com, vyasevich@...il.com
Subject: Re: [PATCH] ipv6: Do route updating for redirect in ndisc layer

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.

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@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html