net/af_unix: don't create a path for a binded socket

in the case of the socket which is bound to an adress
there is no sense to create a path in the next attempts

here is a program that shows the issue:

int main()
{
    int s;
    struct sockaddr_un a;

    s = socket(AF_UNIX, SOCK_STREAM, 0);
    if (s<0)
        perror("socket() failed\n");

    printf("First bind()\n");

    memset(&a, 0, sizeof(a));
    a.sun_family = AF_UNIX;
    strncpy(a.sun_path, "/tmp/.first_bind", sizeof(a.sun_path));

    if ((bind(s, (const struct sockaddr*) &a, sizeof(a))) == -1)
        perror("bind() failed\n");

    printf("Second bind()\n");

    memset(&a, 0, sizeof(a));
    a.sun_family = AF_UNIX;
    strncpy(a.sun_path, "/tmp/.first_bind_failed", sizeof(a.sun_path));

    if ((bind(s, (const struct sockaddr*) &a, sizeof(a))) == -1)
        perror("bind() failed\n");
}

kda@SLES15-SP2:~> ./test
First bind()
Second bind()
bind() failed
: Invalid argument

kda@SLES15-SP2:~> ls -la /tmp/.first_bind
.first_bind         .first_bind_failed

Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
---
 net/unix/af_unix.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

On Tue, 24 Nov 2020 15:24:21 +0300 Denis Kirjanov wrote:
> in the case of the socket which is bound to an adress
> there is no sense to create a path in the next attempts

> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 41c3303c3357..fd76a8fe3907 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -1021,7 +1021,7 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>  
>  	err = -EINVAL;
>  	if (addr_len < offsetofend(struct sockaddr_un, sun_family) ||
> -	    sunaddr->sun_family != AF_UNIX)
> +	    sunaddr->sun_family != AF_UNIX || u->addr)
>  		goto out;
>  
>  	if (addr_len == sizeof(short)) {
> @@ -1049,10 +1049,6 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>  	if (err)
>  		goto out_put;
>  
> -	err = -EINVAL;
> -	if (u->addr)
> -		goto out_up;
> -
>  	err = -ENOMEM;
>  	addr = kmalloc(sizeof(*addr)+addr_len, GFP_KERNEL);
>  	if (!addr)

Well, after your change the check on u->addr is no longer protected by
u->bindlock. Is that okay?

On 11/26/20, Jakub Kicinski <kuba@kernel.org> wrote:
> On Tue, 24 Nov 2020 15:24:21 +0300 Denis Kirjanov wrote:
>> in the case of the socket which is bound to an adress
>> there is no sense to create a path in the next attempts
>
>> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
>> index 41c3303c3357..fd76a8fe3907 100644
>> --- a/net/unix/af_unix.c
>> +++ b/net/unix/af_unix.c
>> @@ -1021,7 +1021,7 @@ static int unix_bind(struct socket *sock, struct
>> sockaddr *uaddr, int addr_len)
>>
>>  	err = -EINVAL;
>>  	if (addr_len < offsetofend(struct sockaddr_un, sun_family) ||
>> -	    sunaddr->sun_family != AF_UNIX)
>> +	    sunaddr->sun_family != AF_UNIX || u->addr)
>>  		goto out;
>>
>>  	if (addr_len == sizeof(short)) {
>> @@ -1049,10 +1049,6 @@ static int unix_bind(struct socket *sock, struct
>> sockaddr *uaddr, int addr_len)
>>  	if (err)
>>  		goto out_put;
>>
>> -	err = -EINVAL;
>> -	if (u->addr)
>> -		goto out_up;
>> -
>>  	err = -ENOMEM;
>>  	addr = kmalloc(sizeof(*addr)+addr_len, GFP_KERNEL);
>>  	if (!addr)
>
> Well, after your change the check on u->addr is no longer protected by
> u->bindlock. Is that okay?

Since we're just checking the assigned address and it's an atomic
operation I think it's okay.
A process performing binding is still protected.

Thanks!


>

On Thu, 26 Nov 2020 17:22:08 +0300 Denis Kirjanov wrote:
> On 11/26/20, Jakub Kicinski <kuba@kernel.org> wrote:
> > On Tue, 24 Nov 2020 15:24:21 +0300 Denis Kirjanov wrote:  
> >> in the case of the socket which is bound to an adress
> >> there is no sense to create a path in the next attempts  
> >  
> >> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> >> index 41c3303c3357..fd76a8fe3907 100644
> >> --- a/net/unix/af_unix.c
> >> +++ b/net/unix/af_unix.c
> >> @@ -1021,7 +1021,7 @@ static int unix_bind(struct socket *sock, struct
> >> sockaddr *uaddr, int addr_len)
> >>
> >>  	err = -EINVAL;
> >>  	if (addr_len < offsetofend(struct sockaddr_un, sun_family) ||
> >> -	    sunaddr->sun_family != AF_UNIX)
> >> +	    sunaddr->sun_family != AF_UNIX || u->addr)
> >>  		goto out;
> >>
> >>  	if (addr_len == sizeof(short)) {
> >> @@ -1049,10 +1049,6 @@ static int unix_bind(struct socket *sock, struct
> >> sockaddr *uaddr, int addr_len)
> >>  	if (err)
> >>  		goto out_put;
> >>
> >> -	err = -EINVAL;
> >> -	if (u->addr)
> >> -		goto out_up;
> >> -
> >>  	err = -ENOMEM;
> >>  	addr = kmalloc(sizeof(*addr)+addr_len, GFP_KERNEL);
> >>  	if (!addr)  
> >
> > Well, after your change the check on u->addr is no longer protected by
> > u->bindlock. Is that okay?  
> 
> Since we're just checking the assigned address and it's an atomic
> operation I think it's okay.

The access to the variable may be atomic, but what protects two
concurrent binds() from progressing past the check and binding to
different paths?

I don't know this code at all, but looks to me like the pattern is
basically:

	lock()
	if (obj->thing)
		goto err; /* already bound to a thing */

	thing = alloc()
	setup_thing(thing);

	obj->thing = thing;
err:
	unlock()

> A process performing binding is still protected.

Isn't checking "did someone already bind" not part of the process of
binding?