Off by one buglets

PATCH Ralf Baechle 2 participants 3 messages Jun 30, 2006 – Jul 31, 2006

Subsystems (5)

Networking Network drivers SCTP protocol ATM +1 more subsystems

lore

Share & tools

lore

Ralf Baechle

author

2006-06-30 14:29 UTC #0 lore raw

Ages ago, changeset

http://www.kernel.org/git/?p=linux/kernel/git/tglx/history.git;a=commit;h=22d864d542a0b92116751186f1794c7d0f1ca1b9

which converted several protocols from using open coded comparisons to use the helper function sk_acceptq_is_full() did introduce a bunch of off by one errors - sk_acceptq_is_full checks for sk_ack_backlog > sk_max_ack_backlog but it replaced >= or == comparisons.

Below patch is really only meant to illustrate the change, not to be applied.

Ralf

Signed-off-by Ralf Baechle <[email protected]>

 net/atm/signaling.c     |    3 ++-
 net/ax25/ax25_in.c      |    2 +-
 net/decnet/dn_nsp_in.c  |    2 +-
 net/netrom/af_netrom.c  |    2 +-
 net/rose/af_rose.c      |    2 +-
 net/sctp/sm_statefuns.c |    2 +-
 net/x25/af_x25.c        |    2 +-
 7 files changed, 8 insertions(+), 7 deletions(-)

net/atm/signaling.c +2 -1

Index: linux-net/net/atm/signaling.c
===================================================================
@@ -132,7 +132,8 @@ static int sigd_send(struct atm_vcc *vcc
 			sk = sk_atm(vcc);
 			DPRINTK("as_indicate!!!\n");
 			lock_sock(sk);
-			if (sk_acceptq_is_full(sk)) {
+			if (vcc->sk->sk_ack_backlog ==
+			    vcc->sk->sk_max_ack_backlog) {
 				sigd_enq(NULL,as_reject,vcc,NULL,NULL);
 				dev_kfree_skb(skb);
 				goto as_indicate_complete;

net/ax25/ax25_in.c +1 -1

Index: linux-net/net/ax25/ax25_in.c
===================================================================
@@ -351,7 +351,7 @@ static int ax25_rcv(struct sk_buff *skb,
 
 	if (sk != NULL) {
 		bh_lock_sock(sk);
-		if (sk_acceptq_is_full(sk) ||
+		if (sk->sk_ack_backlog == sk->sk_max_ack_backlog ||
 		    (make = ax25_make_new(sk, ax25_dev)) == NULL) {
 			if (mine)
 				ax25_return_dm(dev, &src, &dest, &dp);

net/decnet/dn_nsp_in.c +1 -1

Index: linux-net/net/decnet/dn_nsp_in.c
===================================================================
@@ -324,7 +324,7 @@ err_out:
 
 static void dn_nsp_conn_init(struct sock *sk, struct sk_buff *skb)
 {
-	if (sk_acceptq_is_full(sk)) {
+	if (sk->sk_ack_backlog >= sk->sk_max_ack_backlog) {
 		kfree_skb(skb);
 		return;
 	}

net/netrom/af_netrom.c +1 -1

Index: linux-net/net/netrom/af_netrom.c
===================================================================
@@ -933,7 +933,7 @@ int nr_rx_frame(struct sk_buff *skb, str
 
 	user = (ax25_address *)(skb->data + 21);
 
-	if (sk == NULL || sk_acceptq_is_full(sk) ||
+	if (sk == NULL || sk->sk_ack_backlog == sk->sk_max_ack_backlog ||
 	    (make = nr_make_new(sk)) == NULL) {
 		nr_transmit_refusal(skb, 0);
 		if (sk)

net/rose/af_rose.c +1 -1

Index: linux-net/net/rose/af_rose.c
===================================================================
@@ -948,7 +948,7 @@ int rose_rx_call_request(struct sk_buff 
 	/*
 	 * We can't accept the Call Request.
 	 */
-	if (sk == NULL || sk_acceptq_is_full(sk) ||
+	if (sk == NULL || sk->sk_ack_backlog == sk->sk_max_ack_backlog ||
 	    (make = rose_make_new(sk)) == NULL) {
 		rose_transmit_clear_request(neigh, lci, ROSE_NETWORK_CONGESTION, 120);
 		return 0;

net/sctp/sm_statefuns.c +1 -1

Index: linux-net/net/sctp/sm_statefuns.c
===================================================================
@@ -282,7 +282,7 @@ sctp_disposition_t sctp_sf_do_5_1B_init(
 	 */
 	if (!sctp_sstate(sk, LISTENING) ||
 	    (sctp_style(sk, TCP) &&
-	     sk_acceptq_is_full(sk)))
+	     (sk->sk_ack_backlog >= sk->sk_max_ack_backlog)))
 		return sctp_sf_tabort_8_4_8(ep, asoc, type, arg, commands);
 
 	/* 3.1 A packet containing an INIT chunk MUST have a zero Verification

net/x25/af_x25.c +1 -1

Index: linux-net/net/x25/af_x25.c
===================================================================
@@ -879,7 +879,7 @@ int x25_rx_call_request(struct sk_buff *
 	/*
 	 *	We can't accept the Call Request.
 	 */
-	if (sk == NULL || sk_acceptq_is_full(sk))
+	if (sk == NULL || sk->sk_ack_backlog == sk->sk_max_ack_backlog)
 		goto out_clear_request;
 
 	/*

David Miller

maintainer

2006-07-04 02:33 UTC #1 lore raw

From: Ralf Baechle <[email protected]>
Date: Fri, 30 Jun 2006 15:29:01 +0100

Quoted 11 lines

Ages ago, changeset
http://www.kernel.org/git/?p=linux/kernel/git/tglx/history.git;a=commit;h=22d864d542a0b92116751186f1794c7d0f1ca1b9
which converted several protocols from using open coded comparisons to use the helper function sk_acceptq_is_full() did introduce a bunch of off by one errors - sk_acceptq_is_full checks for sk_ack_backlog > sk_max_ack_backlog but it replaced >= or == comparisons.
Below patch is really only meant to illustrate the change, not to be applied.

Ok, I'll try to resolve this when I get back from my vacation unless someone beats me to it.

Thanks for the heads up Ralf.

David Miller

maintainer

2006-07-31 22:29 UTC #2 lore raw

In reply to Ralf Baechle

From: Ralf Baechle <[email protected]>
Date: Fri, 30 Jun 2006 15:29:01 +0100

Quoted 11 lines

Ages ago, changeset
http://www.kernel.org/git/?p=linux/kernel/git/tglx/history.git;a=commit;h=22d864d542a0b92116751186f1794c7d0f1ca1b9
which converted several protocols from using open coded comparisons to use the helper function sk_acceptq_is_full() did introduce a bunch of off by one errors - sk_acceptq_is_full checks for sk_ack_backlog > sk_max_ack_backlog but it replaced >= or == comparisons.
Below patch is really only meant to illustrate the change, not to be applied.

I looked at this again, and the change is perfectly fine.

This patch merely shows that previously the protocols were very inconsistent about what sk_max_ack_backlog really meant. All Arnaldo's changeset did was enforce a consistent meaning of this limit across the entire tree.