[PATCH] DCCP: Initialize ireq6->pktopts before used it

[PATCH] DCCP: Initialize ireq6->pktopts before used it

[Wei Yongjun]

ireq6->pktopts is not initialized after dccp_reqsk_init(), and it will 
be free in dccp_v6_reqsk_destructor(), so if dccp_parse_options() is 
fail, this may cause kernel panic since ireq6->pktopts is not initialized.

This patch fix this problem by initialize ireq6->pktopts before used it.

static void dccp_v6_reqsk_destructor(struct request_sock *req)
{
        dccp_feat_list_purge(&dccp_rsk(req)->dreq_featneg);
        if (inet6_rsk(req)->pktopts != NULL)
                kfree_skb(inet6_rsk(req)->pktopts);
}

Pid: 0, comm: swapper Not tainted (2.6.26-rc2 #1)
EIP: 0060:[<c05acdaf>] EFLAGS: 00010206 CPU: 0
EIP is at kfree_skb+0x9/0x30
EAX: 00002fde EBX: c7306e80 ECX: c7801080 EDX: 00002fde
ESI: c7983680 EDI: c72d9800 EBP: c075adfc ESP: c075adfc
DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
Process swapper (pid: 0, ti¿75a000 task¿6df3a0 task.ti¿714000)
Stack: c075ae08 c8a259d8 c7a0f848 c075ae38 c8a260fc c7983680 c72d9800 c72d9b90
      64000000 c79836a0 c7306e80 8cf2437f c7a0f848 c7983680 c72d9800 c075ae78
      c89e6c78 c7983680 c72d9800 0a804500 c79836a0 0c011908 f24206cc c46c3660
Call Trace:
[<c8a259d8>] ? dccp_v6_reqsk_destructor+0x1f/0x22 [dccp_ipv6]
[<c8a260fc>] ? dccp_v6_conn_request+0x243/0x27d [dccp_ipv6]
[<c89e6c78>] ? dccp_rcv_state_process+0x3d/0x4b5 [dccp]
[<c8a25976>] ? dccp_v6_do_rcv+0x132/0x175 [dccp_ipv6]
[<c05bb355>] ? sk_filter+0x66/0x6d
[<c05ab5c2>] ? sk_receive_skb+0x32/0x7c
[<c8a267b3>] ? dccp_v6_rcv+0x2a5/0x32a [dccp_ipv6]
[<c8ee2ee0>] ? ip6_input_finish+0x158/0x280 [ipv6]
[<c8ee304a>] ? ip6_input+0x42/0x47 [ipv6]
[<c8ee3357>] ? ipv6_rcv+0x27c/0x2c9 [ipv6]
[<c05b1336>] ? netif_receive_skb+0x2e0/0x349
[<c88f2a12>] ? pcnet32_poll+0x333/0x66e [pcnet32]
[<c0438afa>] ? clocksource_watchdog+0x21e/0x22d
[<c040428b>] ? common_interrupt+0x23/0x28
[<c05b308c>] ? net_rx_action+0x8f/0x147
[<c0427c5b>] ? __do_softirq+0x64/0xcd
[<c0405898>] ? do_softirq+0x55/0x88
[<c0427bf5>] ? irq_exit+0x38/0x3a
[<c0412b42>] ? smp_apic_timer_interrupt+0x71/0x7f
[<c04025eb>] ? default_idle+0x0/0x42
[<c0404348>] ? apic_timer_interrupt+0x28/0x30
[<c04025eb>] ? default_idle+0x0/0x42
[<c0402618>] ? default_idle+0x2d/0x42
[<c0402566>] ? cpu_idle+0x8b/0x9f
[<c060c89a>] ? rest_init+0x4e/0x50
============ 


Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>

--- a/net/dccp/ipv6.c	2008-05-29 22:27:55.000000000 -0400
+++ b/net/dccp/ipv6.c	2008-06-05 05:58:00.000000000 -0400
@@ -413,6 +413,9 @@ static int dccp_v6_conn_request(struct s
 	if (dccp_reqsk_init(req, dccp_sk(sk), skb))
 		goto drop_and_free;
 
+	ireq6 = inet6_rsk(req);
+	ireq6->pktopts	= NULL;
+
 	dreq = dccp_rsk(req);
 	if (dccp_parse_options(sk, dreq, skb))
 		goto drop_and_free;
@@ -420,10 +423,8 @@ static int dccp_v6_conn_request(struct s
 	if (security_inet_conn_request(sk, skb, req))
 		goto drop_and_free;
 
-	ireq6 = inet6_rsk(req);
 	ipv6_addr_copy(&ireq6->rmt_addr, &ipv6_hdr(skb)->saddr);
 	ipv6_addr_copy(&ireq6->loc_addr, &ipv6_hdr(skb)->daddr);
-	ireq6->pktopts	= NULL;
 
 	if (ipv6_opt_accepted(sk, skb) ||
 	    np->rxopt.bits.rxinfo || np->rxopt.bits.rxoinfo ||


--
To unsubscribe from this list: send the line "unsubscribe dccp" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] DCCP: Initialize ireq6->pktopts before used it

[Wei Yongjun]

I think I should add dccp_v6_reqsk_init() do to the init work since 
dccp_reqsk_init may be fail too.

Wei Yongjun wrote:
> ireq6->pktopts is not initialized after dccp_reqsk_init(), and it will 
> be free in dccp_v6_reqsk_destructor(), so if dccp_parse_options() is 
> fail, this may cause kernel panic since ireq6->pktopts is not 
> initialized.
>
> This patch fix this problem by initialize ireq6->pktopts before used it.
>
> static void dccp_v6_reqsk_destructor(struct request_sock *req)
> {
>        dccp_feat_list_purge(&dccp_rsk(req)->dreq_featneg);
>        if (inet6_rsk(req)->pktopts != NULL)
>                kfree_skb(inet6_rsk(req)->pktopts);
> }
>
> Pid: 0, comm: swapper Not tainted (2.6.26-rc2 #1)
> EIP: 0060:[<c05acdaf>] EFLAGS: 00010206 CPU: 0
> EIP is at kfree_skb+0x9/0x30
> EAX: 00002fde EBX: c7306e80 ECX: c7801080 EDX: 00002fde
> ESI: c7983680 EDI: c72d9800 EBP: c075adfc ESP: c075adfc
> DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
> Process swapper (pid: 0, tiÀ75a000 taskÀ6df3a0 task.tiÀ714000)
> Stack: c075ae08 c8a259d8 c7a0f848 c075ae38 c8a260fc c7983680 c72d9800 
> c72d9b90
>      64000000 c79836a0 c7306e80 8cf2437f c7a0f848 c7983680 c72d9800 
> c075ae78
>      c89e6c78 c7983680 c72d9800 0a804500 c79836a0 0c011908 f24206cc 
> c46c3660
> Call Trace:
> [<c8a259d8>] ? dccp_v6_reqsk_destructor+0x1f/0x22 [dccp_ipv6]
> [<c8a260fc>] ? dccp_v6_conn_request+0x243/0x27d [dccp_ipv6]
> [<c89e6c78>] ? dccp_rcv_state_process+0x3d/0x4b5 [dccp]
> [<c8a25976>] ? dccp_v6_do_rcv+0x132/0x175 [dccp_ipv6]
> [<c05bb355>] ? sk_filter+0x66/0x6d
> [<c05ab5c2>] ? sk_receive_skb+0x32/0x7c
> [<c8a267b3>] ? dccp_v6_rcv+0x2a5/0x32a [dccp_ipv6]
> [<c8ee2ee0>] ? ip6_input_finish+0x158/0x280 [ipv6]
> [<c8ee304a>] ? ip6_input+0x42/0x47 [ipv6]
> [<c8ee3357>] ? ipv6_rcv+0x27c/0x2c9 [ipv6]
> [<c05b1336>] ? netif_receive_skb+0x2e0/0x349
> [<c88f2a12>] ? pcnet32_poll+0x333/0x66e [pcnet32]
> [<c0438afa>] ? clocksource_watchdog+0x21e/0x22d
> [<c040428b>] ? common_interrupt+0x23/0x28
> [<c05b308c>] ? net_rx_action+0x8f/0x147
> [<c0427c5b>] ? __do_softirq+0x64/0xcd
> [<c0405898>] ? do_softirq+0x55/0x88
> [<c0427bf5>] ? irq_exit+0x38/0x3a
> [<c0412b42>] ? smp_apic_timer_interrupt+0x71/0x7f
> [<c04025eb>] ? default_idle+0x0/0x42
> [<c0404348>] ? apic_timer_interrupt+0x28/0x30
> [<c04025eb>] ? default_idle+0x0/0x42
> [<c0402618>] ? default_idle+0x2d/0x42
> [<c0402566>] ? cpu_idle+0x8b/0x9f
> [<c060c89a>] ? rest_init+0x4e/0x50
> ===========>
> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
>
> --- a/net/dccp/ipv6.c    2008-05-29 22:27:55.000000000 -0400
> +++ b/net/dccp/ipv6.c    2008-06-05 05:58:00.000000000 -0400
> @@ -413,6 +413,9 @@ static int dccp_v6_conn_request(struct s
>     if (dccp_reqsk_init(req, dccp_sk(sk), skb))
>         goto drop_and_free;
>
> +    ireq6 = inet6_rsk(req);
> +    ireq6->pktopts    = NULL;
> +
>     dreq = dccp_rsk(req);
>     if (dccp_parse_options(sk, dreq, skb))
>         goto drop_and_free;
> @@ -420,10 +423,8 @@ static int dccp_v6_conn_request(struct s
>     if (security_inet_conn_request(sk, skb, req))
>         goto drop_and_free;
>
> -    ireq6 = inet6_rsk(req);
>     ipv6_addr_copy(&ireq6->rmt_addr, &ipv6_hdr(skb)->saddr);
>     ipv6_addr_copy(&ireq6->loc_addr, &ipv6_hdr(skb)->daddr);
> -    ireq6->pktopts    = NULL;
>
>     if (ipv6_opt_accepted(sk, skb) ||
>         np->rxopt.bits.rxinfo || np->rxopt.bits.rxoinfo ||
>
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe dccp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>


-- 
--------------------------------------------------
Wei Yongjun
Development Dept.I
Nanjing Fujitsu Nanda Software Tech. Co., Ltd.(FNST)
8/F., Civil Defense Building, No.189 Guangzhou Road,
Nanjing, 210029, China
TEL: +86+25-86630523-836
COINS: 79955-836
FAX: +86+25-83317685
MAIL: yjwei@cn.fujitsu.com
--------------------------------------------------
This communication is for use by the intended recipient(s) only and may contain information that is privileged, confidential and exempt from disclosure under applicable law. If you are not an intended recipient of this communication, you are hereby notified that any dissemination, distribution or copying hereof is strictly prohibited.  If you have received this communication in error, please notify me by reply e-mail, permanently delete this communication from your system, and destroy any hard copies you may have printed

Re: [PATCH] DCCP: Initialize ireq6->pktopts before used it

[Gerrit Renker]

> I think I should add dccp_v6_reqsk_init() do to the init work since  
> dccp_reqsk_init may be fail too.
>
I think this is not necessary, it can be done like this


	req = inet6_reqsk_alloc(&dccp6_request_sock_ops);
	if (req = NULL)
		goto drop;

	ireq6 = inet6_rsk(req);
	ireq6->pktopts = NULL;

	if (dccp_reqsk_init(req, dccp_sk(sk), skb))
		goto drop_and_free;

This is since dccp_reqsk_init() only initialises the inet_sk and dccp_sk
parts, and does not do IPv6-specific initialisation.

Irrespective of the oops, this is an error and will be fixed in the test
tree today. 

With regard to the oops, the log pointed to the dccp_v6_reqsk_destructor
and so it would make sense, since the pktopts was not initialised to NULL 
and since kfree_skb() calls skb->destructor().

As before, thanks a lot for testing this code and for reporting this.

Gerrit

Re: [PATCH] DCCP: Initialize ireq6->pktopts before used it

[Wei Yongjun]

Gerrit Renker wrote:
>> I think I should add dccp_v6_reqsk_init() do to the init work since  
>> dccp_reqsk_init may be fail too.
>>
>>     
> I think this is not necessary, it can be done like this
>
>
> 	req = inet6_reqsk_alloc(&dccp6_request_sock_ops);
> 	if (req = NULL)
> 		goto drop;
>
> 	ireq6 = inet6_rsk(req);
> 	ireq6->pktopts = NULL;
>
> 	if (dccp_reqsk_init(req, dccp_sk(sk), skb))
> 		goto drop_and_free;
>
> This is since dccp_reqsk_init() only initialises the inet_sk and dccp_sk
> parts, and does not do IPv6-specific initialisation.
>
> Irrespective of the oops, this is an error and will be fixed in the test
> tree today. 
>
> With regard to the oops, the log pointed to the dccp_v6_reqsk_destructor
> and so it would make sense, since the pktopts was not initialised to NULL 
> and since kfree_skb() calls skb->destructor().
>
> As before, thanks a lot for testing this code and for reporting this.
>
>   
Yes, You are right. The patch is valid after changed as you said.

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>

--- a/net/dccp/ipv6.c	2008-05-29 22:27:55.000000000 -0400
+++ b/net/dccp/ipv6.c	2008-06-05 07:02:08.000000000 -0400
@@ -410,6 +410,9 @@ static int dccp_v6_conn_request(struct s
 	if (req = NULL)
 		goto drop;
 
+	ireq6 = inet6_rsk(req);
+	ireq6->pktopts	= NULL;
+
 	if (dccp_reqsk_init(req, dccp_sk(sk), skb))
 		goto drop_and_free;
 
@@ -420,10 +423,8 @@ static int dccp_v6_conn_request(struct s
 	if (security_inet_conn_request(sk, skb, req))
 		goto drop_and_free;
 
-	ireq6 = inet6_rsk(req);
 	ipv6_addr_copy(&ireq6->rmt_addr, &ipv6_hdr(skb)->saddr);
 	ipv6_addr_copy(&ireq6->loc_addr, &ipv6_hdr(skb)->daddr);
-	ireq6->pktopts	= NULL;
 
 	if (ipv6_opt_accepted(sk, skb) ||
 	    np->rxopt.bits.rxinfo || np->rxopt.bits.rxoinfo ||

Re: [PATCH] DCCP: Initialize ireq6->pktopts before used it

[Gerrit Renker]

I have dug out the patch to which this applies. Since it is a bug-fix, I
would much rather like to merge your patch with the existing one, as done
below.

Wei, can you please have a look at it and add your Signed-Off if you are
ok with it?

Incidentally, your patch also fixes a second problem: the same problem
would occur if security_inet_conn_request(sk, skb, req) failed; but it
was apparently not triggered so far; this is also avoided below.

If there is no disagreement, I will upload this to the test tree within
the next hour.

-----------------------> Patch v2 (in test tree) <----------------------
dccp: Socket support for feature negotiation and its initialisation

This provides feature-negotiation initialisation for both DCCP sockets and
DCCP request_sockets, to support feature negotiation during connection setup.

It also resolves a FIXME regarding the congestion control initialisation.

Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Acked-by: Ian McDonald <ian.mcdonald@jandi.co.nz>
---
 include/linux/dccp.h |    4 ++++
 net/dccp/dccp.h      |    3 ++-
 net/dccp/feat.c      |   19 +++++++++++++++++++
 net/dccp/feat.h      |    1 +
 net/dccp/input.c     |    2 --
 net/dccp/ipv4.c      |    3 ++-
 net/dccp/ipv6.c      |    8 +++++---
 net/dccp/minisocks.c |    7 ++++++-
 net/dccp/proto.c     |    1 +
 9 files changed, 40 insertions(+), 8 deletions(-)

--- a/net/dccp/ipv6.c
+++ b/net/dccp/ipv6.c
@@ -409,7 +409,11 @@ static int dccp_v6_conn_request(struct s
 	if (req = NULL)
 		goto drop;
 
-	dccp_reqsk_init(req, skb);
+	ireq6 = inet6_rsk(req);
+	ireq6->pktopts = NULL;
+
+	if (dccp_reqsk_init(req, dccp_sk(sk), skb))
+		goto drop_and_free;
 
 	dreq = dccp_rsk(req);
 	if (dccp_parse_options(sk, dreq, skb))
@@ -418,10 +422,8 @@ static int dccp_v6_conn_request(struct s
 	if (security_inet_conn_request(sk, skb, req))
 		goto drop_and_free;
 
-	ireq6 = inet6_rsk(req);
 	ipv6_addr_copy(&ireq6->rmt_addr, &ipv6_hdr(skb)->saddr);
 	ipv6_addr_copy(&ireq6->loc_addr, &ipv6_hdr(skb)->daddr);
-	ireq6->pktopts	= NULL;
 
 	if (ipv6_opt_accepted(sk, skb) ||
 	    np->rxopt.bits.rxinfo || np->rxopt.bits.rxoinfo ||
--- a/net/dccp/input.c
+++ b/net/dccp/input.c
@@ -590,8 +590,6 @@ int dccp_rcv_state_process(struct sock *
 			if (inet_csk(sk)->icsk_af_ops->conn_request(sk,
 								    skb) < 0)
 				return 1;
-
-			/* FIXME: do congestion control initialization */
 			goto discard;
 		}
 		if (dh->dccph_type = DCCP_PKT_RESET)
--- a/include/linux/dccp.h
+++ b/include/linux/dccp.h
@@ -416,6 +416,7 @@ extern void dccp_minisock_init(struct dc
  * @dreq_iss: initial sequence number sent on the Response (RFC 4340, 7.1)
  * @dreq_isr: initial sequence number received on the Request
  * @dreq_service: service code present on the Request (there is just one)
+ * @dreq_featneg: feature negotiation options for this connection
  * The following two fields are analogous to the ones in dccp_sock:
  * @dreq_timestamp_echo: last received timestamp to echo (13.1)
  * @dreq_timestamp_echo: the time of receiving the last @dreq_timestamp_echo
@@ -425,6 +426,7 @@ struct dccp_request_sock {
 	__u64			 dreq_iss;
 	__u64			 dreq_isr;
 	__be32			 dreq_service;
+	struct list_head	 dreq_featneg;
 	__u32			 dreq_timestamp_echo;
 	__u32			 dreq_timestamp_time;
 };
@@ -502,6 +504,7 @@ struct dccp_ackvec;
  * @dccps_mss_cache - current value of MSS (path MTU minus header sizes)
  * @dccps_rate_last - timestamp for rate-limiting DCCP-Sync (RFC 4340, 7.5.4)
  * @dccps_minisock - associated minisock (accessed via dccp_msk)
+ * @dccps_featneg - tracks feature-negotiation state (mostly during handshake)
  * @dccps_hc_rx_ackvec - rx half connection ack vector
  * @dccps_hc_rx_ccid - CCID used for the receiver (or receiving half-connection)
  * @dccps_hc_tx_ccid - CCID used for the sender (or sending half-connection)
@@ -539,6 +542,7 @@ struct dccp_sock {
 	unsigned long			dccps_ndp_count;
 	unsigned long			dccps_rate_last;
 	struct dccp_minisock		dccps_minisock;
+	struct list_head		dccps_featneg;
 	struct dccp_ackvec		*dccps_hc_rx_ackvec;
 	struct ccid			*dccps_hc_rx_ccid;
 	struct ccid			*dccps_hc_tx_ccid;
--- a/net/dccp/proto.c
+++ b/net/dccp/proto.c
@@ -193,6 +193,7 @@ int dccp_init_sock(struct sock *sk, cons
 
 	dccp_init_xmit_timers(sk);
 
+	INIT_LIST_HEAD(&dp->dccps_featneg);
 	/*
 	 * FIXME: We're hardcoding the CCID, and doing this at this point makes
 	 * the listening (master) sock get CCID control blocks, which is not
--- a/net/dccp/feat.h
+++ b/net/dccp/feat.h
@@ -95,6 +95,7 @@ extern int  dccp_feat_confirm_recv(struc
 				   u8 *val, u8 len);
 extern void dccp_feat_clean(struct dccp_minisock *dmsk);
 extern int  dccp_feat_clone(struct sock *oldsk, struct sock *newsk);
+extern int  dccp_feat_clone_list(struct list_head const *, struct list_head *);
 extern int  dccp_feat_init(struct dccp_minisock *dmsk);
 
 #endif /* _DCCP_FEAT_H */
--- a/net/dccp/feat.c
+++ b/net/dccp/feat.c
@@ -272,6 +272,25 @@ void dccp_feat_list_purge(struct list_he
 }
 EXPORT_SYMBOL_GPL(dccp_feat_list_purge);
 
+/* generate @to as full clone of @from - @to must not contain any nodes */
+int dccp_feat_clone_list(struct list_head const *from, struct list_head *to)
+{
+	struct dccp_feat_entry *entry, *new;
+
+	INIT_LIST_HEAD(to);
+	list_for_each_entry(entry, from, node) {
+		new = dccp_feat_clone_entry(entry);
+		if (new = NULL)
+			goto cloning_failed;
+		list_add_tail(&new->node, to);
+	}
+	return 0;
+
+cloning_failed:
+	dccp_feat_list_purge(to);
+	return -ENOMEM;
+}
+
 int dccp_feat_change(struct dccp_minisock *dmsk, u8 type, u8 feature,
 		     u8 *val, u8 len, gfp_t gfp)
 {
--- a/net/dccp/dccp.h
+++ b/net/dccp/dccp.h
@@ -236,7 +236,8 @@ extern const char *dccp_state_name(const
 extern void dccp_set_state(struct sock *sk, const int state);
 extern void dccp_done(struct sock *sk);
 
-extern void dccp_reqsk_init(struct request_sock *req, struct sk_buff *skb);
+extern int  dccp_reqsk_init(struct request_sock *rq, struct dccp_sock const *dp,
+			    struct sk_buff const *skb);
 
 extern int dccp_v4_conn_request(struct sock *sk, struct sk_buff *skb);
 
--- a/net/dccp/minisocks.c
+++ b/net/dccp/minisocks.c
@@ -125,6 +125,7 @@ struct sock *dccp_create_openreq_child(s
 		newdp->dccps_timestamp_time = dreq->dreq_timestamp_time;
 		newicsk->icsk_rto	    = DCCP_TIMEOUT_INIT;
 
+		INIT_LIST_HEAD(&newdp->dccps_featneg);
 		if (dccp_feat_clone(sk, newsk))
 			goto out_free;
 
@@ -303,7 +304,8 @@ void dccp_reqsk_send_ack(struct sk_buff 
 
 EXPORT_SYMBOL_GPL(dccp_reqsk_send_ack);
 
-void dccp_reqsk_init(struct request_sock *req, struct sk_buff *skb)
+int dccp_reqsk_init(struct request_sock *req,
+		    struct dccp_sock const *dp, struct sk_buff const *skb)
 {
 	struct dccp_request_sock *dreq = dccp_rsk(req);
 
@@ -311,6 +313,9 @@ void dccp_reqsk_init(struct request_sock
 	inet_rsk(req)->acked	  = 0;
 	req->rcv_wnd		  = sysctl_dccp_feat_sequence_window;
 	dreq->dreq_timestamp_echo = 0;
+
+	/* inherit feature negotiation options from listening socket */
+	return dccp_feat_clone_list(&dp->dccps_featneg, &dreq->dreq_featneg);
 }
 
 EXPORT_SYMBOL_GPL(dccp_reqsk_init);
--- a/net/dccp/ipv4.c
+++ b/net/dccp/ipv4.c
@@ -593,7 +593,8 @@ int dccp_v4_conn_request(struct sock *sk
 	if (req = NULL)
 		goto drop;
 
-	dccp_reqsk_init(req, skb);
+	if (dccp_reqsk_init(req, dccp_sk(sk), skb))
+		goto drop_and_free;
 
 	dreq = dccp_rsk(req);
 	if (dccp_parse_options(sk, dreq, skb))

Re: [PATCH] DCCP: Initialize ireq6->pktopts before used it

[Wei Yongjun]

Gerrit Renker wrote:
>> I think I should add dccp_v6_reqsk_init() do to the init work since  
>> dccp_reqsk_init may be fail too.
>>
>>     
> I think this is not necessary, it can be done like this
>
>
> 	req = inet6_reqsk_alloc(&dccp6_request_sock_ops);
> 	if (req = NULL)
> 		goto drop;
>
> 	ireq6 = inet6_rsk(req);
> 	ireq6->pktopts = NULL;
>
> 	if (dccp_reqsk_init(req, dccp_sk(sk), skb))
> 		goto drop_and_free;
>
> This is since dccp_reqsk_init() only initialises the inet_sk and dccp_sk
> parts, and does not do IPv6-specific initialisation.
>
> Irrespective of the oops, this is an error and will be fixed in the test
> tree today. 
>
> With regard to the oops, the log pointed to the dccp_v6_reqsk_destructor
> and so it would make sense, since the pktopts was not initialised to NULL 
> and since kfree_skb() calls skb->destructor().
>
> As before, thanks a lot for testing this code and for reporting this.
>
>   
Hi Gerrit:
  IPv4 may have the same problem althought it is hard to test it.

[PATCH] DCCP: Initialize inet_rsk(req)->opt before used it
dccp_reqsk_init() does not initialises the inet_rsk(req)->opt which will 
be used in dccp_v4_reqsk_destructor().

This patch fixed this problem.

--- a/net/dccp/minisocks.c	2008-05-29 22:27:56.000000000 -0400
+++ b/net/dccp/minisocks.c	2008-06-05 06:36:59.000000000 -0400
@@ -266,6 +266,7 @@ int dccp_reqsk_init(struct request_sock 
 
 	inet_rsk(req)->rmt_port	  = dccp_hdr(skb)->dccph_sport;
 	inet_rsk(req)->acked	  = 0;
+	inet_rsk(req)->opt	  = NULL;
 	dreq->dreq_timestamp_echo = 0;
 
 	/* inherit feature negotiation options from listening socket */
--- a/net/dccp/ipv4.c	2008-05-29 22:27:56.000000000 -0400
+++ b/net/dccp/ipv4.c	2008-06-05 06:41:48.000000000 -0400
@@ -607,7 +607,6 @@ int dccp_v4_conn_request(struct sock *sk
 	ireq = inet_rsk(req);
 	ireq->loc_addr = ip_hdr(skb)->daddr;
 	ireq->rmt_addr = ip_hdr(skb)->saddr;
-	ireq->opt	= NULL;
 
 	/*
 	 * Step 3: Process LISTEN state

Re: [PATCH] DCCP: Initialize ireq6->pktopts before used it

[Wei Yongjun]

Gerrit Renker wrote:
> I have dug out the patch to which this applies. Since it is a bug-fix, I
> would much rather like to merge your patch with the existing one, as done
> below.
>
> Wei, can you please have a look at it and add your Signed-Off if you are
> ok with it?
>   

Comment inline.
> Incidentally, your patch also fixes a second problem: the same problem
> would occur if security_inet_conn_request(sk, skb, req) failed; but it
> was apparently not triggered so far; this is also avoided below.
>
> If there is no disagreement, I will upload this to the test tree within
> the next hour.
>
> -----------------------> Patch v2 (in test tree) <----------------------
> dccp: Socket support for feature negotiation and its initialisation
>
> This provides feature-negotiation initialisation for both DCCP sockets and
> DCCP request_sockets, to support feature negotiation during connection setup.
>
> It also resolves a FIXME regarding the congestion control initialisation.
>
> Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
> Acked-by: Ian McDonald <ian.mcdonald@jandi.co.nz>
> ---
>  include/linux/dccp.h |    4 ++++
>  net/dccp/dccp.h      |    3 ++-
>  net/dccp/feat.c      |   19 +++++++++++++++++++
>  net/dccp/feat.h      |    1 +
>  net/dccp/input.c     |    2 --
>  net/dccp/ipv4.c      |    3 ++-
>  net/dccp/ipv6.c      |    8 +++++---
>  net/dccp/minisocks.c |    7 ++++++-
>  net/dccp/proto.c     |    1 +
>  9 files changed, 40 insertions(+), 8 deletions(-)
>
> --- a/net/dccp/ipv6.c
> +++ b/net/dccp/ipv6.c
> @@ -409,7 +409,11 @@ static int dccp_v6_conn_request(struct s
>  	if (req = NULL)
>  		goto drop;
>  
> -	dccp_reqsk_init(req, skb);
> +	ireq6 = inet6_rsk(req);
> +	ireq6->pktopts = NULL;
> +
> +	if (dccp_reqsk_init(req, dccp_sk(sk), skb))
> +		goto drop_and_free;
>  
>  	dreq = dccp_rsk(req);
>  	if (dccp_parse_options(sk, dreq, skb))
> @@ -418,10 +422,8 @@ static int dccp_v6_conn_request(struct s
>  	if (security_inet_conn_request(sk, skb, req))
>  		goto drop_and_free;
>  
> -	ireq6 = inet6_rsk(req);
>  	ipv6_addr_copy(&ireq6->rmt_addr, &ipv6_hdr(skb)->saddr);
>  	ipv6_addr_copy(&ireq6->loc_addr, &ipv6_hdr(skb)->daddr);
> -	ireq6->pktopts	= NULL;
>  
>  	if (ipv6_opt_accepted(sk, skb) ||
>  	    np->rxopt.bits.rxinfo || np->rxopt.bits.rxoinfo ||
> --- a/net/dccp/input.c
> +++ b/net/dccp/input.c
> @@ -590,8 +590,6 @@ int dccp_rcv_state_process(struct sock *
>  			if (inet_csk(sk)->icsk_af_ops->conn_request(sk,
>  								    skb) < 0)
>  				return 1;
> -
> -			/* FIXME: do congestion control initialization */
>  			goto discard;
>  		}
>  		if (dh->dccph_type = DCCP_PKT_RESET)
> --- a/include/linux/dccp.h
> +++ b/include/linux/dccp.h
> @@ -416,6 +416,7 @@ extern void dccp_minisock_init(struct dc
>   * @dreq_iss: initial sequence number sent on the Response (RFC 4340, 7.1)
>   * @dreq_isr: initial sequence number received on the Request
>   * @dreq_service: service code present on the Request (there is just one)
> + * @dreq_featneg: feature negotiation options for this connection
>   * The following two fields are analogous to the ones in dccp_sock:
>   * @dreq_timestamp_echo: last received timestamp to echo (13.1)
>   * @dreq_timestamp_echo: the time of receiving the last @dreq_timestamp_echo
> @@ -425,6 +426,7 @@ struct dccp_request_sock {
>  	__u64			 dreq_iss;
>  	__u64			 dreq_isr;
>  	__be32			 dreq_service;
> +	struct list_head	 dreq_featneg;
>  	__u32			 dreq_timestamp_echo;
>  	__u32			 dreq_timestamp_time;
>  };
> @@ -502,6 +504,7 @@ struct dccp_ackvec;
>   * @dccps_mss_cache - current value of MSS (path MTU minus header sizes)
>   * @dccps_rate_last - timestamp for rate-limiting DCCP-Sync (RFC 4340, 7.5.4)
>   * @dccps_minisock - associated minisock (accessed via dccp_msk)
> + * @dccps_featneg - tracks feature-negotiation state (mostly during handshake)
>   * @dccps_hc_rx_ackvec - rx half connection ack vector
>   * @dccps_hc_rx_ccid - CCID used for the receiver (or receiving half-connection)
>   * @dccps_hc_tx_ccid - CCID used for the sender (or sending half-connection)
> @@ -539,6 +542,7 @@ struct dccp_sock {
>  	unsigned long			dccps_ndp_count;
>  	unsigned long			dccps_rate_last;
>  	struct dccp_minisock		dccps_minisock;
> +	struct list_head		dccps_featneg;
>  	struct dccp_ackvec		*dccps_hc_rx_ackvec;
>  	struct ccid			*dccps_hc_rx_ccid;
>  	struct ccid			*dccps_hc_tx_ccid;
> --- a/net/dccp/proto.c
> +++ b/net/dccp/proto.c
> @@ -193,6 +193,7 @@ int dccp_init_sock(struct sock *sk, cons
>  
>  	dccp_init_xmit_timers(sk);
>  
> +	INIT_LIST_HEAD(&dp->dccps_featneg);
>  	/*
>  	 * FIXME: We're hardcoding the CCID, and doing this at this point makes
>  	 * the listening (master) sock get CCID control blocks, which is not
> --- a/net/dccp/feat.h
> +++ b/net/dccp/feat.h
> @@ -95,6 +95,7 @@ extern int  dccp_feat_confirm_recv(struc
>  				   u8 *val, u8 len);
>  extern void dccp_feat_clean(struct dccp_minisock *dmsk);
>  extern int  dccp_feat_clone(struct sock *oldsk, struct sock *newsk);
> +extern int  dccp_feat_clone_list(struct list_head const *, struct list_head *);
>  extern int  dccp_feat_init(struct dccp_minisock *dmsk);
>  
>  #endif /* _DCCP_FEAT_H */
> --- a/net/dccp/feat.c
> +++ b/net/dccp/feat.c
> @@ -272,6 +272,25 @@ void dccp_feat_list_purge(struct list_he
>  }
>  EXPORT_SYMBOL_GPL(dccp_feat_list_purge);
>  
> +/* generate @to as full clone of @from - @to must not contain any nodes */
> +int dccp_feat_clone_list(struct list_head const *from, struct list_head *to)
> +{
> +	struct dccp_feat_entry *entry, *new;
> +
> +	INIT_LIST_HEAD(to);
> +	list_for_each_entry(entry, from, node) {
> +		new = dccp_feat_clone_entry(entry);
> +		if (new = NULL)
> +			goto cloning_failed;
> +		list_add_tail(&new->node, to);
> +	}
> +	return 0;
> +
> +cloning_failed:
> +	dccp_feat_list_purge(to);
> +	return -ENOMEM;
> +}
> +
>  int dccp_feat_change(struct dccp_minisock *dmsk, u8 type, u8 feature,
>  		     u8 *val, u8 len, gfp_t gfp)
>  {
> --- a/net/dccp/dccp.h
> +++ b/net/dccp/dccp.h
> @@ -236,7 +236,8 @@ extern const char *dccp_state_name(const
>  extern void dccp_set_state(struct sock *sk, const int state);
>  extern void dccp_done(struct sock *sk);
>  
> -extern void dccp_reqsk_init(struct request_sock *req, struct sk_buff *skb);
> +extern int  dccp_reqsk_init(struct request_sock *rq, struct dccp_sock const *dp,
> +			    struct sk_buff const *skb);
>  
>  extern int dccp_v4_conn_request(struct sock *sk, struct sk_buff *skb);
>  
> --- a/net/dccp/minisocks.c
> +++ b/net/dccp/minisocks.c
> @@ -125,6 +125,7 @@ struct sock *dccp_create_openreq_child(s
>  		newdp->dccps_timestamp_time = dreq->dreq_timestamp_time;
>  		newicsk->icsk_rto	    = DCCP_TIMEOUT_INIT;
>  
> +		INIT_LIST_HEAD(&newdp->dccps_featneg);
>  		if (dccp_feat_clone(sk, newsk))
>  			goto out_free;
>  
> @@ -303,7 +304,8 @@ void dccp_reqsk_send_ack(struct sk_buff 
>  
>  EXPORT_SYMBOL_GPL(dccp_reqsk_send_ack);
>  
> -void dccp_reqsk_init(struct request_sock *req, struct sk_buff *skb)
> +int dccp_reqsk_init(struct request_sock *req,
> +		    struct dccp_sock const *dp, struct sk_buff const *skb)
>  {
>  	struct dccp_request_sock *dreq = dccp_rsk(req);
>  
> @@ -311,6 +313,9 @@ void dccp_reqsk_init(struct request_sock
>  	inet_rsk(req)->acked	  = 0;
>  	req->rcv_wnd		  = sysctl_dccp_feat_sequence_window;
>  	dreq->dreq_timestamp_echo = 0;
>   
inet_rsk(req)->opt may have the same problem. You should modify this to 
handle IPv4. Please ignore the patch about IPv4 I send, thanks.

> +
> +	/* inherit feature negotiation options from listening socket */
> +	return dccp_feat_clone_list(&dp->dccps_featneg, &dreq->dreq_featneg);
>  }
>  
>  EXPORT_SYMBOL_GPL(dccp_reqsk_init);
> --- a/net/dccp/ipv4.c
> +++ b/net/dccp/ipv4.c
> @@ -593,7 +593,8 @@ int dccp_v4_conn_request(struct sock *sk
>  	if (req = NULL)
>  		goto drop;
>  
> -	dccp_reqsk_init(req, skb);
> +	if (dccp_reqsk_init(req, dccp_sk(sk), skb))
> +		goto drop_and_free;
>  
>  	dreq = dccp_rsk(req);
>  	if (dccp_parse_options(sk, dreq, skb))
>
>
>   

Re: [PATCH] DCCP: Initialize ireq6->pktopts before used it

[Gerrit Renker]

>  IPv4 may have the same problem althought it is hard to test it.
>
> [PATCH] DCCP: Initialize inet_rsk(req)->opt before used it
> dccp_reqsk_init() does not initialises the inet_rsk(req)->opt which will  
> be used in dccp_v4_reqsk_destructor().
>
> This patch fixed this problem.
>
Yes, correct - and it is the same problem, the dccp_v4_reqsk_destructor()
ends up trying to kfree the `opt' area.

Since this fixes also a problem for the mainline, I would like to add it to
the set of bug-fixes I am about to submit. Please check if you are ok with 
the combined patch as below, I will submit this later on today and the
fix will also be available in the test tree.

Really, thanks heaps for all your helpful work.

------------------------> Patch <---------------------------------------
dccp: Initialise option area of v4/v6 request socket

From: Wei Yongjun <yjwei@cn.fujitsu.com>

This fixes the following bug:
 * dccp_v4_reqsk_destructor() frees inet inet_rsk(req)->opt,
 * but dccp_v4_conn_request() may fail before initialising inet_rsk(req)->opt;
 * likewise, dccp_v6_reqsk_destructor() frees inet6_rsk(req)->pktopts,
 * but dccp_v6_conn_request() may fail before initialising the pktopts.

The fix is in initialising the option areas in both request sockets before
calling any other code that may fail and thus may end up calling the destructor.

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 net/dccp/ipv4.c |    5 +++--
 net/dccp/ipv6.c |    5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

--- a/net/dccp/ipv4.c
+++ b/net/dccp/ipv4.c
@@ -593,6 +593,9 @@ int dccp_v4_conn_request(struct sock *sk
 	if (req = NULL)
 		goto drop;
 
+	ireq = inet_rsk(req);
+	ireq->opt = NULL;
+
 	dccp_reqsk_init(req, skb);
 
 	dreq = dccp_rsk(req);
@@ -602,10 +605,8 @@ int dccp_v4_conn_request(struct sock *sk
 	if (security_inet_conn_request(sk, skb, req))
 		goto drop_and_free;
 
-	ireq = inet_rsk(req);
 	ireq->loc_addr = ip_hdr(skb)->daddr;
 	ireq->rmt_addr = ip_hdr(skb)->saddr;
-	ireq->opt	= NULL;
 
 	/*
 	 * Step 3: Process LISTEN state
--- a/net/dccp/ipv6.c
+++ b/net/dccp/ipv6.c
@@ -409,6 +409,9 @@ static int dccp_v6_conn_request(struct s
 	if (req = NULL)
 		goto drop;
 
+	ireq6 = inet6_rsk(req);
+	ireq6->pktopts = NULL;
+
 	dccp_reqsk_init(req, skb);
 
 	dreq = dccp_rsk(req);
@@ -418,10 +421,8 @@ static int dccp_v6_conn_request(struct s
 	if (security_inet_conn_request(sk, skb, req))
 		goto drop_and_free;
 
-	ireq6 = inet6_rsk(req);
 	ipv6_addr_copy(&ireq6->rmt_addr, &ipv6_hdr(skb)->saddr);
 	ipv6_addr_copy(&ireq6->loc_addr, &ipv6_hdr(skb)->daddr);
-	ireq6->pktopts	= NULL;
 
 	if (ipv6_opt_accepted(sk, skb) ||
 	    np->rxopt.bits.rxinfo || np->rxopt.bits.rxoinfo ||