linux-kernel - Re: [PATCH] af_unix: Use kfree for addresses in unix

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87lh7svv69.fsf@doppelsaurus.mobileactivedefense.com>
Date:	Thu, 14 Jan 2016 19:05:18 +0000
From:	Rainer Weikusat <rweikusat@...ileactivedefense.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	davem@...emloft.net, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] af_unix: Use kfree for addresses in unix_bind

Eric Dumazet <eric.dumazet@...il.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@...ileactivedefense.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.