netfilter: nf_conntrack_sctp: fix build warning

PATCH David Miller 4 participants 8 messages Nov 23, 2008 – Nov 24, 2008 1 message not in the archive

Subsystems (3)

Networking Network drivers netfilter

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): 20081123113315.GA16697@localhost

David Miller

author maintainer

2008-11-23 21:59 UTC #1 lore raw

From: Wu Fengguang <[email protected]>
Date: Sun, 23 Nov 2008 19:33:15 +0800

linux-net is not the mailing list for network development discussion, it is for user questions. Post patches and developer questions to [email protected] instead.

Quoted 26 lines

net/netfilter/nf_conntrack_proto_sctp.c: In function ‘sctp_packet’:
net/netfilter/nf_conntrack_proto_sctp.c:376: warning: array subscript is above array bounds
Signed-off-by Wu Fengguang <[email protected]>
---
DISCLAIMER: I'm newbie to the code, and this fix could be wrong!
net/netfilter/nf_conntrack_proto_sctp.c +3 -1
index ae8c260..d31ced4 100644
@@ -373,6 +373,9 @@ static int sctp_packet(struct nf_conn *ct,
 	}
 	write_unlock_bh(&sctp_lock);
 
+	if (new_state == SCTP_CONNTRACK_MAX)
+		goto out;
+
 	nf_ct_refresh_acct(ct, ctinfo, skb, sctp_timeouts[new_state]);
 
 	if (old_state == SCTP_CONNTRACK_COOKIE_ECHOED &&
--
To unsubscribe from this list: send the line "unsubscribe linux-net" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Wu Fengguang

2008-11-24 00:07 UTC #2 lore raw

Quoted 6 lines · David Miller

From: Wu Fengguang <[email protected]>
Date: Sun, 23 Nov 2008 19:33:15 +0800
linux-net is not the mailing list for network development discussion, it is for user questions. Post patches and developer questions to [email protected] instead.

OK, thanks for the CC!

Fengguang
---

net/netfilter/nf_conntrack_proto_sctp.c: In function ‘sctp_packet’:
net/netfilter/nf_conntrack_proto_sctp.c:376: warning: array subscript is above array bounds

Signed-off-by Wu Fengguang <[email protected]>

---

DISCLAIMER: I'm newbie to the code, and this fix could be wrong!

net/netfilter/nf_conntrack_proto_sctp.c +3 -0

index ae8c260..d31ced4 100644
@@ -373,6 +373,9 @@ static int sctp_packet(struct nf_conn *ct,
 	}
 	write_unlock_bh(&sctp_lock);
 
+	if (new_state == SCTP_CONNTRACK_MAX)
+		goto out;
+
 	nf_ct_refresh_acct(ct, ctinfo, skb, sctp_timeouts[new_state]);
 
 	if (old_state == SCTP_CONNTRACK_COOKIE_ECHOED &&

Wu Fengguang

2008-11-24 01:17 UTC #4 lore raw

Quoted 30 lines · Wang Chen

Wu Fengguang said the following on 2008-11-24 8:06:
Quoted 29 lines · David Miller
Quoted 6 lines
From: Wu Fengguang <[email protected]>
Date: Sun, 23 Nov 2008 19:33:15 +0800
linux-net is not the mailing list for network development discussion, it is for user questions. Post patches and developer questions to [email protected] instead.
OK, thanks for the CC!
Fengguang
---
net/netfilter/nf_conntrack_proto_sctp.c: In function ‘sctp_packet’:
net/netfilter/nf_conntrack_proto_sctp.c:376: warning: array subscript is above array bounds
Signed-off-by Wu Fengguang <[email protected]>
---
DISCLAIMER: I'm newbie to the code, and this fix could be wrong!
net/netfilter/nf_conntrack_proto_sctp.c +0 -0
index ae8c260..d31ced4 100644
@@ -373,6 +373,9 @@ static int sctp_packet(struct nf_conn *ct,
 	}
 	write_unlock_bh(&sctp_lock);
 

Quoted 5 lines

net/netfilter/nf_conntrack_proto_sctp.c static int sctp_packet(struct nf_conn *ct, · line 378
Quoted 3 lines
net/netfilter/nf_conntrack_proto_sctp.c +3 -0
+	if (new_state == SCTP_CONNTRACK_MAX)
+		goto out;
+
print some debug info would be better?

Like this one?
+ pr_debug("Empty sctp packet\n");

Fengguang
---
netfilter: nf_conntrack_sctp: fix build warning

net/netfilter/nf_conntrack_proto_sctp.c: In function ‘sctp_packet’:
net/netfilter/nf_conntrack_proto_sctp.c:376: warning: array subscript is above array bounds

Signed-off-by Wu Fengguang <[email protected]>

---

DISCLAIMER: I'm newbie to the code, and this fix could be wrong!

net/netfilter/nf_conntrack_proto_sctp.c +5 -0

index ae8c260..a0bc24b 100644
@@ -373,6 +373,11 @@ static int sctp_packet(struct nf_conn *ct,
 	}
 	write_unlock_bh(&sctp_lock);
 
+	if (new_state == SCTP_CONNTRACK_MAX) {
+		pr_debug("Empty sctp packet\n");
+		goto out;
+	}
+
 	nf_ct_refresh_acct(ct, ctinfo, skb, sctp_timeouts[new_state]);
 
 	if (old_state == SCTP_CONNTRACK_COOKIE_ECHOED &&

Patrick McHardy

2008-11-24 12:23 UTC #5 lore raw

Quoted 11 lines · Wu Fengguang

Quoted 1 line
print some debug info would be better?
Like this one?
(quoted diff) +1 -0
+		pr_debug("Empty sctp packet\n");
Fengguang
---
netfilter: nf_conntrack_sctp: fix build warning
net/netfilter/nf_conntrack_proto_sctp.c: In function ‘sctp_packet’:
net/netfilter/nf_conntrack_proto_sctp.c:376: warning: array subscript is above array bounds

The warning is bogus, so we don't need a pr_debug() there. I've applied your first patch with an unlikely() added and a comment stating that the warning is bogus.

commit a3e2913ffd3dec7f6975f680035670261ad5f56e

Author: Wu Fengguang <[email protected]>
Date:   Mon Nov 24 13:18:00 2008 +0100

netfilter: nf_conntrack_sctp: fix build warning

net/netfilter/nf_conntrack_proto_sctp.c: In function 'sctp_packet':
net/netfilter/nf_conntrack_proto_sctp.c:376: warning: array subscript is above array bounds

[Patrick; add unlikely and comment stating that the warning is bogus]

Signed-off-by Wu Fengguang <[email protected]>

Signed-off-by Patrick McHardy <[email protected]>

net/netfilter/nf_conntrack_proto_sctp.c +6 -0

index c2bd457..1259ec6 100644
@@ -373,6 +373,12 @@ static int sctp_packet(struct nf_conn *ct,
 	}
 	write_unlock_bh(&sctp_lock);
 
+	/* Avoid bogus warning, gcc doesn't realize do_basic_checks()
+	 * guarantees that there is at least one SCTP chunk.
+	 */
+	if (unlikely(new_state == SCTP_CONNTRACK_MAX))
+		goto out;
+
 	nf_ct_refresh_acct(ct, ctinfo, skb, sctp_timeouts[new_state]);
 
 	if (old_state == SCTP_CONNTRACK_COOKIE_ECHOED &&

Wu Fengguang

2008-11-24 12:35 UTC #6 lore raw

Quoted 10 lines · Patrick McHardy

Quoted 9 lines · Wu Fengguang
Quoted 1 line
print some debug info would be better?
Like this one?
(quoted diff) +1 -0
+		pr_debug("Empty sctp packet\n");
Fengguang
---
netfilter: nf_conntrack_sctp: fix build warning

Quoted 6 lines

Quoted 2 lines
net/netfilter/nf_conntrack_proto_sctp.c: In function ‘sctp_packet’:
net/netfilter/nf_conntrack_proto_sctp.c:376: warning: array subscript is above array bounds
The warning is bogus, so we don't need a pr_debug() there. I've applied your first patch with an unlikely() added and a comment stating that the warning is bogus.

Thank you! I was expecting some better solution (and learn a bit) ;-)

Quoted 19 lines

commit a3e2913ffd3dec7f6975f680035670261ad5f56e
Author: Wu Fengguang <[email protected]>
Date:   Mon Nov 24 13:18:00 2008 +0100
netfilter: nf_conntrack_sctp: fix build warning
net/netfilter/nf_conntrack_proto_sctp.c: In function 'sctp_packet':
net/netfilter/nf_conntrack_proto_sctp.c:376: warning: array subscript is above array bounds
[Patrick; add unlikely and comment stating that the warning is bogus]
Signed-off-by Wu Fengguang <[email protected]>
Signed-off-by Patrick McHardy <[email protected]>
net/netfilter/nf_conntrack_proto_sctp.c +0 -0
index c2bd457..1259ec6 100644

net/netfilter/nf_conntrack_proto_sctp.c static int sctp_packet(struct nf_conn *ct, · line 381

Quoted 9 lines

net/netfilter/nf_conntrack_proto_sctp.c +6 -0

@@ -373,6 +373,12 @@ static int sctp_packet(struct nf_conn *ct,
 	}
 	write_unlock_bh(&sctp_lock);
 
+	/* Avoid bogus warning, gcc doesn't realize do_basic_checks()
+	 * guarantees that there is at least one SCTP chunk.
+	 */
+	if (unlikely(new_state == SCTP_CONNTRACK_MAX))
+		goto out;
+

If do_basic_checks() guarantees that, why not Initialize new_state to 0?

---

net/netfilter/nf_conntrack_proto_sctp.c +5 -1

index ae8c260..218137d 100644
@@ -317,7 +317,11 @@ static int sctp_packet(struct nf_conn *ct,
 		goto out;
 	}
 
-	old_state = new_state = SCTP_CONNTRACK_MAX;
+	/* Avoid bogus warning, gcc doesn't realize do_basic_checks()
+	 * guarantees that there is at least one SCTP chunk.
+	 */
+	old_state = new_state = 0;
+
 	write_lock_bh(&sctp_lock);
 	for_each_sctp_chunk (skb, sch, _sch, offset, dataoff, count) {
 		/* Special cases of Verification tag check (Sec 8.5.1) */

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Patrick McHardy

2008-11-24 12:48 UTC #7 lore raw

Quoted 9 lines · Wu Fengguang

Review
Quoted 6 lines · Patrick McHardy
(quoted diff) +6 -0
+	/* Avoid bogus warning, gcc doesn't realize do_basic_checks()
+	 * guarantees that there is at least one SCTP chunk.
+	 */
+	if (unlikely(new_state == SCTP_CONNTRACK_MAX))
+		goto out;
+
If do_basic_checks() guarantees that, why not Initialize new_state to 0?

Good point. I've replaced the patch by this one:

commit 328bd8997dbb7184d5389e45c642af44ae6e9043

Author: Patrick McHardy <[email protected]>
Date:   Mon Nov 24 13:44:55 2008 +0100

netfilter: nf_conntrack_proto_sctp: avoid bogus warning

net/netfilter/nf_conntrack_proto_sctp.c: In function 'sctp_packet':
net/netfilter/nf_conntrack_proto_sctp.c:376: warning: array subscript is above array bounds

gcc doesn't realize that do_basic_checks() guarantees that there is at least one valid chunk and thus new_state is never SCTP_CONNTRACK_MAX after the loop. Initialize to SCTP_CONNTRACK_NONE to avoid the warning.

Based on patch by Wu Fengguang <[email protected]>

Signed-off-by Patrick McHardy <[email protected]>

net/netfilter/nf_conntrack_proto_sctp.c +1 -1

index c2bd457..74e0379 100644
@@ -317,7 +317,7 @@ static int sctp_packet(struct nf_conn *ct,
 		goto out;
 	}
 
-	old_state = new_state = SCTP_CONNTRACK_MAX;
+	old_state = new_state = SCTP_CONNTRACK_NONE;
 	write_lock_bh(&sctp_lock);
 	for_each_sctp_chunk (skb, sch, _sch, offset, dataoff, count) {
 		/* Special cases of Verification tag check (Sec 8.5.1) */

Wu Fengguang

2008-11-24 12:50 UTC #8 lore raw

Quoted 11 lines · Patrick McHardy

Quoted 9 lines · Wu Fengguang
Review
Quoted 6 lines · Patrick McHardy
(quoted diff) +6 -0
+	/* Avoid bogus warning, gcc doesn't realize do_basic_checks()
+	 * guarantees that there is at least one SCTP chunk.
+	 */
+	if (unlikely(new_state == SCTP_CONNTRACK_MAX))
+		goto out;
+
If do_basic_checks() guarantees that, why not Initialize new_state to 0?

Quoted 1 line

Good point. I've replaced the patch by this one:

Thanks!

Fengguang

Content-Description: x

Quoted 30 lines

commit 328bd8997dbb7184d5389e45c642af44ae6e9043
Author: Patrick McHardy <[email protected]>
Date:   Mon Nov 24 13:44:55 2008 +0100
netfilter: nf_conntrack_proto_sctp: avoid bogus warning
net/netfilter/nf_conntrack_proto_sctp.c: In function 'sctp_packet':
net/netfilter/nf_conntrack_proto_sctp.c:376: warning: array subscript is above array bounds
gcc doesn't realize that do_basic_checks() guarantees that there is at least one valid chunk and thus new_state is never SCTP_CONNTRACK_MAX after the loop. Initialize to SCTP_CONNTRACK_NONE to avoid the warning.
Based on patch by Wu Fengguang <[email protected]>
Signed-off-by Patrick McHardy <[email protected]>
net/netfilter/nf_conntrack_proto_sctp.c +1 -1
index c2bd457..74e0379 100644
@@ -317,7 +317,7 @@ static int sctp_packet(struct nf_conn *ct,
 		goto out;
 	}
 
-	old_state = new_state = SCTP_CONNTRACK_MAX;
+	old_state = new_state = SCTP_CONNTRACK_NONE;
 	write_lock_bh(&sctp_lock);
 	for_each_sctp_chunk (skb, sch, _sch, offset, dataoff, count) {
 		/* Special cases of Verification tag check (Sec 8.5.1) */