net: Fix sock freeing before sock_init_data() with __sk_free()

After recent changes sk_free() frees socks conditionally and depends on sk_wmem_alloc beeing set e.g. in sock_init_data(). But in some cases sk_free() is called earlier, usually after other alloc errors. This patch fixes it by exporting and using __sk_free() directly.

Signed-off-by Jarek Poplawski <[email protected]>

---

 include/net/sock.h |    1 +
 net/ax25/af_ax25.c |    6 +++---
 net/core/sock.c    |    6 ++++--
 net/irda/af_irda.c |    4 ++--
 net/tipc/socket.c  |    2 +-
 5 files changed, 11 insertions(+), 8 deletions(-)

include/net/sock.h +1 -0

index 950409d..e76ef65 100644
@@ -935,6 +935,7 @@ extern void release_sock(struct sock *sk);
 extern struct sock		*sk_alloc(struct net *net, int family,
 					  gfp_t priority,
 					  struct proto *prot);
+extern void			__sk_free(struct sock *sk);
 extern void			sk_free(struct sock *sk);
 extern void			sk_release_kernel(struct sock *sk);
 extern struct sock		*sk_clone(const struct sock *sk,

net/ax25/af_ax25.c +3 -3

index da0f64f..c05738c 100644
@@ -851,7 +851,7 @@ static int ax25_create(struct net *net, struct socket *sock, int protocol)
 
 	ax25 = sk->sk_protinfo = ax25_create_cb();
 	if (!ax25) {
-		sk_free(sk);
+		__sk_free(sk);
 		return -ENOMEM;
 	}
 

@@ -876,7 +876,7 @@ struct sock *ax25_make_new(struct sock *osk, struct ax25_dev *ax25_dev)
 		return NULL;
 
 	if ((ax25 = ax25_create_cb()) == NULL) {
-		sk_free(sk);
+		__sk_free(sk);
 		return NULL;
 	}
 

@@ -886,7 +886,7 @@ struct sock *ax25_make_new(struct sock *osk, struct ax25_dev *ax25_dev)
 	case SOCK_SEQPACKET:
 		break;
 	default:
-		sk_free(sk);
+		__sk_free(sk);
 		ax25_cb_put(ax25);
 		return NULL;
 	}

net/core/sock.c +4 -2

index bbb25be..92793be 100644
@@ -1031,7 +1031,7 @@ struct sock *sk_alloc(struct net *net, int family, gfp_t priority,
 }
 EXPORT_SYMBOL(sk_alloc);
 
-static void __sk_free(struct sock *sk)
+void __sk_free(struct sock *sk)
 {
 	struct sk_filter *filter;
 

@@ -1054,13 +1054,15 @@ static void __sk_free(struct sock *sk)
 	put_net(sock_net(sk));
 	sk_prot_free(sk->sk_prot_creator, sk);
 }
+EXPORT_SYMBOL(__sk_free);
 
 void sk_free(struct sock *sk)
 {
 	/*
 	 * We substract one from sk_wmem_alloc and can know if
 	 * some packets are still in some tx queue.
-	 * If not null, sock_wfree() will call __sk_free(sk) later
+	 * If not null, sock_wfree() will call __sk_free(sk) later.
+	 * (Before sock_init_data() etc. use __sk_free() instead.)
 	 */
 	if (atomic_dec_and_test(&sk->sk_wmem_alloc))
 		__sk_free(sk);

net/irda/af_irda.c +2 -2

index 50b43c5..9ed2060 100644
@@ -1118,12 +1118,12 @@ static int irda_create(struct net *net, struct socket *sock, int protocol)
 			self->max_sdu_size_rx = TTP_SAR_UNBOUND;
 			break;
 		default:
-			sk_free(sk);
+			__sk_free(sk);
 			return -ESOCKTNOSUPPORT;
 		}
 		break;
 	default:
-		sk_free(sk);
+		__sk_free(sk);
 		return -ESOCKTNOSUPPORT;
 	}
 

net/tipc/socket.c +0 -0

index 1848693..4c8435a 100644

Quoted 4 lines · Jarek Poplawski

After recent changes sk_free() frees socks conditionally and depends on sk_wmem_alloc beeing set e.g. in sock_init_data(). But in some cases sk_free() is called earlier, usually after other alloc errors. This patch fixes it by exporting and using __sk_free() directly.

Very nice catch Jarek, but dont you think it would be cleaner to make sure we can call sk_free() right after sk_alloc() instead, and not exporting __sk_free() ?

ie initialize wmem_alloc in sk_alloc() instead of initializing it in sock_init_data() ?

Quoted 5 lines · Eric Dumazet

Quoted 4 lines · Jarek Poplawski

After recent changes sk_free() frees socks conditionally and depends on sk_wmem_alloc beeing set e.g. in sock_init_data(). But in some cases sk_free() is called earlier, usually after other alloc errors. This patch fixes it by exporting and using __sk_free() directly.

...

Quoted 4 lines

Very nice catch Jarek, but dont you think it would be cleaner to make sure we can call sk_free() right after sk_alloc() instead, and not exporting __sk_free() ?

Quoted 3 lines

ie initialize wmem_alloc in sk_alloc() instead of initializing it in sock_init_data() ?

Most probably it should be better. But I meant this fix for -net and didn't wan't to break too much... So, if you're sure it's OK feel free to send your version. (Or it could be changed like this in the -next.)

Quoted 15 lines · Jarek Poplawski

Quoted 5 lines · Eric Dumazet

Quoted 4 lines · Jarek Poplawski

After recent changes sk_free() frees socks conditionally and depends on sk_wmem_alloc beeing set e.g. in sock_init_data(). But in some cases sk_free() is called earlier, usually after other alloc errors. This patch fixes it by exporting and using __sk_free() directly.

...

Quoted 7 lines

Very nice catch Jarek, but dont you think it would be cleaner to make sure we can call sk_free() right after sk_alloc() instead, and not exporting __sk_free() ?

ie initialize wmem_alloc in sk_alloc() instead of initializing it in sock_init_data() ?

Quoted 3 lines

Most probably it should be better. But I meant this fix for -net and didn't wan't to break too much... So, if you're sure it's OK feel free to send your version. (Or it could be changed like this in the -next.)

Well, patch is yours, not mine, and I am confident it is OK.

We should check that no sk_alloc() user did a blind memset() or something strange like that, before calling sock_init_data() or sk_free()

net/core/sock.c +1 -1

index bbb25be..7633422 100644
@@ -1025,6 +1025,7 @@ struct sock *sk_alloc(struct net *net, int family, gfp_t priority,
 		sk->sk_prot = sk->sk_prot_creator = prot;
 		sock_lock_init(sk);
 		sock_net_set(sk, get_net(net));
+		atomic_set(&sk->sk_wmem_alloc, 1);
 	}
 
 	return sk;

@@ -1872,7 +1873,6 @@ void sock_init_data(struct socket *sock, struct sock *sk)
 	 */
 	smp_wmb();
 	atomic_set(&sk->sk_refcnt, 1);
-	atomic_set(&sk->sk_wmem_alloc, 1);
 	atomic_set(&sk->sk_drops, 0);
 }
 EXPORT_SYMBOL(sock_init_data);

Quoted 12 lines · Eric Dumazet

Quoted 11 lines · Jarek Poplawski

Quoted 5 lines · Eric Dumazet

Quoted 4 lines · Jarek Poplawski

After recent changes sk_free() frees socks conditionally and depends on sk_wmem_alloc beeing set e.g. in sock_init_data(). But in some cases sk_free() is called earlier, usually after other alloc errors. This patch fixes it by exporting and using __sk_free() directly.

...

Quoted 4 lines

Very nice catch Jarek, but dont you think it would be cleaner to make sure we can call sk_free() right after sk_alloc() instead, and not exporting __sk_free() ?

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

ie initialize wmem_alloc in sk_alloc() instead of initializing it in sock_init_data() ?

Most probably it should be better. But I meant this fix for -net and didn't wan't to break too much... So, if you're sure it's OK feel free to send your version. (Or it could be changed like this in the -next.)

Well, patch is yours, not mine, and I am confident it is OK.

Well, it's from you, and I guess you'll sign off too, but if you think so...

Signed-off-by Jarek Poplawski <[email protected]>

Thanks,
Jarek P.

Quoted 1 line · Jarek Poplawski

On Mon, Aug 31, 2009 at 08:50:25AM +0200, Eric Dumazet wrote:

Quoted 7 lines

Quoted 1 line

Well, patch is yours, not mine, and I am confident it is OK.

Well, it's from you, and I guess you'll sign off too, but if you think so...

Signed-off-by Jarek Poplawski <[email protected]>

Quoted 3 lines

Thanks,
Jarek P.

Give me a few hours to review sk_alloc() call sites, test patch and officially submit it.

Thanks

From: Jarek Poplawski <[email protected]>

After commit 2b85a34e911bf483c27cfdd124aeb1605145dc80 (net: No more expensive sock_hold()/sock_put() on each tx) sk_free() frees socks conditionally and depends
on sk_wmem_alloc beeing set e.g. in sock_init_data(). But in some

cases sk_free() is called earlier, usually after other alloc errors.

Fix is to move sk_wmem_alloc initialization from sock_init_data() to sk_alloc() itself.

Signed-off-by Jarek Poplawski <[email protected]>

Signed-off-by Eric Dumazet <[email protected]>

---

net/core/sock.c +1 -1

index bbb25be..7633422 100644
@@ -1025,6 +1025,7 @@ struct sock *sk_alloc(struct net *net, int family, gfp_t priority,
 		sk->sk_prot = sk->sk_prot_creator = prot;
 		sock_lock_init(sk);
 		sock_net_set(sk, get_net(net));
+		atomic_set(&sk->sk_wmem_alloc, 1);
 	}
 
 	return sk;

@@ -1872,7 +1873,6 @@ void sock_init_data(struct socket *sock, struct sock *sk)
 	 */
 	smp_wmb();
 	atomic_set(&sk->sk_refcnt, 1);
-	atomic_set(&sk->sk_wmem_alloc, 1);
 	atomic_set(&sk->sk_drops, 0);
 }
 EXPORT_SYMBOL(sock_init_data);

Quoted 6 lines

From: Jarek Poplawski <[email protected]>

After commit 2b85a34e911bf483c27cfdd124aeb1605145dc80 (net: No more expensive sock_hold()/sock_put() on each tx) sk_free() frees socks conditionally and depends
on sk_wmem_alloc beeing set e.g. in sock_init_data(). But in some

Very nice, but I hope David could fix btw. my "beeing" misspelling.

From: Jarek Poplawski <[email protected]>
Date: Mon, 31 Aug 2009 09:30:19 +0000

Quoted 3 lines · Eric Dumazet

Quoted 2 lines

From: Jarek Poplawski <[email protected]>

Quoted 5 lines

Quoted 4 lines

After commit 2b85a34e911bf483c27cfdd124aeb1605145dc80 (net: No more expensive sock_hold()/sock_put() on each tx) sk_free() frees socks conditionally and depends
on sk_wmem_alloc beeing set e.g. in sock_init_data(). But in some

Very nice, but I hope David could fix btw. my "beeing" misspelling.

I will :-)

Quoted 10 lines · David Miller

From: Jarek Poplawski <[email protected]>
Date: Mon, 31 Aug 2009 09:30:19 +0000

Quoted 7 lines · Eric Dumazet

Quoted 6 lines

From: Jarek Poplawski <[email protected]>

After commit 2b85a34e911bf483c27cfdd124aeb1605145dc80 (net: No more expensive sock_hold()/sock_put() on each tx) sk_free() frees socks conditionally and depends
on sk_wmem_alloc beeing set e.g. in sock_init_data(). But in some

Quoted 3 lines

Quoted 1 line

Very nice, but I hope David could fix btw. my "beeing" misspelling.

I will :-)

Thanks :)