Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1309459 > unrolled thread

[PATCH] af_unix: Use kfree for addresses in unix_bind

Back to article view | Back to linux.kernel

Contents

  [PATCH] af_unix: Use kfree for addresses in unix_bind Rainer Weikusat <rweikusat@mobileactivedefense.com> - 2016-01-14 17:30 +0100
    Re: [PATCH] af_unix: Use kfree for addresses in unix_bind Eric Dumazet <eric.dumazet@gmail.com> - 2016-01-14 19:10 +0100
      Re: [PATCH] af_unix: Use kfree for addresses in unix_bind Rainer Weikusat <rweikusat@mobileactivedefense.com> - 2016-01-14 20:10 +0100

#1309459 — [PATCH] af_unix: Use kfree for addresses in unix_bind

Use kfree instead of unix_release_addr when freeing newly-allocated
unix_address structures after binding the socket failed. The second
function does an atomic_dec_and_test in order to free the address once
its reference count falls to zero which isn't necessary for the
unix_bind error path as the new structure wasn't published yet. 'Using
kfree' is also how unix_autobind handles this case.

Signed-off-by: Rainer Weikusat <rweikusat@mobileactivedefense.com>
---
gdiff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index c5bf5ef..b894a3c 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1044,7 +1044,7 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 		if (err) {
 			if (err == -EEXIST)
 				err = -EADDRINUSE;
-			unix_release_addr(addr);
+			kfree(addr);
 			goto out_up;
 		}
 		addr->hash = UNIX_HASH_SIZE;
@@ -1057,7 +1057,7 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 		err = -EADDRINUSE;
 		if (__unix_find_socket_byname(net, sunaddr, addr_len,
 					      sk->sk_type, hash)) {
-			unix_release_addr(addr);
+			kfree(addr);
 			goto out_unlock;
 		}
 

[toc] | [next] | [standalone]

#1309554

On Thu, 2016-01-14 at 16:22 +0000, Rainer Weikusat wrote:
> Use kfree instead of unix_release_addr when freeing newly-allocated
> unix_address structures after binding the socket failed. The second
> function does an atomic_dec_and_test in order to free the address once
> its reference count falls to zero which isn't necessary for the
> unix_bind error path as the new structure wasn't published yet. 'Using
> kfree' is also how unix_autobind handles this case.
> 
> Signed-off-by: Rainer Weikusat <rweikusat@mobileactivedefense.com>
> ---

This looks net-next material ?

net-next tree is closed during merge window.

Not sure what you gain by optimizing error paths ...

[toc] | [prev] | [next] | [standalone]

#1309582

Eric Dumazet <eric.dumazet@gmail.com> writes:
> On Thu, 2016-01-14 at 16:22 +0000, Rainer Weikusat wrote:
>> Use kfree instead of unix_release_addr when freeing newly-allocated
>> unix_address structures after binding the socket failed. The second
>> function does an atomic_dec_and_test in order to free the address once
>> its reference count falls to zero which isn't necessary for the
>> unix_bind error path as the new structure wasn't published yet. 'Using
>> kfree' is also how unix_autobind handles this case.
>> 
>> Signed-off-by: Rainer Weikusat <rweikusat@mobileactivedefense.com>
>> ---
>
> This looks net-next material ?

The patch was against net-next. But it's not exactly a new feature.

> net-next tree is closed during merge window.

Sorry, I didn't know that.

> Not sure what you gain by optimizing error paths ...

What does the error path gain by being differently implemented in
two functionally closely related functions (unix_bind and unix_autobind)?
I already lost the (small amount of) time it took to determine that
there's no reason why unix_release_addr should be called in one case but
not in the other. But this may well help someone else avoid the effort
in future.

With a little more perspective: Something I'd like to do is to shrink
the u->readlock protected critical sections in _bind and _autobind
somewhat. At least the memory allocations certainly don't need to be
protected by the lock. This would mean moving the allocation code and
consequently, the freeing code, too. And changing the code of one
function but keeping the unix_release_lock, perhaps with a comment a la

/* This is useless. But it always grew here. */

while doing the same with the kfree in the other just felt too bizarre.

NB: That's "my" answer: It adds entropy to the code for no gain. One
could also argue that the error path shouldn't execute atomic
instructions for no purpose.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web