New: XFRM is not updating RTAX_ADVMSS metric

Bugme-new Andrew Morton 5 participants 6 messages Apr 5, 2010 – Apr 7, 2010 1 message not in the archive

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Andrew Morton

author

2010-04-05 19:52 UTC #1 lore raw

In reply to a message not in the archive

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

Quoted 85 lines · On Fri, 2 Apr 2010 17:34:35 GMT [email protected] 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: [email protected]
        ReportedBy: [email protected]
        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;
                }

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

Quoted 96 lines · Andrew Morton

(switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface).
Quoted 79 lines · On Fri, 2 Apr 2010 17:34:35 GMT [email protected] 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: [email protected]
        ReportedBy: [email protected]
        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
Quoted 6 lines
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Eduardo Panisset

2010-04-06 19:02 UTC #3 lore raw

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.

Quoted 109 lines · 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
Quoted 96 lines · Andrew Morton
(switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface).
Quoted 77 lines · On Fri, 2 Apr 2010 17:34:35 GMT [email protected] 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: [email protected]
ReportedBy: [email protected]
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;
Quoted 8 lines
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

jamal

2010-04-07 12:19 UTC #4 lore raw

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

Quoted 10 lines · 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.

Herbert Xu

2010-04-07 13:55 UTC #5 lore raw

In reply to jamal

Quoted 6 lines · 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.

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,

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

David Miller

maintainer

2010-04-07 23:47 UTC #6 lore raw

From: Herbert Xu <[email protected]>
Date: Wed, 7 Apr 2010 21:54:46 +0800

Quoted 2 lines

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

This sounds fine.

Quoted 7 lines

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.