* [Patch 0/7] [dccp]: DCCP bug-fixes for net-2.6
[not found] <dccp_bug_fixes_for_net-2.6>
@ 2008-06-10 12:53 ` Gerrit Renker
2008-06-10 12:53 ` [PATCH 1/7] dccp ccid-3: Bug-Fix - Zero RTT is possible Gerrit Renker
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Gerrit Renker @ 2008-06-10 12:53 UTC (permalink / raw)
To: davem; +Cc: dccp, netdev
Hi David,
please consider pulling from
git://eden-feed.erg.abdn.ac.uk/net-2.6
This set has bug-fixes only, non-bug fixes will follow at a later stage.
Some test-tree patches need to be reworked with regard to prefix conventions,
will send a request-for-comments later this week to enquire about this.
Patch #1: Fixes a divide-by-zero bug in CCID-3.
Patch #2: Resolves sparse warnings (gathered from several patches in the tree).
Patch #3: Enforces that Ack Vectors are not interpreted on request sockets.
Patch #4: Computation error in CCID-3 allowed sending rate.
Patch #5: Sending rate in CCID-3 truncated due to u64 -> u32 conversion.
Patch #6: Typo in initial sequence number assignment.
Patch #7: Bug in the initialisation of v4/v6 request-socket option areas.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/7] dccp ccid-3: Bug-Fix - Zero RTT is possible
2008-06-10 12:53 ` [Patch 0/7] [dccp]: DCCP bug-fixes for net-2.6 Gerrit Renker
@ 2008-06-10 12:53 ` Gerrit Renker
2008-06-10 12:53 ` [PATCH 2/7] dccp: Fix sparse warnings Gerrit Renker
2008-06-10 19:31 ` [PATCH 1/7] dccp ccid-3: Bug-Fix - Zero RTT is possible David Miller
2008-06-10 19:29 ` [Patch 0/7] [dccp]: DCCP bug-fixes for net-2.6 David Miller
2008-06-10 19:32 ` David Miller
2 siblings, 2 replies; 15+ messages in thread
From: Gerrit Renker @ 2008-06-10 12:53 UTC (permalink / raw)
To: davem; +Cc: dccp, netdev, Gerrit Renker
Commit 825de27d9e40b3117b29a79d412b7a4b78c5d815 fixed the CCID-3 window counter
computation for RTTs < 4 microseconds (as happens on loopback).
Since it uses the term "1/RTT", it needs to be protected against divide-by-zero,
done in established state using dccp_sample_rtt().
But there was an oversight, as a zero RTT can happen on sender initialisation
when there is no RTT sample from the Request/Response exchange.
The fix is to use the fallback-RTT from RFC 4340, 3.4.
This is also better than just fixing update_win_count() since it allows other
parts of the code to always assume that a (fallback) RTT value is available.
Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
net/dccp/ccids/ccid3.c | 10 ++++++++--
1 files changed, 8 insertions(+), 2 deletions(-)
--- a/net/dccp/ccids/ccid3.c
+++ b/net/dccp/ccids/ccid3.c
@@ -329,8 +329,14 @@ static int ccid3_hc_tx_send_packet(struct sock *sk, struct sk_buff *skb)
hctx->ccid3hctx_x = rfc3390_initial_rate(sk);
hctx->ccid3hctx_t_ld = now;
} else {
- /* Sender does not have RTT sample: X_pps = 1 pkt/sec */
- hctx->ccid3hctx_x = hctx->ccid3hctx_s;
+ /*
+ * Sender does not have RTT sample:
+ * - set fallback RTT (RFC 4340, 3.4) since a RTT value
+ * is needed in several parts (e.g. window counter);
+ * - set sending rate X_pps = 1pps as per RFC 3448, 4.2.
+ */
+ hctx->ccid3hctx_rtt = DCCP_FALLBACK_RTT;
+ hctx->ccid3hctx_x = hctx->ccid3hctx_s;
hctx->ccid3hctx_x <<= 6;
}
ccid3_update_send_interval(hctx);
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/7] dccp: Fix sparse warnings
2008-06-10 12:53 ` [PATCH 1/7] dccp ccid-3: Bug-Fix - Zero RTT is possible Gerrit Renker
@ 2008-06-10 12:53 ` Gerrit Renker
2008-06-10 12:53 ` [PATCH 3/7] dccp ccid-2: Bug-Fix - Ack Vectors need to be ignored on request sockets Gerrit Renker
2008-06-10 19:31 ` [PATCH 1/7] dccp ccid-3: Bug-Fix - Zero RTT is possible David Miller
1 sibling, 1 reply; 15+ messages in thread
From: Gerrit Renker @ 2008-06-10 12:53 UTC (permalink / raw)
To: davem; +Cc: dccp, netdev, Gerrit Renker
This patch fixes the following sparse warnings:
* nested min(max()) expression:
net/dccp/ccids/ccid3.c:91:21: warning: symbol '__x' shadows an earlier one
net/dccp/ccids/ccid3.c:91:21: warning: symbol '__y' shadows an earlier one
* Declaration of function prototypes in .c instead of .h file, resulting in
"should it be static?" warnings.
* Declared "struct dccpw" static (local to dccp_probe).
* Disabled dccp_delayed_ack() - not fully removed due to RFC 4340, 11.3
("Receivers SHOULD implement delayed acknowledgement timers ...").
* Used a different local variable name to avoid
net/dccp/ackvec.c:293:13: warning: symbol 'state' shadows an earlier one
net/dccp/ackvec.c:238:33: originally declared here
* Removed unused functions `dccp_ackvector_print' and `dccp_ackvec_print'.
Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
net/dccp/ackvec.c | 29 ++---------------------------
net/dccp/ccids/ccid3.c | 4 ++--
net/dccp/ccids/lib/tfrc.c | 8 --------
net/dccp/ccids/lib/tfrc.h | 11 +++++++++--
net/dccp/output.c | 2 ++
net/dccp/probe.c | 2 +-
6 files changed, 16 insertions(+), 40 deletions(-)
--- a/net/dccp/ccids/ccid3.c
+++ b/net/dccp/ccids/ccid3.c
@@ -159,8 +159,8 @@ static void ccid3_hc_tx_update_x(struct sock *sk, ktime_t *stamp)
} else if (ktime_us_delta(now, hctx->ccid3hctx_t_ld)
- (s64)hctx->ccid3hctx_rtt >= 0) {
- hctx->ccid3hctx_x =
- max(min(2 * hctx->ccid3hctx_x, min_rate),
+ hctx->ccid3hctx_x = min(2 * hctx->ccid3hctx_x, min_rate);
+ hctx->ccid3hctx_x = max(hctx->ccid3hctx_x,
scaled_div(((__u64)hctx->ccid3hctx_s) << 6,
hctx->ccid3hctx_rtt));
hctx->ccid3hctx_t_ld = now;
--- a/net/dccp/probe.c
+++ b/net/dccp/probe.c
@@ -42,7 +42,7 @@ static int bufsize = 64 * 1024;
static const char procname[] = "dccpprobe";
-struct {
+static struct {
struct kfifo *fifo;
spinlock_t lock;
wait_queue_head_t wait;
--- a/net/dccp/ccids/lib/tfrc.c
+++ b/net/dccp/ccids/lib/tfrc.c
@@ -14,14 +14,6 @@ module_param(tfrc_debug, bool, 0444);
MODULE_PARM_DESC(tfrc_debug, "Enable debug messages");
#endif
-extern int tfrc_tx_packet_history_init(void);
-extern void tfrc_tx_packet_history_exit(void);
-extern int tfrc_rx_packet_history_init(void);
-extern void tfrc_rx_packet_history_exit(void);
-
-extern int tfrc_li_init(void);
-extern void tfrc_li_exit(void);
-
static int __init tfrc_module_init(void)
{
int rc = tfrc_li_init();
--- a/net/dccp/ccids/lib/tfrc.h
+++ b/net/dccp/ccids/lib/tfrc.h
@@ -58,7 +58,14 @@ static inline u32 tfrc_ewma(const u32 avg, const u32 newval, const u8 weight)
return avg ? (weight * avg + (10 - weight) * newval) / 10 : newval;
}
-extern u32 tfrc_calc_x(u16 s, u32 R, u32 p);
-extern u32 tfrc_calc_x_reverse_lookup(u32 fvalue);
+extern u32 tfrc_calc_x(u16 s, u32 R, u32 p);
+extern u32 tfrc_calc_x_reverse_lookup(u32 fvalue);
+extern int tfrc_tx_packet_history_init(void);
+extern void tfrc_tx_packet_history_exit(void);
+extern int tfrc_rx_packet_history_init(void);
+extern void tfrc_rx_packet_history_exit(void);
+
+extern int tfrc_li_init(void);
+extern void tfrc_li_exit(void);
#endif /* _TFRC_H_ */
--- a/net/dccp/output.c
+++ b/net/dccp/output.c
@@ -508,6 +508,7 @@ void dccp_send_ack(struct sock *sk)
EXPORT_SYMBOL_GPL(dccp_send_ack);
+#if 0
/* FIXME: Is this still necessary (11.3) - currently nowhere used by DCCP. */
void dccp_send_delayed_ack(struct sock *sk)
{
@@ -538,6 +539,7 @@ void dccp_send_delayed_ack(struct sock *sk)
icsk->icsk_ack.timeout = timeout;
sk_reset_timer(sk, &icsk->icsk_delack_timer, timeout);
}
+#endif
void dccp_send_sync(struct sock *sk, const u64 ackno,
const enum dccp_pkt_type pkt_type)
--- a/net/dccp/ackvec.c
+++ b/net/dccp/ackvec.c
@@ -290,12 +290,12 @@ int dccp_ackvec_add(struct dccp_ackvec *av, const struct sock *sk,
while (1) {
const u8 len = dccp_ackvec_len(av, index);
- const u8 state = dccp_ackvec_state(av, index);
+ const u8 av_state = dccp_ackvec_state(av, index);
/*
* valid packets not yet in av_buf have a reserved
* entry, with a len equal to 0.
*/
- if (state == DCCP_ACKVEC_STATE_NOT_RECEIVED &&
+ if (av_state == DCCP_ACKVEC_STATE_NOT_RECEIVED &&
len == 0 && delta == 0) { /* Found our
reserved seat! */
dccp_pr_debug("Found %llu reserved seat!\n",
@@ -325,31 +325,6 @@ out_duplicate:
return -EILSEQ;
}
-#ifdef CONFIG_IP_DCCP_DEBUG
-void dccp_ackvector_print(const u64 ackno, const unsigned char *vector, int len)
-{
- dccp_pr_debug_cat("ACK vector len=%d, ackno=%llu |", len,
- (unsigned long long)ackno);
-
- while (len--) {
- const u8 state = (*vector & DCCP_ACKVEC_STATE_MASK) >> 6;
- const u8 rl = *vector & DCCP_ACKVEC_LEN_MASK;
-
- dccp_pr_debug_cat("%d,%d|", state, rl);
- ++vector;
- }
-
- dccp_pr_debug_cat("\n");
-}
-
-void dccp_ackvec_print(const struct dccp_ackvec *av)
-{
- dccp_ackvector_print(av->av_buf_ackno,
- av->av_buf + av->av_buf_head,
- av->av_vec_len);
-}
-#endif
-
static void dccp_ackvec_throw_record(struct dccp_ackvec *av,
struct dccp_ackvec_record *avr)
{
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/7] dccp ccid-2: Bug-Fix - Ack Vectors need to be ignored on request sockets
2008-06-10 12:53 ` [PATCH 2/7] dccp: Fix sparse warnings Gerrit Renker
@ 2008-06-10 12:53 ` Gerrit Renker
2008-06-10 12:53 ` [PATCH 4/7] dccp ccid-3: TFRC reverse-lookup Bug-Fix Gerrit Renker
0 siblings, 1 reply; 15+ messages in thread
From: Gerrit Renker @ 2008-06-10 12:53 UTC (permalink / raw)
To: davem; +Cc: dccp, netdev, Gerrit Renker
This fixes an oversight from an earlier patch, ensuring that Ack Vectors
are not processed on request sockets.
The issue is that Ack Vectors must not be parsed on request sockets, since
the Ack Vector feature depends on the selection of the (TX) CCID. During the
initial handshake the CCIDs are undefined, and so RFC 4340, 10.3 applies:
"Using CCID-specific options and feature options during a negotiation
for the corresponding CCID feature is NOT RECOMMENDED [...]"
Worse, it is not even possible: when the server receives the Request from the
client, the CCID and Ack vector features are undefined; when the Ack finalising
the 3-way hanshake arrives, the request socket has not been cloned yet into a
full socket. (This order is necessary, since otherwise the newly created socket
would have to be destroyed whenever an option error occurred - a malicious
hacker could simply send garbage options and exploit this.)
Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
net/dccp/options.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
--- a/net/dccp/options.c
+++ b/net/dccp/options.c
@@ -107,9 +107,11 @@ int dccp_parse_options(struct sock *sk, struct dccp_request_sock *dreq,
*
* CCID-specific options are ignored during connection setup, as
* negotiation may still be in progress (see RFC 4340, 10.3).
+ * The same applies to Ack Vectors, as these depend on the CCID.
*
*/
- if (dreq != NULL && opt >= 128)
+ if (dreq != NULL && (opt >= 128 ||
+ opt == DCCPO_ACK_VECTOR_0 || opt == DCCPO_ACK_VECTOR_1))
goto ignore_option;
switch (opt) {
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 4/7] dccp ccid-3: TFRC reverse-lookup Bug-Fix
2008-06-10 12:53 ` [PATCH 3/7] dccp ccid-2: Bug-Fix - Ack Vectors need to be ignored on request sockets Gerrit Renker
@ 2008-06-10 12:53 ` Gerrit Renker
2008-06-10 12:53 ` [PATCH 5/7] dccp ccid-3: X truncated due to type conversion Gerrit Renker
0 siblings, 1 reply; 15+ messages in thread
From: Gerrit Renker @ 2008-06-10 12:53 UTC (permalink / raw)
To: davem; +Cc: dccp, netdev, Gerrit Renker
This fixes a bug in the reverse lookup of p: given a value f(p), instead of p,
the function returned the smallest tabulated value f(p).
The smallest tabulated value of
10^6 * f(p) = sqrt(2*p/3) + 12 * sqrt(3*p/8) * (32 * p^3 + p)
for p=0.0001 is 8172.
Since this value is scaled by 10^6, the outcome of this bug is that a loss
of 8172/10^6 = 0.8172% was reported whenever the input was below the table
resolution of 0.01%.
This means that the value was over 80 times too high, resulting in large spikes
of the initial loss interval, thus unnecessarily reducing the throughput.
Also corrected the printk format (%u for u32).
Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
net/dccp/ccids/lib/tfrc_equation.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
--- a/net/dccp/ccids/lib/tfrc_equation.c
+++ b/net/dccp/ccids/lib/tfrc_equation.c
@@ -661,7 +661,7 @@ u32 tfrc_calc_x(u16 s, u32 R, u32 p)
EXPORT_SYMBOL_GPL(tfrc_calc_x);
-/*
+/**
* tfrc_calc_x_reverse_lookup - try to find p given f(p)
*
* @fvalue: function value to match, scaled by 1000000
@@ -676,11 +676,11 @@ u32 tfrc_calc_x_reverse_lookup(u32 fvalue)
/* Error cases. */
if (fvalue < tfrc_calc_x_lookup[0][1]) {
- DCCP_WARN("fvalue %d smaller than resolution\n", fvalue);
- return tfrc_calc_x_lookup[0][1];
+ DCCP_WARN("fvalue %u smaller than resolution\n", fvalue);
+ return TFRC_SMALLEST_P;
}
if (fvalue > tfrc_calc_x_lookup[TFRC_CALC_X_ARRSIZE - 1][0]) {
- DCCP_WARN("fvalue %d exceeds bounds!\n", fvalue);
+ DCCP_WARN("fvalue %u exceeds bounds!\n", fvalue);
return 1000000;
}
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 5/7] dccp ccid-3: X truncated due to type conversion
2008-06-10 12:53 ` [PATCH 4/7] dccp ccid-3: TFRC reverse-lookup Bug-Fix Gerrit Renker
@ 2008-06-10 12:53 ` Gerrit Renker
2008-06-10 12:53 ` [PATCH 6/7] dccp: Bug in initial acknowledgment number assignment Gerrit Renker
0 siblings, 1 reply; 15+ messages in thread
From: Gerrit Renker @ 2008-06-10 12:53 UTC (permalink / raw)
To: davem; +Cc: dccp, netdev, Gerrit Renker
This fixes a bug in computing the inter-packet-interval t_ipi = s/X:
scaled_div32(a, b) uses u32 for b, but in "scaled_div32(s, X)" the type of the
sending rate `X' is u64. Since X is scaled by 2^6, this truncates rates greater
than 2^26 Bps (~537 Mbps).
Using full 64-bit division now.
Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
net/dccp/ccids/lib/tfrc.h | 14 ++++++--------
1 files changed, 6 insertions(+), 8 deletions(-)
--- a/net/dccp/ccids/lib/tfrc.h
+++ b/net/dccp/ccids/lib/tfrc.h
@@ -15,7 +15,7 @@
* (at your option) any later version.
*/
#include <linux/types.h>
-#include <asm/div64.h>
+#include <linux/math64.h>
#include "../../dccp.h"
/* internal includes that this module exports: */
#include "loss_interval.h"
@@ -29,21 +29,19 @@ extern int tfrc_debug;
#endif
/* integer-arithmetic divisions of type (a * 1000000)/b */
-static inline u64 scaled_div(u64 a, u32 b)
+static inline u64 scaled_div(u64 a, u64 b)
{
BUG_ON(b==0);
- a *= 1000000;
- do_div(a, b);
- return a;
+ return div64_u64(a * 1000000, b);
}
-static inline u32 scaled_div32(u64 a, u32 b)
+static inline u32 scaled_div32(u64 a, u64 b)
{
u64 result = scaled_div(a, b);
if (result > UINT_MAX) {
- DCCP_CRIT("Overflow: a(%llu)/b(%u) > ~0U",
- (unsigned long long)a, b);
+ DCCP_CRIT("Overflow: %llu/%llu > UINT_MAX",
+ (unsigned long long)a, (unsigned long long)b);
return UINT_MAX;
}
return result;
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 6/7] dccp: Bug in initial acknowledgment number assignment
2008-06-10 12:53 ` [PATCH 5/7] dccp ccid-3: X truncated due to type conversion Gerrit Renker
@ 2008-06-10 12:53 ` Gerrit Renker
2008-06-10 12:53 ` [PATCH 7/7] dccp: Initialise option area of v4/v6 request socket Gerrit Renker
0 siblings, 1 reply; 15+ messages in thread
From: Gerrit Renker @ 2008-06-10 12:53 UTC (permalink / raw)
To: davem; +Cc: dccp, netdev, Gerrit Renker
Step 8.5 in RFC 4340 says for the newly cloned socket
Initialize S.GAR := S.ISS,
but what in fact the code (minisocks.c) does is
Initialize S.GAR := S.ISR,
which is wrong (typo?) -- fixed by the patch.
Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
net/dccp/minisocks.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
--- a/net/dccp/minisocks.c
+++ b/net/dccp/minisocks.c
@@ -165,12 +165,12 @@ out_free:
/* See dccp_v4_conn_request */
newdmsk->dccpms_sequence_window = req->rcv_wnd;
- newdp->dccps_gar = newdp->dccps_isr = dreq->dreq_isr;
- dccp_update_gsr(newsk, dreq->dreq_isr);
-
- newdp->dccps_iss = dreq->dreq_iss;
+ newdp->dccps_gar = newdp->dccps_iss = dreq->dreq_iss;
dccp_update_gss(newsk, dreq->dreq_iss);
+ newdp->dccps_isr = dreq->dreq_isr;
+ dccp_update_gsr(newsk, dreq->dreq_isr);
+
/*
* SWL and AWL are initially adjusted so that they are not less than
* the initial Sequence Numbers received and sent, respectively:
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 7/7] dccp: Initialise option area of v4/v6 request socket
2008-06-10 12:53 ` [PATCH 6/7] dccp: Bug in initial acknowledgment number assignment Gerrit Renker
@ 2008-06-10 12:53 ` Gerrit Renker
2008-06-10 14:38 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 15+ messages in thread
From: Gerrit Renker @ 2008-06-10 12:53 UTC (permalink / raw)
To: davem; +Cc: dccp, netdev, Wei Yongjun
From: Wei Yongjun <yjwei@cn.fujitsu.com>
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, struct sk_buff *skb)
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, struct sk_buff *skb)
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 sock *sk, struct sk_buff *skb)
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 sock *sk, struct sk_buff *skb)
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 ||
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 7/7] dccp: Initialise option area of v4/v6 request socket
2008-06-10 12:53 ` [PATCH 7/7] dccp: Initialise option area of v4/v6 request socket Gerrit Renker
@ 2008-06-10 14:38 ` Arnaldo Carvalho de Melo
2008-06-10 19:40 ` David Miller
0 siblings, 1 reply; 15+ messages in thread
From: Arnaldo Carvalho de Melo @ 2008-06-10 14:38 UTC (permalink / raw)
To: David S. Miller; +Cc: Gerrit Renker, dccp, netdev, Wei Yongjun
Em Tue, Jun 10, 2008 at 01:53:43PM +0100, Gerrit Renker escreveu:
> From: Wei Yongjun <yjwei@cn.fujitsu.com>
>
> 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>
The problem is present in TCP too, look at cookie_v4_check and
tcp_v4_conn_request, we may get to reqsk_free before we set ->opt if
security_inet_conn_request() returns non zero.
This is one case where bugs found on DCCP lead to an audit of the
similar code in TCP and the fix should be done on the common
infrastructure.
Please consider the following patch instead, it is compile tested only
but should be OK.
Thanks a lot,
- Arnaldo
commit f627bdaa7428f04b828abd70a8145cafb7ce366b
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date: Tue Jun 10 10:37:32 2008 -0300
inet{6}_request_sock: Init ->opt and ->pktopts in the constructor
Wei Yongjun noticed that we may call reqsk_free on request sock objects where
the opt fields may not be initialized, fix it by introducing inet_reqsk_alloc
where we initialize ->opt to NULL and set ->pktopts to NULL in
inet6_reqsk_alloc.
Cc: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Cc: Wei Yongjun <yjwei@cn.fujitsu.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h
index 10b666b..cde056e 100644
--- a/include/linux/ipv6.h
+++ b/include/linux/ipv6.h
@@ -396,8 +396,10 @@ static inline struct request_sock *inet6_reqsk_alloc(struct request_sock_ops *op
{
struct request_sock *req = reqsk_alloc(ops);
- if (req != NULL)
+ if (req != NULL) {
inet_rsk(req)->inet6_rsk_offset = inet6_rsk_offset(req);
+ inet6_rsk(req)->pktopts = NULL;
+ }
return req;
}
diff --git a/include/net/inet_sock.h b/include/net/inet_sock.h
index a42cd63..9fabe5b 100644
--- a/include/net/inet_sock.h
+++ b/include/net/inet_sock.h
@@ -197,4 +197,14 @@ static inline int inet_iif(const struct sk_buff *skb)
return skb->rtable->rt_iif;
}
+static inline struct request_sock *inet_reqsk_alloc(struct request_sock_ops *ops)
+{
+ struct request_sock *req = reqsk_alloc(ops);
+
+ if (req != NULL)
+ inet_rsk(req)->opt = NULL;
+
+ return req;
+}
+
#endif /* _INET_SOCK_H */
diff --git a/net/dccp/ipv4.c b/net/dccp/ipv4.c
index c22a378..37d27bc 100644
--- a/net/dccp/ipv4.c
+++ b/net/dccp/ipv4.c
@@ -589,7 +589,7 @@ int dccp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
if (sk_acceptq_is_full(sk) && inet_csk_reqsk_queue_young(sk) > 1)
goto drop;
- req = reqsk_alloc(&dccp_request_sock_ops);
+ req = inet_reqsk_alloc(&dccp_request_sock_ops);
if (req == NULL)
goto drop;
@@ -605,7 +605,6 @@ int dccp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
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
diff --git a/net/dccp/ipv6.c b/net/dccp/ipv6.c
index 9b1129b..f7fe2a5 100644
--- a/net/dccp/ipv6.c
+++ b/net/dccp/ipv6.c
@@ -421,7 +421,6 @@ static int dccp_v6_conn_request(struct sock *sk, struct sk_buff *skb)
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 ||
diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
index 73ba989..d182a2a 100644
--- a/net/ipv4/syncookies.c
+++ b/net/ipv4/syncookies.c
@@ -285,7 +285,7 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb,
cookie_check_timestamp(&tcp_opt);
ret = NULL;
- req = reqsk_alloc(&tcp_request_sock_ops); /* for safety */
+ req = inet_reqsk_alloc(&tcp_request_sock_ops); /* for safety */
if (!req)
goto out;
@@ -301,7 +301,6 @@ struct sock *cookie_v4_check(struct sock *sk, struct sk_buff *skb,
ireq->rmt_port = th->source;
ireq->loc_addr = ip_hdr(skb)->daddr;
ireq->rmt_addr = ip_hdr(skb)->saddr;
- ireq->opt = NULL;
ireq->snd_wscale = tcp_opt.snd_wscale;
ireq->rcv_wscale = tcp_opt.rcv_wscale;
ireq->sack_ok = tcp_opt.sack_ok;
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index cd601a8..4f8485c 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1285,7 +1285,7 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
if (sk_acceptq_is_full(sk) && inet_csk_reqsk_queue_young(sk) > 1)
goto drop;
- req = reqsk_alloc(&tcp_request_sock_ops);
+ req = inet_reqsk_alloc(&tcp_request_sock_ops);
if (!req)
goto drop;
diff --git a/net/ipv6/syncookies.c b/net/ipv6/syncookies.c
index 938ce4e..3ecc115 100644
--- a/net/ipv6/syncookies.c
+++ b/net/ipv6/syncookies.c
@@ -198,7 +198,6 @@ struct sock *cookie_v6_check(struct sock *sk, struct sk_buff *skb)
ireq = inet_rsk(req);
ireq6 = inet6_rsk(req);
treq = tcp_rsk(req);
- ireq6->pktopts = NULL;
if (security_inet_conn_request(sk, skb, req)) {
reqsk_free(req);
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 715965f..cb46749 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1299,7 +1299,6 @@ static int tcp_v6_conn_request(struct sock *sk, struct sk_buff *skb)
treq = inet6_rsk(req);
ipv6_addr_copy(&treq->rmt_addr, &ipv6_hdr(skb)->saddr);
ipv6_addr_copy(&treq->loc_addr, &ipv6_hdr(skb)->daddr);
- treq->pktopts = NULL;
if (!want_cookie)
TCP_ECN_create_request(req, tcp_hdr(skb));
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Patch 0/7] [dccp]: DCCP bug-fixes for net-2.6
2008-06-10 12:53 ` [Patch 0/7] [dccp]: DCCP bug-fixes for net-2.6 Gerrit Renker
2008-06-10 12:53 ` [PATCH 1/7] dccp ccid-3: Bug-Fix - Zero RTT is possible Gerrit Renker
@ 2008-06-10 19:29 ` David Miller
2008-06-10 19:32 ` David Miller
2 siblings, 0 replies; 15+ messages in thread
From: David Miller @ 2008-06-10 19:29 UTC (permalink / raw)
To: gerrit; +Cc: dccp, netdev
From: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Date: Tue, 10 Jun 2008 13:53:36 +0100
> please consider pulling from
>
> git://eden-feed.erg.abdn.ac.uk/net-2.6
>
> This set has bug-fixes only, non-bug fixes will follow at a later stage.
When specifying a URL for me to pull from, you have to
specify which branch I should pull from even if it is
plainly just "master".
GIT requires this specification when pulling, and you
can eliminate all doubt by letting me know which branch
to pull from explicitly.
In any event, I'm not pulling this because I saw some
things that I want you to cleanup first.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/7] dccp ccid-3: Bug-Fix - Zero RTT is possible
2008-06-10 12:53 ` [PATCH 1/7] dccp ccid-3: Bug-Fix - Zero RTT is possible Gerrit Renker
2008-06-10 12:53 ` [PATCH 2/7] dccp: Fix sparse warnings Gerrit Renker
@ 2008-06-10 19:31 ` David Miller
1 sibling, 0 replies; 15+ messages in thread
From: David Miller @ 2008-06-10 19:31 UTC (permalink / raw)
To: gerrit; +Cc: dccp, netdev
From: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Date: Tue, 10 Jun 2008 13:53:37 +0100
> Commit 825de27d9e40b3117b29a79d412b7a4b78c5d815 fixed the CCID-3 window counter
> computation for RTTs < 4 microseconds (as happens on loopback).
When referencing GIT SHA1 IDs in commit logs, always
provide the commit changelog header line text like
this:
In commit $(SHA1_ID) ("[DCCP]: blah blah blah") we
did whatever...
because if this patch is ported into another GIT tree,
the SHA1_ID of the referenced commit might be different
and the commit header line text helps people find the
correct change you are referring to even if this happens.
I say this to someone at least one time every day that I
integrate patches. Please be mindful of this so I don't
have to ask you to do it again, thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Patch 0/7] [dccp]: DCCP bug-fixes for net-2.6
2008-06-10 12:53 ` [Patch 0/7] [dccp]: DCCP bug-fixes for net-2.6 Gerrit Renker
2008-06-10 12:53 ` [PATCH 1/7] dccp ccid-3: Bug-Fix - Zero RTT is possible Gerrit Renker
2008-06-10 19:29 ` [Patch 0/7] [dccp]: DCCP bug-fixes for net-2.6 David Miller
@ 2008-06-10 19:32 ` David Miller
2008-06-11 10:36 ` [Patch 0/6] [dccp]: DCCP bug-fixes for net-2.6 (revision 01) Gerrit Renker
2 siblings, 1 reply; 15+ messages in thread
From: David Miller @ 2008-06-10 19:32 UTC (permalink / raw)
To: gerrit; +Cc: dccp, netdev
From: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Date: Tue, 10 Jun 2008 13:53:36 +0100
> please consider pulling from
>
> git://eden-feed.erg.abdn.ac.uk/net-2.6
Please redo your tree, fixing up the SHA1 commit ID reference in the
first changeset as I asked, and I will pull this into net-2.6
Thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 7/7] dccp: Initialise option area of v4/v6 request socket
2008-06-10 14:38 ` Arnaldo Carvalho de Melo
@ 2008-06-10 19:40 ` David Miller
0 siblings, 0 replies; 15+ messages in thread
From: David Miller @ 2008-06-10 19:40 UTC (permalink / raw)
To: acme; +Cc: gerrit, dccp, netdev, yjwei
From: Arnaldo Carvalho de Melo <acme@redhat.com>
Date: Tue, 10 Jun 2008 11:38:12 -0300
> Please consider the following patch instead, it is compile tested only
> but should be OK.
I've applied this, thanks Arnaldo.
Gerrit, please be mindful of this when rebuilding your
DCCP bug fix tree. Thanks!
^ permalink raw reply [flat|nested] 15+ messages in thread
* [Patch 0/6] [dccp]: DCCP bug-fixes for net-2.6 (revision 01)
2008-06-10 19:32 ` David Miller
@ 2008-06-11 10:36 ` Gerrit Renker
2008-06-12 1:10 ` David Miller
0 siblings, 1 reply; 15+ messages in thread
From: Gerrit Renker @ 2008-06-11 10:36 UTC (permalink / raw)
To: David Miller; +Cc: dccp, netdev
Hi David,
please consider the revised set of DCCP Bug-Fix patches
(changelog below), which can be pulled from:
git://eden-feed.erg.abdn.ac.uk/net-2.6
All patches are in branch `master', on top of a freshly cloned net-2.6 tree.
Result has been compile-tested again.
List of changes introduced in this submission
---------------------------------------------
Relative to yesterday's submission, only these things have changed:
* rewrote and updated the commit message of the first patch, which was
referencing a SHA1 ID only. This now has data and commit information added;
* removed the request-sock initialisation patch, since this has already been
fixed in your tree.
View-ability of changes
-----------------------
Since there are no code changes, I omit sending all the patches again. But
the differences can in any case be viewed on
http://eden-feed.erg.abdn.ac.uk/cgi-bin/gitweb.cgi?p=net-2.6.git;a=summary
List of patches included in this set:
-------------------------------------
Patch #1: Fixes a divide-by-zero bug in CCID-3.
Patch #2: Resolves sparse warnings (gathered from several patches in the tree).
Patch #3: Enforces that Ack Vectors are not interpreted on request sockets.
Patch #4: Computation error in CCID-3 allowed sending rate.
Patch #5: Sending rate in CCID-3 truncated due to u64 -> u32 conversion.
Patch #6: Typo in initial sequence number assignment.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Patch 0/6] [dccp]: DCCP bug-fixes for net-2.6 (revision 01)
2008-06-11 10:36 ` [Patch 0/6] [dccp]: DCCP bug-fixes for net-2.6 (revision 01) Gerrit Renker
@ 2008-06-12 1:10 ` David Miller
0 siblings, 0 replies; 15+ messages in thread
From: David Miller @ 2008-06-12 1:10 UTC (permalink / raw)
To: gerrit; +Cc: dccp, netdev
From: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Date: Wed, 11 Jun 2008 11:36:47 +0100
> please consider the revised set of DCCP Bug-Fix patches
> (changelog below), which can be pulled from:
>
> git://eden-feed.erg.abdn.ac.uk/net-2.6
>
> All patches are in branch `master', on top of a freshly cloned net-2.6 tree.
> Result has been compile-tested again.
Pulled and pushed back out to net-2.6, thanks!
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2008-06-12 1:10 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <dccp_bug_fixes_for_net-2.6>
2008-06-10 12:53 ` [Patch 0/7] [dccp]: DCCP bug-fixes for net-2.6 Gerrit Renker
2008-06-10 12:53 ` [PATCH 1/7] dccp ccid-3: Bug-Fix - Zero RTT is possible Gerrit Renker
2008-06-10 12:53 ` [PATCH 2/7] dccp: Fix sparse warnings Gerrit Renker
2008-06-10 12:53 ` [PATCH 3/7] dccp ccid-2: Bug-Fix - Ack Vectors need to be ignored on request sockets Gerrit Renker
2008-06-10 12:53 ` [PATCH 4/7] dccp ccid-3: TFRC reverse-lookup Bug-Fix Gerrit Renker
2008-06-10 12:53 ` [PATCH 5/7] dccp ccid-3: X truncated due to type conversion Gerrit Renker
2008-06-10 12:53 ` [PATCH 6/7] dccp: Bug in initial acknowledgment number assignment Gerrit Renker
2008-06-10 12:53 ` [PATCH 7/7] dccp: Initialise option area of v4/v6 request socket Gerrit Renker
2008-06-10 14:38 ` Arnaldo Carvalho de Melo
2008-06-10 19:40 ` David Miller
2008-06-10 19:31 ` [PATCH 1/7] dccp ccid-3: Bug-Fix - Zero RTT is possible David Miller
2008-06-10 19:29 ` [Patch 0/7] [dccp]: DCCP bug-fixes for net-2.6 David Miller
2008-06-10 19:32 ` David Miller
2008-06-11 10:36 ` [Patch 0/6] [dccp]: DCCP bug-fixes for net-2.6 (revision 01) Gerrit Renker
2008-06-12 1:10 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).