Date: Tue, 25 Feb 2020 13:03:05 +0000 From: "Bjoern A. Zeeb" <bz@FreeBSD.org> To: "Kristof Provost" <kp@FreeBSD.org> Cc: "Hans Petter Selasky" <hps@selasky.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r358167 - head/sys/netinet6 Message-ID: <A87D6FF7-C858-4581-82FA-66686502CBFE@FreeBSD.org> In-Reply-To: <963E6D27-9320-4F84-A3BB-A597C829AAAB@FreeBSD.org> References: <202002201056.01KAuC0N029186@repo.freebsd.org> <7d9ea227-7208-5773-080c-5ebc365c4be0@selasky.org> <963E6D27-9320-4F84-A3BB-A597C829AAAB@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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(&exthdrs, sizeof(exthdrs));
>>> optlen = 0;
>>> - unfragpartlen = 0;
>>> if (opt) {
>>> /* Hop-by-Hop options header. */
>>> MAKE_EXTHDR(opt->ip6po_hbh, &exthdrs.ip6e_hbh,
>>> optlen);
>>> @@ -497,8 +496,6 @@ ip6_output(struct mbuf *m0, struct ip6_pktopts
>>> *opt,
>>> /* Routing header. */
>>> MAKE_EXTHDR(opt->ip6po_rthdr, &exthdrs.ip6e_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, &exthdrs.ip6e_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(&exthdrs, sizeof(exthdrs));
optlen = 0;
- unfragpartlen = 0;
+ unfragpartlen = sizeof(struct ip6_hdr);
if (opt) {
/* Hop-by-Hop options header. */
MAKE_EXTHDR(opt->ip6po_hbh, &exthdrs.ip6e_hbh, optlen);
@@ -535,7 +535,7 @@ ip6_output(struct mbuf *m0, struct ip6_pktopts *op
/* Routing header. */
MAKE_EXTHDR(opt->ip6po_rthdr, &exthdrs.ip6e_rthdr,
optlen);
- unfragpartlen = optlen + sizeof(struct ip6_hdr);
+ unfragpartlen += optlen;
/*
* NOTE: we don't add AH/ESP length here (done in
/bz
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?A87D6FF7-C858-4581-82FA-66686502CBFE>