* [PATCH] ipv6: ip6_append_data_mtu do not handle the mtu of the second fragment properly @ 2014-03-11 4:31 Xin Long 2014-03-11 14:49 ` Hannes Frederic Sowa 0 siblings, 1 reply; 7+ messages in thread From: Xin Long @ 2014-03-11 4:31 UTC (permalink / raw) To: network dev, Hannes Frederic Sowa, Gao feng; +Cc: Xin Long In ip6_append_data_mtu(), when the xfrm mode is not tunnel(such as transport),the ipsec header need to be added in the first fragment, so the mtu will decrease to reserve space for it, then the second fragment come, the mtu should be turn back, as the commit 0c1833797a5a6ec23ea9261d979aa18078720b74 said. however, in the commit a493e60ac4bbe2e977e7129d6d8cbb0dd236be, it use *mtu = min(*mtu, ...) to change the mtu, which lead to the new mtu is alway equal with the first fragment's. and cannot turn back. when I test through ping6 -c1 -s5000 $ip: ...frag (0|1232) ESP(spi=0x00002000,seq=0xb), length 1232 ...frag (1232|1216) ...frag (2448|1216) ...frag (3664|1216) ...frag (4880|164) which should be: ...frag (0|1232) ESP(spi=0x00001000,seq=0x1), length 1232 ...frag (1232|1232) ...frag (2464|1232) ...frag (3696|1232) ...frag (4928|116) so delete the min() when change back the mtu. Signed-off-by: Xin Long <lucien.xin@gmail.com> --- net/ipv6/ip6_output.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c index 2bc1070..dd05067 100644 --- a/net/ipv6/ip6_output.c +++ b/net/ipv6/ip6_output.c @@ -1113,9 +1113,8 @@ static void ip6_append_data_mtu(unsigned int *mtu, * this fragment is not first, the headers * space is regarded as data space. */ - *mtu = min(*mtu, pmtuprobe ? - rt->dst.dev->mtu : - dst_mtu(rt->dst.path)); + *mtu = pmtuprobe ? rt->dst.dev->mtu : + dst_mtu(rt->dst.path); } *maxfraglen = ((*mtu - fragheaderlen) & ~7) + fragheaderlen - sizeof(struct frag_hdr); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] ipv6: ip6_append_data_mtu do not handle the mtu of the second fragment properly 2014-03-11 4:31 [PATCH] ipv6: ip6_append_data_mtu do not handle the mtu of the second fragment properly Xin Long @ 2014-03-11 14:49 ` Hannes Frederic Sowa 2014-03-12 2:40 ` lucien xin 0 siblings, 1 reply; 7+ messages in thread From: Hannes Frederic Sowa @ 2014-03-11 14:49 UTC (permalink / raw) To: Xin Long; +Cc: network dev, Gao feng On Tue, Mar 11, 2014 at 12:31:49PM +0800, Xin Long wrote: > - *mtu = min(*mtu, pmtuprobe ? > - rt->dst.dev->mtu : > - dst_mtu(rt->dst.path)); > + *mtu = pmtuprobe ? rt->dst.dev->mtu : > + dst_mtu(rt->dst.path); Sorry, that is not correct: The min() protects the mtu going over np->frag_size (if set). In case we remove the min we would fallback to dev->mtu or dst_mtu and thus this could lead to a situation where the first fragment respects frag_size but second not. This confuses ip6_append_data and would lead to a crash. I am thinking about changing this to min(*mtu + rt->dst.header_len, pmtuprobe ? rt->dst.dev->mtu : dst_mtu(rt->dst.path)) or we pass the np directly and test for frag_size again. Good catch which should be fixed. Thanks! ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ipv6: ip6_append_data_mtu do not handle the mtu of the second fragment properly 2014-03-11 14:49 ` Hannes Frederic Sowa @ 2014-03-12 2:40 ` lucien xin 2014-03-12 10:26 ` Hannes Frederic Sowa 0 siblings, 1 reply; 7+ messages in thread From: lucien xin @ 2014-03-12 2:40 UTC (permalink / raw) To: Xin Long, network dev, Gao feng On Tue, Mar 11, 2014 at 10:49 PM, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > > Sorry, that is not correct: > > The min() protects the mtu going over np->frag_size (if set). In case we > remove the min we would fallback to dev->mtu or dst_mtu and thus this could > lead to a situation where the first fragment respects frag_size but second > not. This confuses ip6_append_data and would lead to a crash. > yes, your analysis is quite right, I ignore the code: if (np->frag_size < mtu) { if (np->frag_size) mtu = np->frag_size; } > I am thinking about changing this to > > min(*mtu + rt->dst.header_len, pmtuprobe ? rt->dst.dev->mtu : dst_mtu(rt->dst.path)) > > or we pass the np directly and test for frag_size again. but I cannot understand the top half of ip6_append_data() has the code to get mtu, if (rt->dst.flags & DST_XFRM_TUNNEL) mtu = np->pmtudisc >= IPV6_PMTUDISC_PROBE ? rt->dst.dev->mtu : dst_mtu(&rt->dst); else mtu = np->pmtudisc >= IPV6_PMTUDISC_PROBE ? rt->dst.dev->mtu : dst_mtu(rt->dst.path); why it need to calculate mtu again? just "mtu=*mtu + rt->dst.header_len", isn't it sufficient? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ipv6: ip6_append_data_mtu do not handle the mtu of the second fragment properly 2014-03-12 2:40 ` lucien xin @ 2014-03-12 10:26 ` Hannes Frederic Sowa 2014-03-13 5:38 ` lucien xin 0 siblings, 1 reply; 7+ messages in thread From: Hannes Frederic Sowa @ 2014-03-12 10:26 UTC (permalink / raw) To: lucien xin; +Cc: network dev, Gao feng On Wed, Mar 12, 2014 at 10:40:50AM +0800, lucien xin wrote: > but I cannot understand the top half of ip6_append_data() has the code > to get mtu, > if (rt->dst.flags & DST_XFRM_TUNNEL) > mtu = np->pmtudisc >= IPV6_PMTUDISC_PROBE ? > rt->dst.dev->mtu : dst_mtu(&rt->dst); > else > mtu = np->pmtudisc >= IPV6_PMTUDISC_PROBE ? > rt->dst.dev->mtu : dst_mtu(rt->dst.path); > > why it need to calculate mtu again? just "mtu=*mtu + > rt->dst.header_len", isn't it sufficient? It would be possible if we are absolutely sure if we don't call ip6_append_data_mtu a second time, which I have not yet reviewed. The line I proposed above may also suffer from this problem. Maybe you already checked that? Greetings, Hannes ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ipv6: ip6_append_data_mtu do not handle the mtu of the second fragment properly 2014-03-12 10:26 ` Hannes Frederic Sowa @ 2014-03-13 5:38 ` lucien xin 2014-03-15 15:55 ` Hannes Frederic Sowa 0 siblings, 1 reply; 7+ messages in thread From: lucien xin @ 2014-03-13 5:38 UTC (permalink / raw) To: lucien xin, network dev, Gao feng On Wed, Mar 12, 2014 at 6:26 PM, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > On Wed, Mar 12, 2014 at 10:40:50AM +0800, lucien xin wrote: > > It would be possible if we are absolutely sure if we don't call > ip6_append_data_mtu a second time, which I have not yet reviewed. > > The line I proposed above may also suffer from this problem. > > Maybe you already checked that? > hmm... this problem do exist. when it enter "the while( length>0 ){ }" with skb != NULL first, the problem will happen, of course, perhaps there are also other cases that trigger that problem. because that code seems a little mess， I hope the following change can make it more clear and eliminate potential insecurity, pls help to check it diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c index 2bc1070..07ac8f9 100644 --- a/net/ipv6/ip6_output.c +++ b/net/ipv6/ip6_output.c @@ -1101,21 +1101,19 @@ static void ip6_append_data_mtu(unsigned int *mtu, unsigned int fragheaderlen, struct sk_buff *skb, struct rt6_info *rt, - bool pmtuprobe) + unsigned int orig_mtu) { if (!(rt->dst.flags & DST_XFRM_TUNNEL)) { if (skb == NULL) { /* first fragment, reserve header_len */ - *mtu = *mtu - rt->dst.header_len; + *mtu = orig_mtu - rt->dst.header_len; } else { /* * this fragment is not first, the headers * space is regarded as data space. */ - *mtu = min(*mtu, pmtuprobe ? - rt->dst.dev->mtu : - dst_mtu(rt->dst.path)); + *mtu = orig_mtu; } *maxfraglen = ((*mtu - fragheaderlen) & ~7) + fragheaderlen - sizeof(struct frag_hdr); @@ -1132,7 +1130,7 @@ int ip6_append_data(struct sock *sk, int getfrag(void *from, char *to, struct ipv6_pinfo *np = inet6_sk(sk); struct inet_cork *cork; struct sk_buff *skb, *skb_prev = NULL; - unsigned int maxfraglen, fragheaderlen, mtu; + unsigned int maxfraglen, fragheaderlen, mtu, orig_mtu; int exthdrlen; int dst_exthdrlen; int hh_len; @@ -1214,6 +1212,7 @@ int ip6_append_data(struct sock *sk, int getfrag(void *from, char *to, dst_exthdrlen = 0; mtu = cork->fragsize; } + orig_mtu = mtu; hh_len = LL_RESERVED_SPACE(rt->dst.dev); @@ -1313,8 +1312,7 @@ alloc_new_skb: if (skb == NULL || skb_prev == NULL) ip6_append_data_mtu(&mtu, &maxfraglen, fragheaderlen, skb, rt, - np->pmtudisc >= - IPV6_PMTUDISC_PROBE); + orig_mtu); skb_prev = skb; ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] ipv6: ip6_append_data_mtu do not handle the mtu of the second fragment properly 2014-03-13 5:38 ` lucien xin @ 2014-03-15 15:55 ` Hannes Frederic Sowa 2014-03-16 4:50 ` lucien xin 0 siblings, 1 reply; 7+ messages in thread From: Hannes Frederic Sowa @ 2014-03-15 15:55 UTC (permalink / raw) To: lucien xin; +Cc: network dev, Gao feng On Thu, Mar 13, 2014 at 01:38:15PM +0800, lucien xin wrote: > On Wed, Mar 12, 2014 at 6:26 PM, Hannes Frederic Sowa > <hannes@stressinduktion.org> wrote: > > On Wed, Mar 12, 2014 at 10:40:50AM +0800, lucien xin wrote: > > > > It would be possible if we are absolutely sure if we don't call > > ip6_append_data_mtu a second time, which I have not yet reviewed. > > > > The line I proposed above may also suffer from this problem. > > > > Maybe you already checked that? > > > hmm... this problem do exist. when it enter "the while( length>0 ){ > }" with skb != NULL first, the problem > will happen, of course, perhaps there are also other cases that > trigger that problem. because that code seems > a little mess， I hope the following change can make it more clear and > eliminate potential insecurity, > pls help to check it The diff is good, thanks! > diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c > index 2bc1070..07ac8f9 100644 > --- a/net/ipv6/ip6_output.c > +++ b/net/ipv6/ip6_output.c > @@ -1101,21 +1101,19 @@ static void ip6_append_data_mtu(unsigned int > *mtu, > unsigned int fragheaderlen, > struct sk_buff *skb, > struct rt6_info *rt, > - bool pmtuprobe) > + unsigned int orig_mtu) > { > if (!(rt->dst.flags & DST_XFRM_TUNNEL)) { > if (skb == NULL) { > /* first fragment, reserve header_len */ > - *mtu = *mtu - rt->dst.header_len; > + *mtu = orig_mtu - rt->dst.header_len; > > } else { > /* > * this fragment is not first, the headers > * space is regarded as data space. > */ > - *mtu = min(*mtu, pmtuprobe ? > - rt->dst.dev->mtu : > - dst_mtu(rt->dst.path)); > + *mtu = orig_mtu; > } > *maxfraglen = ((*mtu - fragheaderlen) & ~7) > + fragheaderlen - sizeof(struct frag_hdr); > @@ -1132,7 +1130,7 @@ int ip6_append_data(struct sock *sk, int > getfrag(void *from, char *to, > struct ipv6_pinfo *np = inet6_sk(sk); > struct inet_cork *cork; > struct sk_buff *skb, *skb_prev = NULL; > - unsigned int maxfraglen, fragheaderlen, mtu; > + unsigned int maxfraglen, fragheaderlen, mtu, orig_mtu; > int exthdrlen; > int dst_exthdrlen; > int hh_len; > @@ -1214,6 +1212,7 @@ int ip6_append_data(struct sock *sk, int > getfrag(void *from, char *to, > dst_exthdrlen = 0; > mtu = cork->fragsize; > } > + orig_mtu = mtu; > > hh_len = LL_RESERVED_SPACE(rt->dst.dev); > > @@ -1313,8 +1312,7 @@ alloc_new_skb: > if (skb == NULL || skb_prev == NULL) > ip6_append_data_mtu(&mtu, &maxfraglen, > fragheaderlen, > skb, rt, > - np->pmtudisc >= > - IPV6_PMTUDISC_PROBE); > + orig_mtu); > > skb_prev = skb; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ipv6: ip6_append_data_mtu do not handle the mtu of the second fragment properly 2014-03-15 15:55 ` Hannes Frederic Sowa @ 2014-03-16 4:50 ` lucien xin 0 siblings, 0 replies; 7+ messages in thread From: lucien xin @ 2014-03-16 4:50 UTC (permalink / raw) To: lucien xin, network dev, Gao feng On Sat, Mar 15, 2014 at 11:55 PM, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > On Thu, Mar 13, 2014 at 01:38:15PM +0800, lucien xin wrote: >> On Wed, Mar 12, 2014 at 6:26 PM, Hannes Frederic Sowa >> <hannes@stressinduktion.org> wrote: >> > On Wed, Mar 12, 2014 at 10:40:50AM +0800, lucien xin wrote: >> > >> > It would be possible if we are absolutely sure if we don't call >> > ip6_append_data_mtu a second time, which I have not yet reviewed. >> > >> > The line I proposed above may also suffer from this problem. >> > >> > Maybe you already checked that? >> > >> hmm... this problem do exist. when it enter "the while( length>0 ){ >> }" with skb != NULL first, the problem >> will happen, of course, perhaps there are also other cases that >> trigger that problem. because that code seems >> a little mess， I hope the following change can make it more clear and >> eliminate potential insecurity, >> pls help to check it > > The diff is good, thanks! > okay , I'll repost it. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-03-16 4:50 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-03-11 4:31 [PATCH] ipv6: ip6_append_data_mtu do not handle the mtu of the second fragment properly Xin Long 2014-03-11 14:49 ` Hannes Frederic Sowa 2014-03-12 2:40 ` lucien xin 2014-03-12 10:26 ` Hannes Frederic Sowa 2014-03-13 5:38 ` lucien xin 2014-03-15 15:55 ` Hannes Frederic Sowa 2014-03-16 4:50 ` lucien xin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).