[PATCH net] netlink: fix boolean evaluation on bound

[PATCH net] netlink: fix boolean evaluation on bound

[Hannes Frederic Sowa]

portid may be 0, thus bound will set the flag to false for in-kernel
created netlink sockets.

Fixes: da314c9923fed55 ("netlink: Replace rhash_portid with bound")
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
This patch should not affect anything and is just meant to close this
loophole in future. I based it on net, but you can also apply it to
net-next.

 net/netlink/af_netlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 59651af8cc2705..278e94c3c7f6d1 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1137,7 +1137,7 @@ static int netlink_insert(struct sock *sk, u32 portid)
 
 	/* We need to ensure that the socket is hashed and visible. */
 	smp_wmb();
-	nlk_sk(sk)->bound = portid;
+	nlk_sk(sk)->bound = true;
 
 err:
 	release_sock(sk);
-- 
2.5.0

Re: [PATCH net] netlink: fix boolean evaluation on bound

[Herbert Xu]

On Mon, Dec 14, 2015 at 05:55:25PM +0100, Hannes Frederic Sowa wrote:
> portid may be 0, thus bound will set the flag to false for in-kernel
> created netlink sockets.
> 
> Fixes: da314c9923fed55 ("netlink: Replace rhash_portid with bound")
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> ---
> This patch should not affect anything and is just meant to close this
> loophole in future. I based it on net, but you can also apply it to
> net-next.

Nack.  The bound field only needs to be true for user-space sockets.
So please explain why you need it to be true for kernel sockets.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net] netlink: fix boolean evaluation on bound

[Hannes Frederic Sowa]

On 14.12.2015 18:06, Herbert Xu wrote:
> On Mon, Dec 14, 2015 at 05:55:25PM +0100, Hannes Frederic Sowa wrote:
>> portid may be 0, thus bound will set the flag to false for in-kernel
>> created netlink sockets.
>>
>> Fixes: da314c9923fed55 ("netlink: Replace rhash_portid with bound")
>> Cc: Herbert Xu <herbert@gondor.apana.org.au>
>> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
>> ---
>> This patch should not affect anything and is just meant to close this
>> loophole in future. I based it on net, but you can also apply it to
>> net-next.
> 
> Nack.  The bound field only needs to be true for user-space sockets.
> So please explain why you need it to be true for kernel sockets.

I reviewed this very carefully and think this is currently a matter of
taste as it does not change current logic.

Otherwise I would recommend adding a "!!" to express that we actually
want bound set based on the portid value?

Bye,
Hannes

Re: [PATCH net] netlink: fix boolean evaluation on bound

[Eric Dumazet]

On Mon, 2015-12-14 at 18:27 +0100, Hannes Frederic Sowa wrote:

> Otherwise I would recommend adding a "!!" to express that we actually
> want bound set based on the portid value?

Note that compiler already does the !! thing, when you store an integer
into a bool var.

So it definitely is a matter of taste and code readability.

Re: [PATCH net] netlink: fix boolean evaluation on bound

[Hannes Frederic Sowa]

On 14.12.2015 18:50, Eric Dumazet wrote:
> On Mon, 2015-12-14 at 18:27 +0100, Hannes Frederic Sowa wrote:
> 
>> Otherwise I would recommend adding a "!!" to express that we actually
>> want bound set based on the portid value?
> 
> Note that compiler already does the !! thing, when you store an integer
> into a bool var.

Exactly, the gcc compiler does that automatically when writing to a
_Bool type.

> So it definitely is a matter of taste and code readability.

Yes, as I wrote, this patch doesn't alter the any runtime behavior, it
confused me and thus I wanted to make the assignment more explicit.

It looked like a trap to me, thus I wanted to change it.

Bye,
Hannes