[PATCH] inet6: Fix paramater issue of inet6_csk_xmit

[PATCH] inet6: Fix paramater issue of inet6_csk_xmit

[Wei Yongjun]

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 <yjwei@cn.fujitsu.com>
---
 net/ipv6/inet6_connection_sock.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/ipv6/inet6_connection_sock.c b/net/ipv6/inet6_connection_sock.c
index 87801cc..2b5e1fb 100644
--- a/net/ipv6/inet6_connection_sock.c
+++ b/net/ipv6/inet6_connection_sock.c
@@ -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);
-- 
1.5.3.8

Re: [PATCH] inet6: Fix paramater issue of inet6_csk_xmit

[David Miller]

From: Wei Yongjun <yjwei@cn.fujitsu.com>
Date: Thu, 31 Jul 2008 17:56:25 +0800

> 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 <yjwei@cn.fujitsu.com>

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

Re: [PATCH] inet6: Fix paramater issue of inet6_csk_xmit

[Wei Yongjun]

David Miller wrote:
> From: Wei Yongjun <yjwei@cn.fujitsu.com>
> Date: Thu, 31 Jul 2008 17:56:25 +0800
>
>   
>> 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 <yjwei@cn.fujitsu.com>
>>     
>
> 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.

Re: [PATCH] inet6: Fix paramater issue of inet6_csk_xmit

[Herbert Xu]

Wei Yongjun <yjwei@cn.fujitsu.com> wrote:
>
> 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.

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

Thanks,
-- 
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: [PATCH] inet6: Fix paramater issue of inet6_csk_xmit

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Fri, 01 Aug 2008 17:11:01 +0800

> Wei Yongjun <yjwei@cn.fujitsu.com> wrote:
> >
> > 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.
> 
> 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.

Re: [PATCH] inet6: Fix paramater issue of inet6_csk_xmit

[Herbert Xu]

On Fri, Aug 01, 2008 at 02:18:25AM -0700, David Miller wrote:
>
> 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,
-- 
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: [PATCH] inet6: Fix paramater issue of inet6_csk_xmit

[Herbert Xu]

On Fri, Aug 01, 2008 at 05:25:48PM +0800, Herbert Xu wrote:
>
> 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,
-- 
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: [PATCH] inet6: Fix paramater issue of inet6_csk_xmit

[Herbert Xu]

On Fri, Aug 01, 2008 at 07:50:41PM +0800, Herbert Xu wrote:
>
> 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 <herbert@gondor.apana.org.au>

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 535a18f..ab1c472 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -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,
diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
index a238d68..11dab07 100644
--- a/net/sctp/ipv6.c
+++ b/net/sctp/ipv6.c
@@ -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
diff --git a/net/sctp/output.c b/net/sctp/output.c
index 4568464..0dc4a7d 100644
--- a/net/sctp/output.c
+++ b/net/sctp/output.c
@@ -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;
diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index a6e0818..80b8efb 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -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,
-- 
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: [PATCH] inet6: Fix paramater issue of inet6_csk_xmit

[Vlad Yasevich]

Herbert Xu wrote:
> On Fri, Aug 01, 2008 at 07:50:41PM +0800, Herbert Xu wrote:
>> 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

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.

> 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.

> 
> 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 <herbert@gondor.apana.org.au>
> 
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index 535a18f..ab1c472 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -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,
> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> index a238d68..11dab07 100644
> --- a/net/sctp/ipv6.c
> +++ b/net/sctp/ipv6.c
> @@ -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
> diff --git a/net/sctp/output.c b/net/sctp/output.c
> index 4568464..0dc4a7d 100644
> --- a/net/sctp/output.c
> +++ b/net/sctp/output.c
> @@ -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;
> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> index a6e0818..80b8efb 100644
> --- a/net/sctp/protocol.c
> +++ b/net/sctp/protocol.c
> @@ -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.
> +	 */

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

> +	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

> +
>  	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,

Re: [PATCH] inet6: Fix paramater issue of inet6_csk_xmit

[Herbert Xu]

On Fri, Aug 01, 2008 at 10:50:17AM -0400, Vlad Yasevich wrote:
>
> > +	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,
-- 
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: [PATCH] inet6: Fix paramater issue of inet6_csk_xmit

[Vlad Yasevich]

Herbert Xu wrote:
> On Fri, Aug 01, 2008 at 10:50:17AM -0400, Vlad Yasevich wrote:
>>> +	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,

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

Re: [PATCH] inet6: Fix paramater issue of inet6_csk_xmit

[Herbert Xu]

On Sat, Aug 02, 2008 at 07:26:29PM -0400, Vlad Yasevich wrote:
>
> 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!
-- 
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: [PATCH] inet6: Fix paramater issue of inet6_csk_xmit

[David Miller]

From: Vlad Yasevich <vladislav.yasevich@hp.com>
Date: Sat, 02 Aug 2008 19:26:29 -0400

> Herbert Xu wrote:
> > On Fri, Aug 01, 2008 at 10:50:17AM -0400, Vlad Yasevich wrote:
> >>> +	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,
> 
> 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.

Re: [PATCH] inet6: Fix paramater issue of inet6_csk_xmit

[Vlad Yasevich]

Hi Herbert

Herbert Xu wrote:
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index 535a18f..ab1c472 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -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,
> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> index a238d68..11dab07 100644
> --- a/net/sctp/ipv6.c
> +++ b/net/sctp/ipv6.c
> @@ -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;
> +

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

> +	return ip6_xmit(sk, skb, &fl, np->opt, 0);
>  }
>  
>  /* Returns the dst cache entry for the given source and destination ip
> diff --git a/net/sctp/output.c b/net/sctp/output.c
> index 4568464..0dc4a7d 100644
> --- a/net/sctp/output.c
> +++ b/net/sctp/output.c
> @@ -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;
> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> index a6e0818..80b8efb 100644
> --- a/net/sctp/protocol.c
> +++ b/net/sctp/protocol.c
> @@ -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.
> +	 */

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

> +	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

Re: [PATCH] inet6: Fix paramater issue of inet6_csk_xmit

[Herbert Xu]

Vlad Yasevich <vladislav.yasevich@hp.com> wrote:
>
> 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 <herbert@gondor.apana.org.au>

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 535a18f..ab1c472 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -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,
diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
index a238d68..483a01d 100644
--- a/net/sctp/ipv6.c
+++ b/net/sctp/ipv6.c
@@ -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
diff --git a/net/sctp/output.c b/net/sctp/output.c
index 4568464..0dc4a7d 100644
--- a/net/sctp/output.c
+++ b/net/sctp/output.c
@@ -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;
diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index a6e0818..0b65354 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -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,
-- 
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: [PATCH] inet6: Fix paramater issue of inet6_csk_xmit

[Wei Yongjun]

Hi Herbert Xu:

> Vlad Yasevich <vladislav.yasevich@hp.com> wrote:
>   
>> 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.
>
>   
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 <yjwei@cn.fujitsu.com>
---
 net/ipv6/ip6_output.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 0969f2a..e9f2a35 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -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,
-- 
1.5.3.8

Re: [PATCH] inet6: Fix paramater issue of inet6_csk_xmit

[Herbert Xu]

On Mon, Aug 04, 2008 at 10:58:06AM +0800, Wei Yongjun wrote:
>
> You need this patch to let sctp under IPv6 do the correct thing.

Good point.

> 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 <yjwei@cn.fujitsu.com>

Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

Thanks,
-- 
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: [PATCH] inet6: Fix paramater issue of inet6_csk_xmit

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Mon, 4 Aug 2008 11:05:07 +0800

> On Mon, Aug 04, 2008 at 10:58:06AM +0800, Wei Yongjun wrote:
> >
> > You need this patch to let sctp under IPv6 do the correct thing.
> 
> Good point.
> 
> > 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 <yjwei@cn.fujitsu.com>
> 
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

Also applied, thanks everyone.

Re: [PATCH] inet6: Fix paramater issue of inet6_csk_xmit

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Mon, 04 Aug 2008 09:55:19 +0800

> sctp: Drop ipfargok in sctp_xmit function
 ...
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied, thanks.