[PATCH net] tcp: init tcp_options before using it.

I searched 4308fc58dced ("tcp: Document use of undefined variable") in
archive list, but did not find the thread. So I'm not sure why we only
add a description about un-initialized value.

Even we don't use tmp_opt.sack_ok, I think it would be more safe to
initialize the value before using it. Just as other caller did.

Signed-off-by: Hangbin Liu <redacted>
---
 net/ipv4/tcp_minisocks.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 717be4d..6ca2546 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c

@@ -85,8 +85,6 @@ tcp_timewait_check_oow_rate_limit(struct inet_timewait_sock *tw,
  * 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,7 +94,7 @@ tcp_timewait_state_process(struct inet_timewait_sock *tw, struct sk_buff *skb,
 	struct tcp_timewait_sock *tcptw = tcp_twsk((struct sock *)tw);
 	bool paws_reject = false;
 
-	tmp_opt.saw_tstamp = 0;
+	memset(&tmp_opt, 0, sizeof(tmp_opt));
 	if (th->doff > (sizeof(*th) >> 2) && tcptw->tw_ts_recent_stamp) {
 		tcp_parse_options(skb, &tmp_opt, 0, NULL);
 

@@ -542,8 +540,6 @@ 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,

@@ -557,7 +553,7 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
 	bool paws_reject = false;
 	bool own_req;
 
-	tmp_opt.saw_tstamp = 0;
+	memset(&tmp_opt, 0, sizeof(tmp_opt));
 	if (th->doff > (sizeof(struct tcphdr)>>2)) {
 		tcp_parse_options(skb, &tmp_opt, 0, NULL);
 

-- 
2.5.5

On Mon, 2017-05-08 at 17:57 +0800, Hangbin Liu wrote:

I searched 4308fc58dced ("tcp: Document use of undefined variable") in
archive list, but did not find the thread. So I'm not sure why we only
add a description about un-initialized value.

Even we don't use tmp_opt.sack_ok, I think it would be more safe to
initialize the value before using it. Just as other caller did.

Patch is not needed at all.

Comment and code are pretty clear.

This part of the code uses a generic function ( tcp_parse_options()) to
decode TCP options, but we are only caring about TS one.

On Mon, May 08, 2017 at 07:49:23AM -0700, Eric Dumazet wrote:

On Mon, 2017-05-08 at 17:57 +0800, Hangbin Liu wrote:

quoted

I searched 4308fc58dced ("tcp: Document use of undefined variable") in
archive list, but did not find the thread. So I'm not sure why we only
add a description about un-initialized value.

Even we don't use tmp_opt.sack_ok, I think it would be more safe to
initialize the value before using it. Just as other caller did.

Patch is not needed at all.

Comment and code are pretty clear.

This part of the code uses a generic function ( tcp_parse_options()) to
decode TCP options, but we are only caring about TS one.

OK, got it. Thanks for the explanation and sorry for the inconvenience.

Best Regards
Hangbin

On Tue, 2017-05-09 at 21:22 +0800, Hangbin Liu wrote:

On Mon, May 08, 2017 at 07:49:23AM -0700, Eric Dumazet wrote:

quoted

On Mon, 2017-05-08 at 17:57 +0800, Hangbin Liu wrote:

quoted

I searched 4308fc58dced ("tcp: Document use of undefined variable") in
archive list, but did not find the thread. So I'm not sure why we only
add a description about un-initialized value.

Even we don't use tmp_opt.sack_ok, I think it would be more safe to
initialize the value before using it. Just as other caller did.

Patch is not needed at all.

Comment and code are pretty clear.

This part of the code uses a generic function ( tcp_parse_options()) to
decode TCP options, but we are only caring about TS one.

OK, got it. Thanks for the explanation and sorry for the inconvenience.

No inconvenience taken ;)