Message-ID: <20130917135232.GG13408@order.stressinduktion.org>
Date:	Tue, 17 Sep 2013 15:52:32 +0200
From:	Hannes Frederic Sowa <hannes@...essinduktion.org>
To:	Duan Jiong <djduanjiong@...il.com>
Cc:	Daniel Borkmann <dborkman@...hat.com>,
	Duan Jiong <duanj.fnst@...fujitsu.com>, davem@...emloft.net,
	netdev@...r.kernel.org,
	"linux-sctp@...r.kernel.org" <linux-sctp@...r.kernel.org>
Subject: Re: [PATCH  v3 0/6] ipv6: Do route updating for redirect in ndisc layer

On Mon, Sep 16, 2013 at 10:08:12PM +0800, Duan Jiong wrote:
> 于 2013/9/16 20:22, Daniel Borkmann 写道:
> > On 09/16/2013 01:47 PM, Duan Jiong wrote:
> >> From: Duan Jiong <duanj.fnst@...fujitsu.com>
> >>
> >> the ip6_redirect() could be replaced with
> >> ip6_redirect_no_header(), we could always use ip6_redirect()
> >> for route updating in ndisc layer and use the data of the
> >> redirected header option just for finding the socket to be
> >> notified and then notify user in protocols' err_handler.
> > If I get this right, it seems to me that this patchset actually consists of two
> > different kind of changes:
> >
> > 1) Not notifying user space on ICMP redirects (net material)
> > 2) Simplify code for updating route in ndisc layer instead of error handlers (net-next)
> >
> > Also, you do the *actual* change in the very last patch, which means that from
> > patch 1 to 5 we're in an inconsistent and buggy state unless we also apply patch
> > number 6. It should actually be the other way around, that you first do the actual
> > change and then migrate users (also commit messages are quite terse).
> 
> I make the patch set on net tree, not on net-next. Maybe those
> things should be done in two patch sets.

I have thought about going the other direction. Just make it one patch
and update the whole logic atomically in the git tree. So we won't have
any partial logic updates in the tree.

Please also rebase your series onto net from today, because Daniel's
patch regarding updating sk->sk_err in sctp went in yesterday.

> > Moreover, just looking at the SCTP part (sctp_err_lookup() function) ...
> >
> > /* RFC 4960, Appendix C. ICMP Handling
> >  *
> >  * ICMP6) An implementation MUST validate that the Verification Tag
> >  * contained in the ICMP message matches the Verification Tag of
> >  * the peer.  If the Verification Tag is not 0 and does NOT
> >  * match, discard the ICMP message.  If it is 0 and the ICMP
> >  * message contains enough bytes to verify that the chunk type is
> >  * an INIT chunk and that the Initiate Tag matches the tag of the
> >  * peer, continue with ICMP7.  If the ICMP message is too short
> >  * or the chunk type or the Initiate Tag does not match, silently
> >  * discard the packet.
> >  */
> >  
> > ... it seems to me that we would simply ignore such RFC requirements with
> > your patch for the sctp_v6_err() part.
> >
> > Care to elaborate? ;-)
> 
> Sorry, i didn't notice that.
> 
> According to the RFC requirements, it suggests that we can't update
> route for redirect in ndisc layer before calling into sctp_err_lookup(),
> because we must verify the ICMP Message. Now maybe we update route for
> redirect in ndisc layer is wrong.
> 
> Do you have any idea?

IMO updating of routes in ndisc layer is fine. We already accept redirects
(and also change routes) for sctp connections where the redirect packet does
not contain any tag. Also it is debatable if redirects are counted as
icmp packets or merely just belong to the kind of neighbour discovery
packets which just use icmp framing (and so the sctp rfc would not say
anything about them).

As Daniel already said, it would be better to update the commit message
to clarify the reasons why this is ok (with some pointers to RFCs).

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

Powered by blists - more mailing lists