Skip to contentSkip to conversation

inet6: Fix paramater issue of inet6_csk_xmit

PATCH Applied Wei Yongjun 4 participants 19 messages Jul 31, 2008 – Aug 4, 2008

Subsystems (5)
Networking Network drivers IPv4/IPv6 TCP +1 more subsystems
lore
Share & tools
lore

Wei Yongjun

author
#0 lore raw

PATCH 1/1 inet6: Fix paramater issue of inet6_csk_xmit

inet6_csk_xmit() has a paramater ipfragok, but never pass to ip6_xmit(), always call it with ipfragok = 0.

This patch fixed this problem by instead 0 of ipfragok.

Compiled test only.

Signed-off-by Wei Yongjun <[email protected]>

---
net/ipv6/inet6_connection_sock.c | 2 +-

1 files changed, 1 insertions(+), 1 deletions(-)
net/ipv6/inet6_connection_sock.c +0 -0
index 87801cc..2b5e1fb 100644
@@ -233,7 +233,7 @@ int inet6_csk_xmit(struct sk_buff *skb, int ipfragok)

	/* Restore final destination back after routing done */
	ipv6_addr_copy(&fl.fl6_dst, &np->daddr);

- return ip6_xmit(sk, skb, &fl, np->opt, 0);
+ return ip6_xmit(sk, skb, &fl, np->opt, ipfragok);
}

EXPORT_SYMBOL_GPL(inet6_csk_xmit);

Signature · 1 line
1.5.3.8
maintainer
#1 lore raw

From: Wei Yongjun <[email protected]>
Date: Thu, 31 Jul 2008 17:56:25 +0800

Quoted 8 lines

inet6_csk_xmit() has a paramater ipfragok, but never pass to ip6_xmit(), always call it with ipfragok = 0.

This patch fixed this problem by instead 0 of ipfragok.

Compiled test only.

Signed-off-by Wei Yongjun <[email protected]>

I would like to see someone do some testing of this patch before I apply it.

author
#2 lore raw
Quoted 9 lines · David Miller

From: Wei Yongjun <[email protected]>
Date: Thu, 31 Jul 2008 17:56:25 +0800

Quoted 5 lines

inet6_csk_xmit() has a paramater ipfragok, but never pass to ip6_xmit(), always call it with ipfragok = 0.

This patch fixed this problem by instead 0 of ipfragok.

Quoted 8 lines
Quoted 4 lines

Compiled test only.

Signed-off-by Wei Yongjun <[email protected]>

I would like to see someone do some testing of this patch before I apply it.

inet6_csk_xmit() just be used by tcp and dccp:
1042 net/dccp/ipv6.c .queue_xmit = inet6_csk_xmit
1853 net/ipv6/tcp_ipv6.c .queue_xmit = inet6_csk_xmit,

And used always with ipfragok = 0.
139 net/dccp/output.c err = icsk->icsk_af_ops->queue_xmit(skb, 0);
626 net/ipv4/tcp_output.c err = icsk->icsk_af_ops->queue_xmit(skb, 0);

I can not find any place used inet6_csk_xmit() with ipfragok = 1.

So I think it is safe to apply this patch. Or maybe this paramater is useless.

#3 lore raw
Quoted 5 lines · Wei Yongjun

inet6_csk_xmit() just be used by tcp and dccp:
1042 net/dccp/ipv6.c .queue_xmit = inet6_csk_xmit
1853 net/ipv6/tcp_ipv6.c .queue_xmit = inet6_csk_xmit,

Quoted 8 lines

And used always with ipfragok = 0.
139 net/dccp/output.c err = icsk->icsk_af_ops->queue_xmit(skb, 0);
626 net/ipv4/tcp_output.c err = icsk->icsk_af_ops->queue_xmit(skb, 0);

I can not find any place used inet6_csk_xmit() with ipfragok = 1.

So I think it is safe to apply this patch. Or maybe this paramater is useless.

No we should get rid of the ipfragok argument if noone uses it.

Thanks,

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
maintainer
#4 lore raw

From: Herbert Xu <[email protected]>
Date: Fri, 01 Aug 2008 17:11:01 +0800

Quoted 10 lines
Quoted 9 lines · Wei Yongjun

inet6_csk_xmit() just be used by tcp and dccp:
1042 net/dccp/ipv6.c .queue_xmit = inet6_csk_xmit
1853 net/ipv6/tcp_ipv6.c .queue_xmit = inet6_csk_xmit,

And used always with ipfragok = 0.
139 net/dccp/output.c err = icsk->icsk_af_ops->queue_xmit(skb, 0);
626 net/ipv4/tcp_output.c err = icsk->icsk_af_ops->queue_xmit(skb, 0);

Quoted 6 lines
Quoted 4 lines

I can not find any place used inet6_csk_xmit() with ipfragok = 1.

So I think it is safe to apply this patch. Or maybe this paramater is useless.

No we should get rid of the ipfragok argument if noone uses it.

SCTP calls ip_queue_xmit() and ip6_xmit() directly with it's ipfragok argument, so it is used for these functions.

As a result ->icsk_af_ops->queue_xmit() needs to have arguments at least matching those function's prototypes.

#5 lore raw
Quoted 6 lines · David Miller

SCTP calls ip_queue_xmit() and ip6_xmit() directly with it's ipfragok argument, so it is used for these functions.

As a result ->icsk_af_ops->queue_xmit() needs to have arguments at least matching those function's prototypes.

Hmm, so SCTP is the only reason why ipfragok exists and yet it doesn't even use icsk_af_ops->queue_xmit. So unless SCTP has plans to use icsk_af_ops, perhaps we could provide it a copy of ip_queue_xmit/ip6_xmit that has the ipfragok argument and then put a wrapper around them without the argument for icsk_af_ops and everybody else.

Who knows, it might even save more memory than the ethtool hack :)

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
#6 lore raw
Quoted 7 lines · Herbert Xu

Hmm, so SCTP is the only reason why ipfragok exists and yet it doesn't even use icsk_af_ops->queue_xmit. So unless SCTP has plans to use icsk_af_ops, perhaps we could provide it a copy of ip_queue_xmit/ip6_xmit that has the ipfragok argument and then put a wrapper around them without the argument for icsk_af_ops and everybody else.

I dug up this thread from the archives:

http://osdir.com/ml/network/2003-01/msg00065.html

SCTP is just a POS.

However, I think we can improve on what we have at the moment. Really we only need to clear the DF bit on packets which are above the PMTU on output. There is no need to clear the DF for packets which we don't need to fragment ourselves.

Yes, the RFC says we SHOULD do that, but it's braindead. In any case, this isn't allowed by IPv6 and it makes little sense for IPv4. If we are doing PMTU then we want to get that error back in all cases.

So we should be able to simply replace ipfragok with skb->local_df. The only trouble is that SCTP doesn't seem to keep its PMTU setting in the usual spot so we'll need to fix that before this can be done.

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
#7 lore raw
Quoted 4 lines · Herbert Xu

So we should be able to simply replace ipfragok with skb->local_df. The only trouble is that SCTP doesn't seem to keep its PMTU setting in the usual spot so we'll need to fix that before this can be done.

OK I think this should work:

sctp: Drop ipfargok in sctp_xmit function

The ipfragok flag controls whether the packet may be fragmented either on the local host on beyond. The latter is only valid on IPv4.

In fact, we never want to do the latter even on IPv4 when PMTU is enabled. This is because even though we can't fragment packets within SCTP due to the prtocol's inherent faults, we can still fragment it at IP layer. By setting the DF bit we will improve the PMTU process.

RFC 2960 only says that we SHOULD clear the DF bit in this case, so we're compliant even if we set the DF bit.

Once we make this change, we only need to control the local fragmentation. There is already a bit in the skb which controls that, local_df. So this patch sets that instead of using the ipfragok argument.

The only complication is that there isn't a struct sock object per transport, so for IPv4 we have to resort to changing the pmtudisc field for every packet. This should be safe though as the protocol is single-threaded.

Note that after this patch we can remove ipfragok from the rest of the stack too.

Signed-off-by Herbert Xu <[email protected]>
include/net/sctp/structs.h +1 -2
index 535a18f..ab1c472 100644
@@ -524,8 +524,7 @@ static inline void sctp_ssn_skip(struct sctp_stream *stream, __u16 id,
  */
 struct sctp_af {
 	int		(*sctp_xmit)	(struct sk_buff *skb,
-					 struct sctp_transport *,
-					 int ipfragok);
+					 struct sctp_transport *);
 	int		(*setsockopt)	(struct sock *sk,
 					 int level,
 					 int optname,
net/sctp/ipv6.c +5 -3
index a238d68..11dab07 100644
@@ -195,8 +195,7 @@ out:
 }
 
 /* Based on tcp_v6_xmit() in tcp_ipv6.c. */
-static int sctp_v6_xmit(struct sk_buff *skb, struct sctp_transport *transport,
-			int ipfragok)
+static int sctp_v6_xmit(struct sk_buff *skb, struct sctp_transport *transport)
 {
 	struct sock *sk = skb->sk;
 	struct ipv6_pinfo *np = inet6_sk(sk);

@@ -231,7 +230,10 @@ static int sctp_v6_xmit(struct sk_buff *skb, struct sctp_transport *transport,
 
 	SCTP_INC_STATS(SCTP_MIB_OUTSCTPPACKS);
 
-	return ip6_xmit(sk, skb, &fl, np->opt, ipfragok);
+	if (!(tp->param_flags & SPP_PMTUD_ENABLE))
+		skb->local_df = 1;
+
+	return ip6_xmit(sk, skb, &fl, np->opt, 0);
 }
 
 /* Returns the dst cache entry for the given source and destination ip
net/sctp/output.c +2 -4
index 4568464..0dc4a7d 100644
@@ -586,10 +586,8 @@ int sctp_packet_transmit(struct sctp_packet *packet)
 	SCTP_DEBUG_PRINTK("***sctp_transmit_packet*** skb len %d\n",
 			  nskb->len);
 
-	if (tp->param_flags & SPP_PMTUD_ENABLE)
-		(*tp->af_specific->sctp_xmit)(nskb, tp, packet->ipfragok);
-	else
-		(*tp->af_specific->sctp_xmit)(nskb, tp, 1);
+	nskb->local_df = packet->ipfragok;
+	(*tp->af_specific->sctp_xmit)(nskb, tp);
 
 out:
 	packet->size = packet->overhead;
net/sctp/protocol.c +11 -2
index a6e0818..80b8efb 100644
@@ -862,16 +862,25 @@ static int sctp_inet_supported_addrs(const struct sctp_sock *opt,
 
 /* Wrapper routine that calls the ip transmit routine. */
 static inline int sctp_v4_xmit(struct sk_buff *skb,
-			       struct sctp_transport *transport, int ipfragok)
+			       struct sctp_transport *transport)
 {
+	struct inet_sock *inet = inet_sk(skb->sk);
+
 	SCTP_DEBUG_PRINTK("%s: skb:%p, len:%d, "
 			  "src:%u.%u.%u.%u, dst:%u.%u.%u.%u\n",
 			  __func__, skb, skb->len,
 			  NIPQUAD(skb->rtable->rt_src),
 			  NIPQUAD(skb->rtable->rt_dst));
 
+	/*
+	 * You'd think with a protocol this big they could afford
+	 * a struct sock per transport.
+	 */
+	inet->pmtudisc = transport->param_flags & SPP_PMTUD_ENABLE ?
+			 IP_PMTUDISC_DO : IP_PMTUDISC_DONT;
+
 	SCTP_INC_STATS(SCTP_MIB_OUTSCTPPACKS);
-	return ip_queue_xmit(skb, ipfragok);
+	return ip_queue_xmit(skb, 0);
 }
 
 static struct sctp_af sctp_af_inet;

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
#8 lore raw
Quoted 7 lines · Herbert Xu
Quoted 3 lines · Herbert Xu

So we should be able to simply replace ipfragok with skb->local_df. The only trouble is that SCTP doesn't seem to keep its PMTU setting in the usual spot so we'll need to fix that before this can be done.

OK I think this should work:

Quoted 8 lines

sctp: Drop ipfargok in sctp_xmit function

The ipfragok flag controls whether the packet may be fragmented either on the local host on beyond. The latter is only valid on IPv4.

In fact, we never want to do the latter even on IPv4 when PMTU is enabled. This is because even though we can't fragment packets

I think you mean that we can't "re-fragment packets". The reason for that is how SCTP handles sequence numbering, Instead of numbering bytes, it number messages. Thus once the number is assigned, you can't re-fragment it as that would result in different sequence numbers.

Quoted 6 lines

within SCTP due to the prtocol's inherent faults, we can still fragment it at IP layer. By setting the DF bit we will improve the PMTU process.

RFC 2960 only says that we SHOULD clear the DF bit in this case, so we're compliant even if we set the DF bit.

RFC 4960 doesn't mention DF bit anywhere, so that's not an issue.

Quoted 86 lines

Once we make this change, we only need to control the local fragmentation. There is already a bit in the skb which controls that, local_df. So this patch sets that instead of using the ipfragok argument.

The only complication is that there isn't a struct sock object per transport, so for IPv4 we have to resort to changing the pmtudisc field for every packet. This should be safe though as the protocol is single-threaded.

Note that after this patch we can remove ipfragok from the rest of the stack too.

Signed-off-by Herbert Xu <[email protected]>
include/net/sctp/structs.h +1 -2
index 535a18f..ab1c472 100644
@@ -524,8 +524,7 @@ static inline void sctp_ssn_skip(struct sctp_stream *stream, __u16 id,
  */
 struct sctp_af {
 	int		(*sctp_xmit)	(struct sk_buff *skb,
-					 struct sctp_transport *,
-					 int ipfragok);
+					 struct sctp_transport *);
 	int		(*setsockopt)	(struct sock *sk,
 					 int level,
 					 int optname,
net/sctp/ipv6.c +5 -3
index a238d68..11dab07 100644
@@ -195,8 +195,7 @@ out:
 }
 
 /* Based on tcp_v6_xmit() in tcp_ipv6.c. */
-static int sctp_v6_xmit(struct sk_buff *skb, struct sctp_transport *transport,
-			int ipfragok)
+static int sctp_v6_xmit(struct sk_buff *skb, struct sctp_transport *transport)
 {
 	struct sock *sk = skb->sk;
 	struct ipv6_pinfo *np = inet6_sk(sk);

@@ -231,7 +230,10 @@ static int sctp_v6_xmit(struct sk_buff *skb, struct sctp_transport *transport,
 
 	SCTP_INC_STATS(SCTP_MIB_OUTSCTPPACKS);
 
-	return ip6_xmit(sk, skb, &fl, np->opt, ipfragok);
+	if (!(tp->param_flags & SPP_PMTUD_ENABLE))
+		skb->local_df = 1;
+
+	return ip6_xmit(sk, skb, &fl, np->opt, 0);
 }
 
 /* Returns the dst cache entry for the given source and destination ip
net/sctp/output.c +2 -4
index 4568464..0dc4a7d 100644
@@ -586,10 +586,8 @@ int sctp_packet_transmit(struct sctp_packet *packet)
 	SCTP_DEBUG_PRINTK("***sctp_transmit_packet*** skb len %d\n",
 			  nskb->len);
 
-	if (tp->param_flags & SPP_PMTUD_ENABLE)
-		(*tp->af_specific->sctp_xmit)(nskb, tp, packet->ipfragok);
-	else
-		(*tp->af_specific->sctp_xmit)(nskb, tp, 1);
+	nskb->local_df = packet->ipfragok;
+	(*tp->af_specific->sctp_xmit)(nskb, tp);
 
 out:
 	packet->size = packet->overhead;
net/sctp/protocol.c +3 -1
index a6e0818..80b8efb 100644
@@ -862,16 +862,25 @@ static int sctp_inet_supported_addrs(const struct sctp_sock *opt,
 
 /* Wrapper routine that calls the ip transmit routine. */
 static inline int sctp_v4_xmit(struct sk_buff *skb,
-			       struct sctp_transport *transport, int ipfragok)
+			       struct sctp_transport *transport)
 {
+	struct inet_sock *inet = inet_sk(skb->sk);
+

net/sctp/protocol.c static int sctp_inet_supported_addrs(const struct sctp_sock *opt, · line 878

Quoted 10 lines
net/sctp/protocol.c +4 -0
 	SCTP_DEBUG_PRINTK("%s: skb:%p, len:%d, "
 			  "src:%u.%u.%u.%u, dst:%u.%u.%u.%u\n",
 			  __func__, skb, skb->len,
 			  NIPQUAD(skb->rtable->rt_src),
 			  NIPQUAD(skb->rtable->rt_dst));
 
+	/*
+	 * You'd think with a protocol this big they could afford
+	 * a struct sock per transport.
+	 */

Please don't add comments like this. It does not contribute anything.

net/sctp/protocol.c static int sctp_inet_supported_addrs(const struct sctp_sock *opt, · line 880

Quoted 2 lines
net/sctp/protocol.c +2 -0
+	inet->pmtudisc = transport->param_flags & SPP_PMTUD_ENABLE ?
+			 IP_PMTUDISC_DO : IP_PMTUDISC_DONT;

If you insist on doing this, you need to save the old value and restore it after ip_queue_xmit().

The reason is that PMTU discovery change on a specific transport or association should not affect the socket, since there could be multiple associations on a given socket. Each association has it's own control.

Thanks
-vlad

Quoted 9 lines
net/sctp/protocol.c +2 -1
+
 	SCTP_INC_STATS(SCTP_MIB_OUTSCTPPACKS);
-	return ip_queue_xmit(skb, ipfragok);
+	return ip_queue_xmit(skb, 0);
 }
 
 static struct sctp_af sctp_af_inet;

Cheers,

#9 lore raw
Quoted 10 lines · Vlad Yasevich

net/sctp/protocol.c static int sctp_inet_supported_addrs(const struct sctp_sock *opt, · line 880

Quoted 3 lines
net/sctp/protocol.c +2 -0
+	inet->pmtudisc = transport->param_flags & SPP_PMTUD_ENABLE ?
+			 IP_PMTUDISC_DO : IP_PMTUDISC_DONT;

If you insist on doing this, you need to save the old value and restore it after ip_queue_xmit().

The reason is that PMTU discovery change on a specific transport or association should not affect the socket, since there could be multiple associations on a given socket. Each association has it's own control.

Yes but they'll all pass through here so it'll get set to the appropriate value.

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
#10 lore raw
Quoted 6 lines · Herbert Xu
Quoted 5 lines · Vlad Yasevich

net/sctp/protocol.c static int sctp_inet_supported_addrs(const struct sctp_sock *opt, · line 880

Quoted 2 lines
net/sctp/protocol.c +2 -0
+	inet->pmtudisc = transport->param_flags & SPP_PMTUD_ENABLE ?
+			 IP_PMTUDISC_DO : IP_PMTUDISC_DONT;

If you insist on doing this, you need to save the old value and restore it after ip_queue_xmit().

Quoted 8 lines
Quoted 3 lines

The reason is that PMTU discovery change on a specific transport or association should not affect the socket, since there could be multiple associations on a given socket. Each association has it's own control.

Yes but they'll all pass through here so it'll get set to the appropriate value.

Cheers,

OK, I see what you mean. Let me run some tests with this patch. I doesn't look like it will cause any issues, but I'd like to verify.

Thanks
-vlad

#11 lore raw
Quoted 3 lines · Vlad Yasevich

OK, I see what you mean. Let me run some tests with this patch. I doesn't look like it will cause any issues, but I'd like to verify.

Thanks Vlad!

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
maintainer
#12 lore raw
In reply to Vlad Yasevich

From: Vlad Yasevich <[email protected]>
Date: Sat, 02 Aug 2008 19:26:29 -0400

Quoted 11 lines
Quoted 10 lines · Herbert Xu
Quoted 8 lines · Vlad Yasevich

net/sctp/protocol.c static int sctp_inet_supported_addrs(const struct sctp_sock *opt, · line 880

Quoted 2 lines
net/sctp/protocol.c +2 -0
+	inet->pmtudisc = transport->param_flags & SPP_PMTUD_ENABLE ?
+			 IP_PMTUDISC_DO : IP_PMTUDISC_DONT;

If you insist on doing this, you need to save the old value and restore it after ip_queue_xmit().

The reason is that PMTU discovery change on a specific transport or association should not affect the socket, since there could be multiple associations on a given socket. Each association has it's own control.

Quoted 7 lines
Quoted 4 lines

Yes but they'll all pass through here so it'll get set to the appropriate value.

Cheers,

OK, I see what you mean. Let me run some tests with this patch. I doesn't look like it will cause any issues, but I'd like to verify.

Like Herbert said, thanks for doing this testing.

Let me know how it goes so I can apply the patch if everything is OK.

#13 lore raw
In reply to Herbert Xu

Hi Herbert

Quoted 29 lines · Herbert Xu
include/net/sctp/structs.h +1 -2
index 535a18f..ab1c472 100644
@@ -524,8 +524,7 @@ static inline void sctp_ssn_skip(struct sctp_stream *stream, __u16 id,
  */
 struct sctp_af {
 	int		(*sctp_xmit)	(struct sk_buff *skb,
-					 struct sctp_transport *,
-					 int ipfragok);
+					 struct sctp_transport *);
 	int		(*setsockopt)	(struct sock *sk,
 					 int level,
 					 int optname,
net/sctp/ipv6.c +1 -2
index a238d68..11dab07 100644
@@ -195,8 +195,7 @@ out:
 }
 
 /* Based on tcp_v6_xmit() in tcp_ipv6.c. */
-static int sctp_v6_xmit(struct sk_buff *skb, struct sctp_transport *transport,
-			int ipfragok)
+static int sctp_v6_xmit(struct sk_buff *skb, struct sctp_transport *transport)
 {
 	struct sock *sk = skb->sk;
 	struct ipv6_pinfo *np = inet6_sk(sk);

net/sctp/ipv6.c static int sctp_v6_xmit(struct sk_buff *skb, struct sctp_transport *transport, · line 235

Quoted 7 lines
net/sctp/ipv6.c +3 -1
@@ -231,7 +230,10 @@ static int sctp_v6_xmit(struct sk_buff *skb, struct sctp_transport *transport,
 
 	SCTP_INC_STATS(SCTP_MIB_OUTSCTPPACKS);
 
-	return ip6_xmit(sk, skb, &fl, np->opt, ipfragok);
+	if (!(tp->param_flags & SPP_PMTUD_ENABLE))
+		skb->local_df = 1;
+

s/tp/transport. Otherwise, will not compile.

Quoted 34 lines
net/sctp/ipv6.c +1 -0
+	return ip6_xmit(sk, skb, &fl, np->opt, 0);
 }
 
 /* Returns the dst cache entry for the given source and destination ip
net/sctp/output.c +2 -4
index 4568464..0dc4a7d 100644
@@ -586,10 +586,8 @@ int sctp_packet_transmit(struct sctp_packet *packet)
 	SCTP_DEBUG_PRINTK("***sctp_transmit_packet*** skb len %d\n",
 			  nskb->len);
 
-	if (tp->param_flags & SPP_PMTUD_ENABLE)
-		(*tp->af_specific->sctp_xmit)(nskb, tp, packet->ipfragok);
-	else
-		(*tp->af_specific->sctp_xmit)(nskb, tp, 1);
+	nskb->local_df = packet->ipfragok;
+	(*tp->af_specific->sctp_xmit)(nskb, tp);
 
 out:
 	packet->size = packet->overhead;
net/sctp/protocol.c +3 -1
index a6e0818..80b8efb 100644
@@ -862,16 +862,25 @@ static int sctp_inet_supported_addrs(const struct sctp_sock *opt,
 
 /* Wrapper routine that calls the ip transmit routine. */
 static inline int sctp_v4_xmit(struct sk_buff *skb,
-			       struct sctp_transport *transport, int ipfragok)
+			       struct sctp_transport *transport)
 {
+	struct inet_sock *inet = inet_sk(skb->sk);
+

net/sctp/protocol.c static int sctp_inet_supported_addrs(const struct sctp_sock *opt, · line 878

Quoted 10 lines
net/sctp/protocol.c +4 -0
 	SCTP_DEBUG_PRINTK("%s: skb:%p, len:%d, "
 			  "src:%u.%u.%u.%u, dst:%u.%u.%u.%u\n",
 			  __func__, skb, skb->len,
 			  NIPQUAD(skb->rtable->rt_src),
 			  NIPQUAD(skb->rtable->rt_dst));
 
+	/*
+	 * You'd think with a protocol this big they could afford
+	 * a struct sock per transport.
+	 */

Please change this comment to be descriptive of the code, or just remove it.

Quoted 11 lines
net/sctp/protocol.c +4 -1
+	inet->pmtudisc = transport->param_flags & SPP_PMTUD_ENABLE ?
+			 IP_PMTUDISC_DO : IP_PMTUDISC_DONT;
+
 	SCTP_INC_STATS(SCTP_MIB_OUTSCTPPACKS);
-	return ip_queue_xmit(skb, ipfragok);
+	return ip_queue_xmit(skb, 0);
 }
 
 static struct sctp_af sctp_af_inet;

Cheers,

Otherwise, all of my tests have passed.

Thanks
-vlad

#14 lore raw
Quoted 2 lines · Vlad Yasevich

Otherwise, all of my tests have passed.

Thanks for testing, here is the corrected patch:

sctp: Drop ipfargok in sctp_xmit function

The ipfragok flag controls whether the packet may be fragmented either on the local host on beyond. The latter is only valid on IPv4.

In fact, we never want to do the latter even on IPv4 when PMTU is enabled. This is because even though we can't fragment packets within SCTP due to the prtocol's inherent faults, we can still fragment it at IP layer. By setting the DF bit we will improve the PMTU process.

RFC 2960 only says that we SHOULD clear the DF bit in this case, so we're compliant even if we set the DF bit. In fact RFC 4960 no longer has this statement.

Once we make this change, we only need to control the local fragmentation. There is already a bit in the skb which controls that, local_df. So this patch sets that instead of using the ipfragok argument.

The only complication is that there isn't a struct sock object per transport, so for IPv4 we have to resort to changing the pmtudisc field for every packet. This should be safe though as the protocol is single-threaded.

Note that after this patch we can remove ipfragok from the rest of the stack too.

Signed-off-by Herbert Xu <[email protected]>
include/net/sctp/structs.h +1 -2
index 535a18f..ab1c472 100644
@@ -524,8 +524,7 @@ static inline void sctp_ssn_skip(struct sctp_stream *stream, __u16 id,
  */
 struct sctp_af {
 	int		(*sctp_xmit)	(struct sk_buff *skb,
-					 struct sctp_transport *,
-					 int ipfragok);
+					 struct sctp_transport *);
 	int		(*setsockopt)	(struct sock *sk,
 					 int level,
 					 int optname,
net/sctp/ipv6.c +5 -3
index a238d68..483a01d 100644
@@ -195,8 +195,7 @@ out:
 }
 
 /* Based on tcp_v6_xmit() in tcp_ipv6.c. */
-static int sctp_v6_xmit(struct sk_buff *skb, struct sctp_transport *transport,
-			int ipfragok)
+static int sctp_v6_xmit(struct sk_buff *skb, struct sctp_transport *transport)
 {
 	struct sock *sk = skb->sk;
 	struct ipv6_pinfo *np = inet6_sk(sk);

@@ -231,7 +230,10 @@ static int sctp_v6_xmit(struct sk_buff *skb, struct sctp_transport *transport,
 
 	SCTP_INC_STATS(SCTP_MIB_OUTSCTPPACKS);
 
-	return ip6_xmit(sk, skb, &fl, np->opt, ipfragok);
+	if (!(transport->param_flags & SPP_PMTUD_ENABLE))
+		skb->local_df = 1;
+
+	return ip6_xmit(sk, skb, &fl, np->opt, 0);
 }
 
 /* Returns the dst cache entry for the given source and destination ip
net/sctp/output.c +2 -4
index 4568464..0dc4a7d 100644
@@ -586,10 +586,8 @@ int sctp_packet_transmit(struct sctp_packet *packet)
 	SCTP_DEBUG_PRINTK("***sctp_transmit_packet*** skb len %d\n",
 			  nskb->len);
 
-	if (tp->param_flags & SPP_PMTUD_ENABLE)
-		(*tp->af_specific->sctp_xmit)(nskb, tp, packet->ipfragok);
-	else
-		(*tp->af_specific->sctp_xmit)(nskb, tp, 1);
+	nskb->local_df = packet->ipfragok;
+	(*tp->af_specific->sctp_xmit)(nskb, tp);
 
 out:
 	packet->size = packet->overhead;
net/sctp/protocol.c +7 -2
index a6e0818..0b65354 100644
@@ -862,16 +862,21 @@ static int sctp_inet_supported_addrs(const struct sctp_sock *opt,
 
 /* Wrapper routine that calls the ip transmit routine. */
 static inline int sctp_v4_xmit(struct sk_buff *skb,
-			       struct sctp_transport *transport, int ipfragok)
+			       struct sctp_transport *transport)
 {
+	struct inet_sock *inet = inet_sk(skb->sk);
+
 	SCTP_DEBUG_PRINTK("%s: skb:%p, len:%d, "
 			  "src:%u.%u.%u.%u, dst:%u.%u.%u.%u\n",
 			  __func__, skb, skb->len,
 			  NIPQUAD(skb->rtable->rt_src),
 			  NIPQUAD(skb->rtable->rt_dst));
 
+	inet->pmtudisc = transport->param_flags & SPP_PMTUD_ENABLE ?
+			 IP_PMTUDISC_DO : IP_PMTUDISC_DONT;
+
 	SCTP_INC_STATS(SCTP_MIB_OUTSCTPPACKS);
-	return ip_queue_xmit(skb, ipfragok);
+	return ip_queue_xmit(skb, 0);
 }
 
 static struct sctp_af sctp_af_inet;

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
author
#15 lore raw

Hi Herbert Xu:

Quoted 28 lines
Quoted 2 lines · Vlad Yasevich

Otherwise, all of my tests have passed.

Thanks for testing, here is the corrected patch:

sctp: Drop ipfargok in sctp_xmit function

The ipfragok flag controls whether the packet may be fragmented either on the local host on beyond. The latter is only valid on IPv4.

In fact, we never want to do the latter even on IPv4 when PMTU is enabled. This is because even though we can't fragment packets within SCTP due to the prtocol's inherent faults, we can still fragment it at IP layer. By setting the DF bit we will improve the PMTU process.

RFC 2960 only says that we SHOULD clear the DF bit in this case, so we're compliant even if we set the DF bit. In fact RFC 4960 no longer has this statement.

Once we make this change, we only need to control the local fragmentation. There is already a bit in the skb which controls that, local_df. So this patch sets that instead of using the ipfragok argument.

Quoted 9 lines

The only complication is that there isn't a struct sock object per transport, so for IPv4 we have to resort to changing the pmtudisc field for every packet. This should be safe though as the protocol is single-threaded.

Note that after this patch we can remove ipfragok from the rest of the stack too.

You need this patch to let sctp under IPv6 do the correct thing.

ipv6: Do not drop packet if skb->local_df is set to true

The old code will drop IPv6 packet if ipfragok is not set, since ipfragok is obsoleted, will be instead by used skb->local_df, so this check must be changed to skb->local_df.

This patch fix this problem and not drop packet if skb->local_df is set to true.

Signed-off-by Wei Yongjun <[email protected]>

---
net/ipv6/ip6_output.c | 2 +-

1 files changed, 1 insertions(+), 1 deletions(-)
net/ipv6/ip6_output.c +0 -0
index 0969f2a..e9f2a35 100644
@@ -269,7 +269,7 @@ int ip6_xmit(struct sock *sk, struct sk_buff *skb, struct flowi *fl,

	skb->mark = sk->sk_mark;

	mtu = dst_mtu(dst);
-	if ((skb->len <= mtu) || ipfragok || skb_is_gso(skb)) {
+	if ((skb->len <= mtu) || skb->local_df || skb_is_gso(skb)) {
		IP6_INC_STATS(ip6_dst_idev(skb->dst),
			      IPSTATS_MIB_OUTREQUESTS);
		return NF_HOOK(PF_INET6, NF_INET_LOCAL_OUT, skb, NULL, dst->dev,
Signature · 1 line
1.5.3.8
Acked-by
#16 lore raw
Quoted 2 lines · Wei Yongjun

You need this patch to let sctp under IPv6 do the correct thing.

Good point.

Quoted 11 lines

ipv6: Do not drop packet if skb->local_df is set to true

The old code will drop IPv6 packet if ipfragok is not set, since ipfragok is obsoleted, will be instead by used skb->local_df, so this check must be changed
to skb->local_df.

This patch fix this problem and not drop packet if skb->local_df is set to true.

Signed-off-by Wei Yongjun <[email protected]>
Acked-by Herbert Xu <[email protected]>

Thanks,

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
maintainer
#17 lore raw

From: Herbert Xu <[email protected]>
Date: Mon, 4 Aug 2008 11:05:07 +0800

Quoted 13 lines · Wei Yongjun
Quoted 2 lines

You need this patch to let sctp under IPv6 do the correct thing.

Good point.

Quoted 7 lines

ipv6: Do not drop packet if skb->local_df is set to true

The old code will drop IPv6 packet if ipfragok is not set, since ipfragok is obsoleted, will be instead by used skb->local_df, so this check must be changed
to skb->local_df.

Quoted 6 lines
Quoted 4 lines

This patch fix this problem and not drop packet if skb->local_df is set to true.

Signed-off-by Wei Yongjun <[email protected]>
Acked-by Herbert Xu <[email protected]>

Also applied, thanks everyone.