[PATCH] net: Fix sock freeing before sock_init_data() with __sk_free()

[PATCH] net: Fix sock freeing before sock_init_data() with __sk_free()

[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.

Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
---

 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(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index 950409d..e76ef65 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -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,
diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index da0f64f..c05738c 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -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;
 	}
diff --git a/net/core/sock.c b/net/core/sock.c
index bbb25be..92793be 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -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);
diff --git a/net/irda/af_irda.c b/net/irda/af_irda.c
index 50b43c5..9ed2060 100644
--- a/net/irda/af_irda.c
+++ b/net/irda/af_irda.c
@@ -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;
 	}
 
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 1848693..4c8435a 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -228,7 +228,7 @@ static int tipc_create(struct net *net, struct socket *sock, int protocol)
 	tp_ptr = tipc_createport_raw(sk, &dispatch, &wakeupdispatch,
 				     TIPC_LOW_IMPORTANCE);
 	if (unlikely(!tp_ptr)) {
-		sk_free(sk);
+		__sk_free(sk);
 		return -ENOMEM;
 	}
 

Re: [PATCH] net: Fix sock freeing before sock_init_data() with __sk_free()

[Eric Dumazet]

Jarek Poplawski a écrit :
> 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 <jarkao2@gmail.com>
> ---
> 
>  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(-)
> 
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 950409d..e76ef65 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -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,
> diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
> index da0f64f..c05738c 100644
> --- a/net/ax25/af_ax25.c
> +++ b/net/ax25/af_ax25.c
> @@ -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;
>  	}
> diff --git a/net/core/sock.c b/net/core/sock.c
> index bbb25be..92793be 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -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);
> diff --git a/net/irda/af_irda.c b/net/irda/af_irda.c
> index 50b43c5..9ed2060 100644
> --- a/net/irda/af_irda.c
> +++ b/net/irda/af_irda.c
> @@ -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;
>  	}
>  
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index 1848693..4c8435a 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -228,7 +228,7 @@ static int tipc_create(struct net *net, struct socket *sock, int protocol)
>  	tp_ptr = tipc_createport_raw(sk, &dispatch, &wakeupdispatch,
>  				     TIPC_LOW_IMPORTANCE);
>  	if (unlikely(!tp_ptr)) {
> -		sk_free(sk);
> +		__sk_free(sk);
>  		return -ENOMEM;
>  	}
>  
> --

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() ?

Re: [PATCH] net: Fix sock freeing before sock_init_data() with __sk_free()

[Jarek Poplawski]

On Mon, Aug 31, 2009 at 08:26:43AM +0200, Eric Dumazet wrote:
> Jarek Poplawski a écrit :
> > 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() ?
> 

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.)

Jarek P.

Re: [PATCH] net: Fix sock freeing before sock_init_data() with __sk_free()

[Eric Dumazet]

Jarek Poplawski a écrit :
> On Mon, Aug 31, 2009 at 08:26:43AM +0200, Eric Dumazet wrote:
>> Jarek Poplawski a écrit :
>>> 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() ?
>>
> 
> 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()

diff --git a/net/core/sock.c b/net/core/sock.c
index bbb25be..7633422 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -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);

Re: [PATCH] net: Fix sock freeing before sock_init_data() with __sk_free()

[Jarek Poplawski]

On Mon, Aug 31, 2009 at 08:50:25AM +0200, Eric Dumazet wrote:
> Jarek Poplawski a écrit :
> > On Mon, Aug 31, 2009 at 08:26:43AM +0200, Eric Dumazet wrote:
> >> Jarek Poplawski a écrit :
> >>> 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() ?
> >>
> > 
> > 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 <jarkao2@gmail.com>

Thanks,
Jarek P.

> 
> 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()
> 
> diff --git a/net/core/sock.c b/net/core/sock.c
> index bbb25be..7633422 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -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);
> 

Re: [PATCH] net: Fix sock freeing before sock_init_data() with __sk_free()

[Eric Dumazet]

Jarek Poplawski a écrit :
> On Mon, Aug 31, 2009 at 08:50:25AM +0200, Eric Dumazet wrote:
>> Jarek Poplawski a écrit :
>>> On Mon, Aug 31, 2009 at 08:26:43AM +0200, Eric Dumazet wrote:
>>>> Jarek Poplawski a écrit :
>>>>> 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() ?
>>>>
>>> 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 <jarkao2@gmail.com>
> 
> Thanks,
> Jarek P.
> 

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

Thanks

Re: [PATCH] net: Fix sock freeing before sock_init_data() with __sk_free()

[Jarek Poplawski]

On Mon, Aug 31, 2009 at 09:18:46AM +0200, Eric Dumazet wrote:
> Jarek Poplawski a écrit :
> > On Mon, Aug 31, 2009 at 08:50:25AM +0200, Eric Dumazet wrote:
...
> >> 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 <jarkao2@gmail.com>
> > 
> > Thanks,
> > Jarek P.
> > 
> 
> Give me a few hours to review sk_alloc() call sites, test patch and officially submit it.
> 
> Thanks

Hmm... but you're "confident it is OK"?! (You know, I could've risked
my life or something... ;-)

Jarek P.

[PATCH] net: sk_free() should be allowed right after sk_alloc()

[Eric Dumazet]

From: Jarek Poplawski <jarkao2@gmail.com>

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 <jarkao2@gmail.com>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/net/core/sock.c b/net/core/sock.c
index bbb25be..7633422 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -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);

Re: [PATCH] net: sk_free() should be allowed right after sk_alloc()

[Jarek Poplawski]

On Mon, Aug 31, 2009 at 11:15:36AM +0200, Eric Dumazet wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> 
> 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.

Thanks everyone,
Jarek P.

> 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 <jarkao2@gmail.com>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
> diff --git a/net/core/sock.c b/net/core/sock.c
> index bbb25be..7633422 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -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);
> 

Re: [PATCH] net: sk_free() should be allowed right after sk_alloc()

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Mon, 31 Aug 2009 09:30:19 +0000

> On Mon, Aug 31, 2009 at 11:15:36AM +0200, Eric Dumazet wrote:
>> From: Jarek Poplawski <jarkao2@gmail.com>
>> 
>> 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 :-)

Re: [PATCH] net: sk_free() should be allowed right after sk_alloc()

[Eric Dumazet]

David Miller a écrit :
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Mon, 31 Aug 2009 09:30:19 +0000
> 
>> On Mon, Aug 31, 2009 at 11:15:36AM +0200, Eric Dumazet wrote:
>>> From: Jarek Poplawski <jarkao2@gmail.com>
>>>
>>> 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 :-)

Thanks :)

Re: [PATCH] net: sk_free() should be allowed right after sk_alloc()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 31 Aug 2009 14:12:02 +0200

> David Miller a écrit :
>> From: Jarek Poplawski <jarkao2@gmail.com>
>> Date: Mon, 31 Aug 2009 09:30:19 +0000
>> 
>>> On Mon, Aug 31, 2009 at 11:15:36AM +0200, Eric Dumazet wrote:
>>>> From: Jarek Poplawski <jarkao2@gmail.com>
>>>>
>>>> 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 :-)
> 
> Thanks :)

Applied to net-2.6, thanks everyone.