[RFC PATCH] tcp: use of undefined variable

[RFC PATCH] tcp: use of undefined variable

[Alan Cox]

From: Alan Cox <alan@linux.intel.com>

Both tcp_timewait_state_process and tcp_check_req use the same basic
construct of

	struct tcp_options received tmp_opt;
	tmp_opt.saw_tstamp = 0;

then call

	tcp_parse_options

However if they are fed a frame containing a TCP_SACK then tbe code
behaviour is undefined because opt_rx->sack_ok is undefined data.

This ought to be documented if it is intentional.

Signed-off-by: Alan Cox <alan@linux.intel.com>
---

 net/ipv4/tcp_minisocks.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index e965319..a4ace80 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -85,6 +85,8 @@ static bool tcp_in_window(u32 seq, u32 end_seq, u32 s_win, u32 e_win)
  * spinlock it. I do not want! Well, probability of misbehaviour
  * is ridiculously low and, seems, we could use some mb() tricks
  * to avoid misread sequence numbers, states etc.  --ANK
+ *
+ * We don't need to initialize tmp_out.sack_ok as we don't use the results
  */
 enum tcp_tw_status
 tcp_timewait_state_process(struct inet_timewait_sock *tw, struct sk_buff *skb,
@@ -96,6 +98,7 @@ tcp_timewait_state_process(struct inet_timewait_sock *tw, struct sk_buff *skb,
 	bool paws_reject = false;
 
 	tmp_opt.saw_tstamp = 0;
+
 	if (th->doff > (sizeof(*th) >> 2) && tcptw->tw_ts_recent_stamp) {
 		tcp_parse_options(skb, &tmp_opt, &hash_location, 0, NULL);
 
@@ -522,6 +525,8 @@ EXPORT_SYMBOL(tcp_create_openreq_child);
  *
  * XXX (TFO) - The current impl contains a special check for ack
  * validation and inside tcp_v4_reqsk_send_ack(). Can we do better?
+ *
+ * We don't need to initialize tmp_opt.sack_ok as we don't use the results
  */
 
 struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,

Re: [RFC PATCH] tcp: use of undefined variable

[David Miller]

From: Alan Cox <alan@lxorguk.ukuu.org.uk>
Date: Wed, 19 Sep 2012 15:46:06 +0100

> From: Alan Cox <alan@linux.intel.com>
> 
> Both tcp_timewait_state_process and tcp_check_req use the same basic
> construct of
> 
> 	struct tcp_options received tmp_opt;
> 	tmp_opt.saw_tstamp = 0;
> 
> then call
> 
> 	tcp_parse_options
> 
> However if they are fed a frame containing a TCP_SACK then tbe code
> behaviour is undefined because opt_rx->sack_ok is undefined data.
> 
> This ought to be documented if it is intentional.
> 
> Signed-off-by: Alan Cox <alan@linux.intel.com>

Applied to net-next, except I took this hunk out:

> @@ -96,6 +98,7 @@ tcp_timewait_state_process(struct inet_timewait_sock *tw, struct sk_buff *skb,
>  	bool paws_reject = false;
>  
>  	tmp_opt.saw_tstamp = 0;
> +
>  	if (th->doff > (sizeof(*th) >> 2) && tcptw->tw_ts_recent_stamp) {
>  		tcp_parse_options(skb, &tmp_opt, &hash_location, 0, NULL);
>  

Since it's unrelated to your change, and if you were going to do this in
tcp_timewait_state_process() you should do it in tcp_check_req() as well
since the code is identical.

Longer term maybe we probably should add a
tcp_minisock_parse_options() that elides TCP_SACK and other bits these
cases do not want.

Thanks Alan.