Re: [Bugme-new] [Bug 15682] New: XFRM is not updating RTAX_ADVMSS metric

Re: [Bugme-new] [Bug 15682] New: XFRM is not updating RTAX_ADVMSS metric

[Andrew Morton]

(switched to email.  Please respond via emailed reply-to-all, not via the
bugzilla web interface).

On Fri, 2 Apr 2010 17:34:35 GMT
bugzilla-daemon@bugzilla.kernel.org wrote:

> https://bugzilla.kernel.org/show_bug.cgi?id=15682
> 
>            Summary: XFRM is not updating RTAX_ADVMSS metric
>            Product: Networking
>            Version: 2.5
>     Kernel Version: 2.6.28-2
>           Platform: All
>         OS/Version: Linux
>               Tree: Mainline
>             Status: NEW
>           Severity: normal
>           Priority: P1
>          Component: Other
>         AssignedTo: acme@ghostprotocols.net
>         ReportedBy: eduardo.panisset@gmail.com
>         Regression: No
> 
> 
> I have been testing DSMIPv6 code which uses all kind of advanced
> features of XFRM framework and I believe I have found a bug related to
> update RTAX_ADVMSS route metric.
> The XFRM code on net/xfrm/xfrm_policy.c by its functions
> xfrm_init_pmtu and xfrm_bundle_ok updates RTAX_MTU route caching
> metric however I believe it must update RTAX_ADVMSS as this later is
> used by tcp connect function for adverting the MSS value on SYN
> messages.
> 
> As MSS is not being updated by XFRM the TCP SYN messages (e.g.
> originated from a internet browser)  is erroneously informing its MSS
> (without taking into account the overhead added to IP packet size by
> XFRM transformations).  One result of that is the browser gets
> "frozen" after starts a TCP connection because TCP messages sent by
> TCP server will never get to it (TCP server is sending too large
> segments to browser).
> 
> Below I describe the changes I have done (on xfrm_init_pmtu and
> xfrm_bundle_ok) and that seem to fix this problem:
> 
> xfrm_init_pmtu:
>                  .
>                  .
>                  .
> 
>         dst->metrics[RTAX_MTU-1] = pmtu; // original code, below my changes
> 
>         if (dst->xfrm->props.mode == XFRM_MODE_TUNNEL)
>                  switch (dst->xfrm->props.family)
>                  {
>                  case AF_INET:
>                  dst->metrics[RTAX_ADVMSS-1] = max_t(unsigned int,
> pmtu - sizeof(struct iphdr) - sizeof(struct tcphdr), 256);
>                  break;
> 
>                  case AF_INET6:
>                  dst->metrics[RTAX_ADVMSS-1] = max_t(unsigned int,
> pmtu - sizeof(struct ipv6hdr) - sizeof(struct tcphdr),
>                             dev_net(dst->dev)->ipv6.
> sysctl.ip6_rt_min_advmss);
>                  break;
>                  }
> 
> xfrm_bundle_ok:
> 
>                .
>                .
>                .
> 
>         dst->metrics[RTAX_MTU-1] = mtu; // original code, below my changes
> 
>         if (dst->xfrm->props.mode == XFRM_MODE_TUNNEL)
>                 switch (dst->xfrm->props.family)
>                 {
>                 case AF_INET:
>                         dst->metrics[RTAX_ADVMSS-1] = max_t(unsigned
> int, mtu - sizeof(struct iphdr) - sizeof(struct tcphdr), 256);
>                 break;
> 
>                 case AF_INET6:
>                         dst->metrics[RTAX_ADVMSS-1] = max_t(unsigned
> int, mtu - sizeof(struct ipv6hdr) - sizeof(struct tcphdr),
> 
> dev_net(dst->dev)->ipv6.sysctl.ip6_rt_min_advmss);
>                 break;
>                 }
> 

Re: [Bugme-new] [Bug 15682] New: XFRM is not updating RTAX_ADVMSS metric

[jamal]

Herbert would give better answers. I dont think what Eduardo is 
doing is correct. You cant just start factoring in tcp headers
at the xfrm level - and besides, the mtu calculation
already takes care tunnel headers - so tcp should be able to 
compute correct MSS. 

cheers,
jamal

On Mon, 2010-04-05 at 12:50 -0700, Andrew Morton wrote:
> (switched to email.  Please respond via emailed reply-to-all, not via the
> bugzilla web interface).
> 
> On Fri, 2 Apr 2010 17:34:35 GMT
> bugzilla-daemon@bugzilla.kernel.org wrote:
> 
> > https://bugzilla.kernel.org/show_bug.cgi?id=15682
> > 
> >            Summary: XFRM is not updating RTAX_ADVMSS metric
> >            Product: Networking
> >            Version: 2.5
> >     Kernel Version: 2.6.28-2
> >           Platform: All
> >         OS/Version: Linux
> >               Tree: Mainline
> >             Status: NEW
> >           Severity: normal
> >           Priority: P1
> >          Component: Other
> >         AssignedTo: acme@ghostprotocols.net
> >         ReportedBy: eduardo.panisset@gmail.com
> >         Regression: No
> > 
> > 
> > I have been testing DSMIPv6 code which uses all kind of advanced
> > features of XFRM framework and I believe I have found a bug related to
> > update RTAX_ADVMSS route metric.
> > The XFRM code on net/xfrm/xfrm_policy.c by its functions
> > xfrm_init_pmtu and xfrm_bundle_ok updates RTAX_MTU route caching
> > metric however I believe it must update RTAX_ADVMSS as this later is
> > used by tcp connect function for adverting the MSS value on SYN
> > messages.
> > 
> > As MSS is not being updated by XFRM the TCP SYN messages (e.g.
> > originated from a internet browser)  is erroneously informing its MSS
> > (without taking into account the overhead added to IP packet size by
> > XFRM transformations).  One result of that is the browser gets
> > "frozen" after starts a TCP connection because TCP messages sent by
> > TCP server will never get to it (TCP server is sending too large
> > segments to browser).
> > 
> > Below I describe the changes I have done (on xfrm_init_pmtu and
> > xfrm_bundle_ok) and that seem to fix this problem:
> > 
> > xfrm_init_pmtu:
> >                  .
> >                  .
> >                  .
> > 
> >         dst->metrics[RTAX_MTU-1] = pmtu; // original code, below my changes
> > 
> >         if (dst->xfrm->props.mode == XFRM_MODE_TUNNEL)
> >                  switch (dst->xfrm->props.family)
> >                  {
> >                  case AF_INET:
> >                  dst->metrics[RTAX_ADVMSS-1] = max_t(unsigned int,
> > pmtu - sizeof(struct iphdr) - sizeof(struct tcphdr), 256);
> >                  break;
> > 
> >                  case AF_INET6:
> >                  dst->metrics[RTAX_ADVMSS-1] = max_t(unsigned int,
> > pmtu - sizeof(struct ipv6hdr) - sizeof(struct tcphdr),
> >                             dev_net(dst->dev)->ipv6.
> > sysctl.ip6_rt_min_advmss);
> >                  break;
> >                  }
> > 
> > xfrm_bundle_ok:
> > 
> >                .
> >                .
> >                .
> > 
> >         dst->metrics[RTAX_MTU-1] = mtu; // original code, below my changes
> > 
> >         if (dst->xfrm->props.mode == XFRM_MODE_TUNNEL)
> >                 switch (dst->xfrm->props.family)
> >                 {
> >                 case AF_INET:
> >                         dst->metrics[RTAX_ADVMSS-1] = max_t(unsigned
> > int, mtu - sizeof(struct iphdr) - sizeof(struct tcphdr), 256);
> >                 break;
> > 
> >                 case AF_INET6:
> >                         dst->metrics[RTAX_ADVMSS-1] = max_t(unsigned
> > int, mtu - sizeof(struct ipv6hdr) - sizeof(struct tcphdr),
> > 
> > dev_net(dst->dev)->ipv6.sysctl.ip6_rt_min_advmss);
> >                 break;
> >                 }
> > 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Bugme-new] [Bug 15682] New: XFRM is not updating RTAX_ADVMSS metric

[Eduardo Panisset]

Hi,

My intention is only to report a problem that I have faced. The
solution proposed isn't (I know that) probably the best one to adopt
as I'm not a kernel specialist, it is more illustrative to allow you
guys understanding what I'm meaning and solve that on the better way
(hence I haven't submited a patch).

Regards,
Eduardo Panisset.

On Tue, Apr 6, 2010 at 10:40 AM, jamal <hadi@cyberus.ca> wrote:
>
> Herbert would give better answers. I dont think what Eduardo is
> doing is correct. You cant just start factoring in tcp headers
> at the xfrm level - and besides, the mtu calculation
> already takes care tunnel headers - so tcp should be able to
> compute correct MSS.
>
> cheers,
> jamal
>
> On Mon, 2010-04-05 at 12:50 -0700, Andrew Morton wrote:
>> (switched to email.  Please respond via emailed reply-to-all, not via the
>> bugzilla web interface).
>>
>> On Fri, 2 Apr 2010 17:34:35 GMT
>> bugzilla-daemon@bugzilla.kernel.org wrote:
>>
>> > https://bugzilla.kernel.org/show_bug.cgi?id=15682
>> >
>> >            Summary: XFRM is not updating RTAX_ADVMSS metric
>> >            Product: Networking
>> >            Version: 2.5
>> >     Kernel Version: 2.6.28-2
>> >           Platform: All
>> >         OS/Version: Linux
>> >               Tree: Mainline
>> >             Status: NEW
>> >           Severity: normal
>> >           Priority: P1
>> >          Component: Other
>> >         AssignedTo: acme@ghostprotocols.net
>> >         ReportedBy: eduardo.panisset@gmail.com
>> >         Regression: No
>> >
>> >
>> > I have been testing DSMIPv6 code which uses all kind of advanced
>> > features of XFRM framework and I believe I have found a bug related to
>> > update RTAX_ADVMSS route metric.
>> > The XFRM code on net/xfrm/xfrm_policy.c by its functions
>> > xfrm_init_pmtu and xfrm_bundle_ok updates RTAX_MTU route caching
>> > metric however I believe it must update RTAX_ADVMSS as this later is
>> > used by tcp connect function for adverting the MSS value on SYN
>> > messages.
>> >
>> > As MSS is not being updated by XFRM the TCP SYN messages (e.g.
>> > originated from a internet browser)  is erroneously informing its MSS
>> > (without taking into account the overhead added to IP packet size by
>> > XFRM transformations).  One result of that is the browser gets
>> > "frozen" after starts a TCP connection because TCP messages sent by
>> > TCP server will never get to it (TCP server is sending too large
>> > segments to browser).
>> >
>> > Below I describe the changes I have done (on xfrm_init_pmtu and
>> > xfrm_bundle_ok) and that seem to fix this problem:
>> >
>> > xfrm_init_pmtu:
>> >                  .
>> >                  .
>> >                  .
>> >
>> >         dst->metrics[RTAX_MTU-1] = pmtu; // original code, below my changes
>> >
>> >         if (dst->xfrm->props.mode == XFRM_MODE_TUNNEL)
>> >                  switch (dst->xfrm->props.family)
>> >                  {
>> >                  case AF_INET:
>> >                  dst->metrics[RTAX_ADVMSS-1] = max_t(unsigned int,
>> > pmtu - sizeof(struct iphdr) - sizeof(struct tcphdr), 256);
>> >                  break;
>> >
>> >                  case AF_INET6:
>> >                  dst->metrics[RTAX_ADVMSS-1] = max_t(unsigned int,
>> > pmtu - sizeof(struct ipv6hdr) - sizeof(struct tcphdr),
>> >                             dev_net(dst->dev)->ipv6.
>> > sysctl.ip6_rt_min_advmss);
>> >                  break;
>> >                  }
>> >
>> > xfrm_bundle_ok:
>> >
>> >                .
>> >                .
>> >                .
>> >
>> >         dst->metrics[RTAX_MTU-1] = mtu; // original code, below my changes
>> >
>> >         if (dst->xfrm->props.mode == XFRM_MODE_TUNNEL)
>> >                 switch (dst->xfrm->props.family)
>> >                 {
>> >                 case AF_INET:
>> >                         dst->metrics[RTAX_ADVMSS-1] = max_t(unsigned
>> > int, mtu - sizeof(struct iphdr) - sizeof(struct tcphdr), 256);
>> >                 break;
>> >
>> >                 case AF_INET6:
>> >                         dst->metrics[RTAX_ADVMSS-1] = max_t(unsigned
>> > int, mtu - sizeof(struct ipv6hdr) - sizeof(struct tcphdr),
>> >
>> > dev_net(dst->dev)->ipv6.sysctl.ip6_rt_min_advmss);
>> >                 break;
>> >                 }
>> >
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

Re: [Bugme-new] [Bug 15682] New: XFRM is not updating RTAX_ADVMSS metric

[jamal]

Hi Eduardo,

As a first step, I dont know what the real cause is but 
what you showed as a solution didnt look right. I am also not familiar
with DSMIPv6. I know a lot of other xfrm subsystems work fine with
the current code. So thats why i pointed to Herbert who is more
knowledgeable. I am adding Yoshfuji to the list. 
If you tell me what kernel options to turn on and a very
basic test setup to reproduce it (I dont have much hardware, so make it
very minimal maybe requiring one or two PCs max) then I could make time
and try to reproduce it.

cheers,
jamal

On Tue, 2010-04-06 at 16:02 -0300, Eduardo Panisset wrote:
> Hi,
> 
> My intention is only to report a problem that I have faced. The
> solution proposed isn't (I know that) probably the best one to adopt
> as I'm not a kernel specialist, it is more illustrative to allow you
> guys understanding what I'm meaning and solve that on the better way
> (hence I haven't submited a patch).
> 
> Regards,
> Eduardo Panisset.

Re: [Bugme-new] [Bug 15682] New: XFRM is not updating RTAX_ADVMSS metric

[Herbert Xu]

On Tue, Apr 06, 2010 at 09:40:40AM -0400, jamal wrote:
> 
> Herbert would give better answers. I dont think what Eduardo is 
> doing is correct. You cant just start factoring in tcp headers
> at the xfrm level - and besides, the mtu calculation
> already takes care tunnel headers - so tcp should be able to 
> compute correct MSS. 

I think Eduardo is basically right.

Unfortunately, we've kind of stuffed up in how we handle ADVMSS.
In particular, we don't update it at all when we get a normal
non-IPsec PTMU message.

As a result, if we now start updating it it may actually break
existing users.

We do provide a locked bit for the user to use, however, because
the fact that we have never updated ADVMSS on the fly before,
people out there may not be aware of the locked bit.

Dave, what do you think about us starting to update ADVMSS on
the fly, just like the MTU?

The only risk is that existing users who are forcing ADVMSS to
a value higher than what it would otherwise be would now get a
lower value, if they're not using the "lock" keyword.

This shouldn't be fatal as it would only result in smaller packets
which should still work, and they can always start using the
"lock" keyword to get back the old behaviour.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [Bugme-new] [Bug 15682] New: XFRM is not updating RTAX_ADVMSS metric

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 7 Apr 2010 21:54:46 +0800

> Dave, what do you think about us starting to update ADVMSS on
> the fly, just like the MTU?

This sounds fine.

> The only risk is that existing users who are forcing ADVMSS to
> a value higher than what it would otherwise be would now get a
> lower value, if they're not using the "lock" keyword.
> 
> This shouldn't be fatal as it would only result in smaller packets
> which should still work, and they can always start using the
> "lock" keyword to get back the old behaviour.

And if we get them to fix their kit and use the lock setting,
it'll work in older kernels too.