[PATCH]: Consolidate ax25 socket grafting.

[PATCH]: Consolidate ax25 socket grafting.

[David Miller]

[ For the AX.25 folks: I'm basically walking the tree auditing
  how parent sockets get grafted to child socks and making protocols
  use sock_graft() and sock_orphan() when they are open-coding that
  construct. ]

Applied and pushed to net-next-2.6

ax25: Use sock_graft() and remove bogus sk_socket and sk_sleep init.

The way that listening sockets work in ax25 is that the packet input
code path creates new socks via ax25_make_new() and attaches them
to the incoming SKB.  This SKB gets queued up into the listening
socket's receive queue.

When accept()'d the sock gets hooked up to the real parent socket.
Alternatively, if the listening socket is closed and released, any
unborn socks stuff up in the receive queue get released.

So during this time period these sockets are unreachable in any
other way, so no wakeup events nor references to their ->sk_socket
and ->sk_sleep members can occur.  And even if they do, all such
paths have to make NULL checks.

So do not deceptively initialize them in ax25_make_new() to the
values in the listening socket.  Leave them at NULL.

Finally, use sock_graft() in ax25_accept().

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/ax25/af_ax25.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index 2712544..97eaa23 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -893,13 +893,11 @@ struct sock *ax25_make_new(struct sock *osk, struct ax25_dev *ax25_dev)
 
 	sk->sk_destruct = ax25_free_sock;
 	sk->sk_type     = osk->sk_type;
-	sk->sk_socket   = osk->sk_socket;
 	sk->sk_priority = osk->sk_priority;
 	sk->sk_protocol = osk->sk_protocol;
 	sk->sk_rcvbuf   = osk->sk_rcvbuf;
 	sk->sk_sndbuf   = osk->sk_sndbuf;
 	sk->sk_state    = TCP_ESTABLISHED;
-	sk->sk_sleep    = osk->sk_sleep;
 	sock_copy_flags(sk, osk);
 
 	oax25 = ax25_sk(osk);
@@ -1361,13 +1359,11 @@ static int ax25_accept(struct socket *sock, struct socket *newsock, int flags)
 		goto out;
 
 	newsk		 = skb->sk;
-	newsk->sk_socket = newsock;
-	newsk->sk_sleep	 = &newsock->wait;
+	sock_graft(newsk, newsock);
 
 	/* Now attach up the new socket */
 	kfree_skb(skb);
 	sk->sk_ack_backlog--;
-	newsock->sk    = newsk;
 	newsock->state = SS_CONNECTED;
 
 out:
-- 
1.5.5.1.308.g1fbb5

Re: [PATCH]: Consolidate ax25 socket grafting.

[Tihomir Heidelberg - 9a4gl]

Hi linux-hams,

David Miller wrote:
> The way that listening sockets work in ax25 is that the packet input
> code path creates new socks via ax25_make_new() and attaches them
> to the incoming SKB.  This SKB gets queued up into the listening
> socket's receive queue.
>
> When accept()'d the sock gets hooked up to the real parent socket.
> Alternatively, if the listening socket is closed and released, any
> unborn socks stuff up in the receive queue get released.
>
> So during this time period these sockets are unreachable in any
> other way, so no wakeup events nor references to their ->sk_socket
> and ->sk_sleep members can occur.  And even if they do, all such
> paths have to make NULL checks.
>   
Yes, but...

I would like to direct you attention to one problem existing in ax.25 
kernel since 2.4. If listening socket is closed and its SKB queue is 
released but those sockets get weird. Those "unAccepted()" sockets 
should be destroyed in ax25_std_heartbeat_expiry, but it will not 
happen. And there is also a note about that in ax25_std_timer.c:
/* Magic here: If we listen() and a new link dies before it
is accepted() it isn't 'dead' so doesn't get removed. */

This issue cause ax25d to stop accepting new connections and I had to 
restarted ax25d approximately each day and my services were unavailable. 
Also netstat -n -l shows invalid source and device for those listening 
sockets. It is strange why ax25d's listening socket get weird because of 
this issue, but definitely when I solved this bug I do not have problems 
with ax25d anymore and my ax25d can run for months without problems.

Actually I didn't fix the bug, I made a hack. In af_ax25.c file, 
function ax25_destroy_socket in while loop:
        if (ax25->sk != NULL) {
                while ((skb = skb_dequeue(&ax25->sk->sk_receive_queue)) 
!= NULL
                        if (skb->sk != ax25->sk) {
                                /* A pending connection */
                                ax25_cb *sax25 = ax25_sk(skb->sk);

                                /* Queue the unaccepted socket for death */
                                sock_orphan(skb->sk);

+                                /* 9A4GL: hack to release unaccepted 
sockets */
+                                skb->sk->sk_state = TCP_LISTEN;

                                ax25_start_heartbeat(sax25);
                                sax25->state = AX25_STATE_0;
                        }

                        kfree_skb(skb);
                }
                skb_queue_purge(&ax25->sk->sk_write_queue);
        }

73 de Tihomir, 9a4gl

Re: [PATCH]: Consolidate ax25 socket grafting.

[David Miller]

From: Tihomir Heidelberg - 9a4gl <9a4gl@hamradio.hr>
Date: Tue, 17 Jun 2008 14:11:36 +0200

> I would like to direct you attention to one problem existing in ax.25 
> kernel since 2.4. If listening socket is closed and its SKB queue is 
> released but those sockets get weird. Those "unAccepted()" sockets 
> should be destroyed in ax25_std_heartbeat_expiry, but it will not 
> happen. And there is also a note about that in ax25_std_timer.c:
> /* Magic here: If we listen() and a new link dies before it
> is accepted() it isn't 'dead' so doesn't get removed. */
> 
> This issue cause ax25d to stop accepting new connections and I had to 
> restarted ax25d approximately each day and my services were unavailable. 
> Also netstat -n -l shows invalid source and device for those listening 
> sockets. It is strange why ax25d's listening socket get weird because of 
> this issue, but definitely when I solved this bug I do not have problems 
> with ax25d anymore and my ax25d can run for months without problems.

Thanks for the report.  I just committed the following patch
which should fix the bug:

ax25: Fix std timer socket destroy handling.

Tihomir Heidelberg - 9a4gl, reports:

--------------------
I would like to direct you attention to one problem existing in ax.25
kernel since 2.4. If listening socket is closed and its SKB queue is
released but those sockets get weird. Those "unAccepted()" sockets
should be destroyed in ax25_std_heartbeat_expiry, but it will not
happen. And there is also a note about that in ax25_std_timer.c:
/* Magic here: If we listen() and a new link dies before it
is accepted() it isn't 'dead' so doesn't get removed. */

This issue cause ax25d to stop accepting new connections and I had to
restarted ax25d approximately each day and my services were unavailable.
Also netstat -n -l shows invalid source and device for those listening
sockets. It is strange why ax25d's listening socket get weird because of
this issue, but definitely when I solved this bug I do not have problems
with ax25d anymore and my ax25d can run for months without problems.
--------------------

Actually as far as I can see, this problem is even in releases
as far back as 2.2.x as well.

It seems senseless to special case this test on TCP_LISTEN state.
Anything still stuck in state 0 has no external references and
we can just simply kill it off directly.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/ax25/ax25_std_timer.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/net/ax25/ax25_std_timer.c b/net/ax25/ax25_std_timer.c
index 96e4b92..cdc7e75 100644
--- a/net/ax25/ax25_std_timer.c
+++ b/net/ax25/ax25_std_timer.c
@@ -39,11 +39,9 @@ void ax25_std_heartbeat_expiry(ax25_cb *ax25)
 
 	switch (ax25->state) {
 	case AX25_STATE_0:
-		/* Magic here: If we listen() and a new link dies before it
-		   is accepted() it isn't 'dead' so doesn't get removed. */
-		if (!sk || sock_flag(sk, SOCK_DESTROY) ||
-		    (sk->sk_state == TCP_LISTEN &&
-		     sock_flag(sk, SOCK_DEAD))) {
+		if (!sk ||
+		    sock_flag(sk, SOCK_DESTROY) ||
+		    sock_flag(sk, SOCK_DEAD)) {
 			if (sk) {
 				sock_hold(sk);
 				ax25_destroy_socket(ax25);
-- 
1.5.5.1.308.g1fbb5

[PATCH]: Consolidate ax25 socket grafting.

[David Miller]

This is the netrom variant of the ax25 patch I just posted.

Applied and pushed to net-next-2.6

netrom: Use sock_graft() and remove bogus sk_socket and sk_sleep init.

This is the netrom variant of changeset
9375cb8a1232d2a15fe34bec4d3474872e02faec
("ax25: Use sock_graft() and remove bogus sk_socket and sk_sleep init.")

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/netrom/af_netrom.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/net/netrom/af_netrom.c b/net/netrom/af_netrom.c
index 4bae8b9..5877962 100644
--- a/net/netrom/af_netrom.c
+++ b/net/netrom/af_netrom.c
@@ -475,13 +475,11 @@ static struct sock *nr_make_new(struct sock *osk)
 	sock_init_data(NULL, sk);
 
 	sk->sk_type     = osk->sk_type;
-	sk->sk_socket   = osk->sk_socket;
 	sk->sk_priority = osk->sk_priority;
 	sk->sk_protocol = osk->sk_protocol;
 	sk->sk_rcvbuf   = osk->sk_rcvbuf;
 	sk->sk_sndbuf   = osk->sk_sndbuf;
 	sk->sk_state    = TCP_ESTABLISHED;
-	sk->sk_sleep    = osk->sk_sleep;
 	sock_copy_flags(sk, osk);
 
 	skb_queue_head_init(&nr->ack_queue);
@@ -810,13 +808,11 @@ static int nr_accept(struct socket *sock, struct socket *newsock, int flags)
 		goto out_release;
 
 	newsk = skb->sk;
-	newsk->sk_socket = newsock;
-	newsk->sk_sleep = &newsock->wait;
+	sock_graft(newsk, newsock);
 
 	/* Now attach up the new socket */
 	kfree_skb(skb);
 	sk_acceptq_removed(sk);
-	newsock->sk = newsk;
 
 out_release:
 	release_sock(sk);
-- 
1.5.5.1.308.g1fbb5