Re: svn commit: r358167 - head/sys/netinet6
On 25 Feb 2020, at 12:44, Kristof Provost wrote:
This change introduces a slight regression when a host replies to
IPv6 ping fragmented packets. The problem is the "unfragpartlen" must
also be set in the else case of "if (opt)", else the payload offset
computation for IPv6 fragments goes wrong by the size of the IPv6
header!
Confirmed, because the pf fragmentation:v6 test also fails on this.
Patch goes like this:
diff --git a/sys/netinet6/ip6_output.c b/sys/netinet6/ip6_output.c
index 06c57bcec48..a6c8d148833 100644
--- a/sys/netinet6/ip6_output.c
+++ b/sys/netinet6/ip6_output.c
@@ -459,7 +459,6 @@ ip6_output(struct mbuf *m0, struct ip6_pktopts
*opt,
*/
bzero(, sizeof(exthdrs));
optlen = 0;
- unfragpartlen = 0;
if (opt) {
/* Hop-by-Hop options header. */
MAKE_EXTHDR(opt->ip6po_hbh, _hbh,
optlen);
@@ -497,8 +496,6 @@ ip6_output(struct mbuf *m0, struct ip6_pktopts
*opt,
/* Routing header. */
MAKE_EXTHDR(opt->ip6po_rthdr, _rthdr,
optlen);
- unfragpartlen = optlen + sizeof(struct ip6_hdr);
-
/*
* NOTE: we don't add AH/ESP length here (done in
* ip6_ipsec_output()).
@@ -508,6 +505,8 @@ ip6_output(struct mbuf *m0, struct ip6_pktopts
*opt,
MAKE_EXTHDR(opt->ip6po_dest2, _dest2,
optlen);
}
+ unfragpartlen = optlen + sizeof(struct ip6_hdr);
+
/*
* If there is at least one extension header,
* separate IP6 header from the payload.
And with this patch the test passes again.
And fails for other packets. The patch is wrong.
Offset gets changed after we set unfragpartlen inside the block so
moving it outside the block unfragpartlen may point to the wrong thing.
Your tests are too simple to detect this problem.
Try this one please:
Index: ip6_output.c
===
--- ip6_output.c(revision 358297)
+++ ip6_output.c(working copy)
@@ -497,7 +497,7 @@ ip6_output(struct mbuf *m0, struct ip6_pktopts *op
*/
bzero(, sizeof(exthdrs));
optlen = 0;
- unfragpartlen = 0;
+ unfragpartlen = sizeof(struct ip6_hdr);
if (opt) {
/* Hop-by-Hop options header. */
MAKE_EXTHDR(opt->ip6po_hbh, _hbh, optlen);
@@ -535,7 +535,7 @@ ip6_output(struct mbuf *m0, struct ip6_pktopts *op
/* Routing header. */
MAKE_EXTHDR(opt->ip6po_rthdr, _rthdr,
optlen);
- unfragpartlen = optlen + sizeof(struct ip6_hdr);
+ unfragpartlen += optlen;
/*
* NOTE: we don't add AH/ESP length here (done in
/bz
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r358167 - head/sys/netinet6
On 24 Feb 2020, at 15:21, Hans Petter Selasky wrote:
On 2020-02-20 11:56, Bjoern A. Zeeb wrote:
+
+ unfragpartlen = optlen + sizeof(struct ip6_hdr);
+
Hi Bjoren,
This change introduces a slight regression when a host replies to IPv6
ping fragmented packets. The problem is the "unfragpartlen" must also
be set in the else case of "if (opt)", else the payload offset
computation for IPv6 fragments goes wrong by the size of the IPv6
header!
Confirmed, because the pf fragmentation:v6 test also fails on this.
After r358167:
ping6 -s 3000 fe80::ee0d:9aff:fed4:2c8c%mce2
PING6(3048=40+8+3000 bytes) fe80::ee0d:9aff:fed4:2c94%mce2 -->
fe80::ee0d:9aff:fed4:2c8c%mce2
^C
--- fe80::ee0d:9aff:fed4:2c8c%mce2 ping6 statistics ---
2 packets transmitted, 0 packets received, 100.0% packet loss
With the patch mentioned in the end of this e-mail:
ping6 -s 3000 fe80::ee0d:9aff:fed4:2c8c%mce2
PING6(3048=40+8+3000 bytes) fe80::ee0d:9aff:fed4:2c8c%mce2 -->
fe80::ee0d:9aff:fed4:2c8c%mce2
3008 bytes from fe80::ee0d:9aff:fed4:2c8c%mce2, icmp_seq=0 hlim=64
time=0.499 ms
3008 bytes from fe80::ee0d:9aff:fed4:2c8c%mce2, icmp_seq=1 hlim=64
time=0.405 ms
3008 bytes from fe80::ee0d:9aff:fed4:2c8c%mce2, icmp_seq=2 hlim=64
time=0.097 ms
^C
--- fe80::ee0d:9aff:fed4:2c8c%mce2 ping6 statistics ---
3 packets transmitted, 3 packets received, 0.0% packet loss
round-trip min/avg/max/std-dev = 0.097/0.334/0.499/0.172 ms
Patch goes like this:
diff --git a/sys/netinet6/ip6_output.c b/sys/netinet6/ip6_output.c
index 06c57bcec48..a6c8d148833 100644
--- a/sys/netinet6/ip6_output.c
+++ b/sys/netinet6/ip6_output.c
@@ -459,7 +459,6 @@ ip6_output(struct mbuf *m0, struct ip6_pktopts
*opt,
*/
bzero(, sizeof(exthdrs));
optlen = 0;
- unfragpartlen = 0;
if (opt) {
/* Hop-by-Hop options header. */
MAKE_EXTHDR(opt->ip6po_hbh, _hbh,
optlen);
@@ -497,8 +496,6 @@ ip6_output(struct mbuf *m0, struct ip6_pktopts
*opt,
/* Routing header. */
MAKE_EXTHDR(opt->ip6po_rthdr, _rthdr,
optlen);
- unfragpartlen = optlen + sizeof(struct ip6_hdr);
-
/*
* NOTE: we don't add AH/ESP length here (done in
* ip6_ipsec_output()).
@@ -508,6 +505,8 @@ ip6_output(struct mbuf *m0, struct ip6_pktopts
*opt,
MAKE_EXTHDR(opt->ip6po_dest2, _dest2,
optlen);
}
+ unfragpartlen = optlen + sizeof(struct ip6_hdr);
+
/*
* If there is at least one extension header,
* separate IP6 header from the payload.
And with this patch the test passes again.
Regards,
Kristof
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r358167 - head/sys/netinet6
On 2020-02-24 18:19, Bjoern A. Zeeb wrote: are you sure that this is the problem? Or does https://reviews.freebsd.org/D23760 solve the actual problem? Yes, I'm confident this is the problem. I made traces with tcpdump and looked at it using wireshark. Just setup two -current machines on a network and ping6 a large packet back2back w/o using lo0 . --HPS ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r358167 - head/sys/netinet6
On 24 Feb 2020, at 14:21, Hans Petter Selasky wrote: Hi Hans, On 2020-02-20 11:56, Bjoern A. Zeeb wrote: + + unfragpartlen = optlen + sizeof(struct ip6_hdr); + Hi Bjoren, This change introduces a slight regression when a host replies to IPv6 ping fragmented packets. The problem is the "unfragpartlen" must also be set in the else case of "if (opt)", else the payload offset computation for IPv6 fragments goes wrong by the size of the IPv6 header! After r358167: ping6 -s 3000 fe80::ee0d:9aff:fed4:2c8c%mce2 PING6(3048=40+8+3000 bytes) fe80::ee0d:9aff:fed4:2c94%mce2 --> fe80::ee0d:9aff:fed4:2c8c%mce2 ^C --- fe80::ee0d:9aff:fed4:2c8c%mce2 ping6 statistics --- 2 packets transmitted, 0 packets received, 100.0% packet loss With the patch mentioned in the end of this e-mail: are you sure that this is the problem? Or does https://reviews.freebsd.org/D23760 solve the actual problem? /bz ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r358167 - head/sys/netinet6
On 2020-02-20 11:56, Bjoern A. Zeeb wrote:
+
+ unfragpartlen = optlen + sizeof(struct ip6_hdr);
+
Hi Bjoren,
This change introduces a slight regression when a host replies to IPv6
ping fragmented packets. The problem is the "unfragpartlen" must also be
set in the else case of "if (opt)", else the payload offset computation
for IPv6 fragments goes wrong by the size of the IPv6 header!
After r358167:
ping6 -s 3000 fe80::ee0d:9aff:fed4:2c8c%mce2
PING6(3048=40+8+3000 bytes) fe80::ee0d:9aff:fed4:2c94%mce2 -->
fe80::ee0d:9aff:fed4:2c8c%mce2
^C
--- fe80::ee0d:9aff:fed4:2c8c%mce2 ping6 statistics ---
2 packets transmitted, 0 packets received, 100.0% packet loss
With the patch mentioned in the end of this e-mail:
ping6 -s 3000 fe80::ee0d:9aff:fed4:2c8c%mce2
PING6(3048=40+8+3000 bytes) fe80::ee0d:9aff:fed4:2c8c%mce2 -->
fe80::ee0d:9aff:fed4:2c8c%mce2
3008 bytes from fe80::ee0d:9aff:fed4:2c8c%mce2, icmp_seq=0 hlim=64 time=0.499 ms
3008 bytes from fe80::ee0d:9aff:fed4:2c8c%mce2, icmp_seq=1 hlim=64 time=0.405 ms
3008 bytes from fe80::ee0d:9aff:fed4:2c8c%mce2, icmp_seq=2 hlim=64 time=0.097 ms
^C
--- fe80::ee0d:9aff:fed4:2c8c%mce2 ping6 statistics ---
3 packets transmitted, 3 packets received, 0.0% packet loss
round-trip min/avg/max/std-dev = 0.097/0.334/0.499/0.172 ms
Patch goes like this:
diff --git a/sys/netinet6/ip6_output.c b/sys/netinet6/ip6_output.c
index 06c57bcec48..a6c8d148833 100644
--- a/sys/netinet6/ip6_output.c
+++ b/sys/netinet6/ip6_output.c
@@ -459,7 +459,6 @@ ip6_output(struct mbuf *m0, struct ip6_pktopts *opt,
*/
bzero(, sizeof(exthdrs));
optlen = 0;
- unfragpartlen = 0;
if (opt) {
/* Hop-by-Hop options header. */
MAKE_EXTHDR(opt->ip6po_hbh, _hbh, optlen);
@@ -497,8 +496,6 @@ ip6_output(struct mbuf *m0, struct ip6_pktopts *opt,
/* Routing header. */
MAKE_EXTHDR(opt->ip6po_rthdr, _rthdr, optlen);
- unfragpartlen = optlen + sizeof(struct ip6_hdr);
-
/*
* NOTE: we don't add AH/ESP length here (done in
* ip6_ipsec_output()).
@@ -508,6 +505,8 @@ ip6_output(struct mbuf *m0, struct ip6_pktopts *opt,
MAKE_EXTHDR(opt->ip6po_dest2, _dest2, optlen);
}
+ unfragpartlen = optlen + sizeof(struct ip6_hdr);
+
/*
* If there is at least one extension header,
* separate IP6 header from the payload.
--HPS
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r358167 - head/sys/netinet6
Author: bz
Date: Thu Feb 20 10:56:12 2020
New Revision: 358167
URL: https://svnweb.freebsd.org/changeset/base/358167
Log:
ip6_output: improve extension header handling
Move IPv6 source address checks from after extension header heandling
to the top of the function. If we do not pass these checks there is
no reason to do a lot of work upfront.
Fold extension header preparations and length calculations together into
a single branch and macro rather than doing them sequentially.
Likewise move extension header concatination into a single branch block
only doing it if we recorded any extension header length length.
Reviewed by: melifaro (earlier version), markj, gallatin
Sponsored by: Netflix (partially, originally)
MFC after:1 week
Differential Revision:https://reviews.freebsd.org/D23740
Modified:
head/sys/netinet6/ip6_output.c
Modified: head/sys/netinet6/ip6_output.c
==
--- head/sys/netinet6/ip6_output.c Thu Feb 20 09:33:14 2020
(r358166)
+++ head/sys/netinet6/ip6_output.c Thu Feb 20 10:56:12 2020
(r358167)
@@ -156,10 +156,10 @@ static int copypktopts(struct ip6_pktopts *, struct ip
/*
- * Make an extension header from option data. hp is the source, and
- * mp is the destination.
+ * Make an extension header from option data. hp is the source,
+ * mp is the destination, and _ol is the optlen.
*/
-#define MAKE_EXTHDR(hp, mp)\
+#defineMAKE_EXTHDR(hp, mp, _ol)
\
do { \
if (hp) { \
struct ip6_ext *eh = (struct ip6_ext *)(hp);\
@@ -167,6 +167,7 @@ static int copypktopts(struct ip6_pktopts *, struct ip
((eh)->ip6e_len + 1) << 3); \
if (error) \
goto freehdrs; \
+ (_ol) += (*(mp))->m_len;\
} \
} while (/*CONSTCOND*/ 0)
@@ -384,22 +385,23 @@ ip6_output(struct mbuf *m0, struct ip6_pktopts *opt,
struct ip6_hdr *ip6;
struct ifnet *ifp, *origifp;
struct mbuf *m = m0;
- struct mbuf *mprev = NULL;
+ struct mbuf *mprev;
int hlen, tlen, len;
struct route_in6 ip6route;
struct rtentry *rt = NULL;
struct sockaddr_in6 *dst, src_sa, dst_sa;
struct in6_addr odst;
+ u_char *nexthdrp;
int error = 0;
struct in6_ifaddr *ia = NULL;
u_long mtu;
int alwaysfrag, dontfrag;
- u_int32_t optlen = 0, plen = 0, unfragpartlen = 0;
+ u_int32_t optlen, plen = 0, unfragpartlen;
struct ip6_exthdrs exthdrs;
struct in6_addr src0, dst0;
u_int32_t zone;
struct route_in6 *ro_pmtu = NULL;
- int hdrsplit = 0;
+ bool hdrsplit;
int sw_csum, tso;
int needfiblookup;
uint32_t fibnum;
@@ -436,13 +438,50 @@ ip6_output(struct mbuf *m0, struct ip6_pktopts *opt,
}
#endif /* IPSEC */
+ /* Source address validation. */
+ ip6 = mtod(m, struct ip6_hdr *);
+ if (IN6_IS_ADDR_UNSPECIFIED(>ip6_src) &&
+ (flags & IPV6_UNSPECSRC) == 0) {
+ error = EOPNOTSUPP;
+ IP6STAT_INC(ip6s_badscope);
+ goto bad;
+ }
+ if (IN6_IS_ADDR_MULTICAST(>ip6_src)) {
+ error = EOPNOTSUPP;
+ IP6STAT_INC(ip6s_badscope);
+ goto bad;
+ }
+
+ /*
+* If we are given packet options to add extension headers prepare them.
+* Calculate the total length of the extension header chain.
+* Keep the length of the unfragmentable part for fragmentation.
+*/
bzero(, sizeof(exthdrs));
+ optlen = 0;
+ unfragpartlen = 0;
if (opt) {
/* Hop-by-Hop options header. */
- MAKE_EXTHDR(opt->ip6po_hbh, _hbh);
+ MAKE_EXTHDR(opt->ip6po_hbh, _hbh, optlen);
+
/* Destination options header (1st part). */
if (opt->ip6po_rthdr) {
+#ifndef RTHDR_SUPPORT_IMPLEMENTED
/*
+* If there is a routing header, discard the packet
+* right away here. RH0/1 are obsolete and we do not
+* currently support RH2/3/4.
+* People trying to use RH253/254 may want to disable
+* this check.
+* The moment we do support any routing header (again)
+* this block should check the routing type more
+*
