* [PATCH] net/af_unix: don't create a path for a binded socket
@ 2020-11-24 12:24 Denis Kirjanov
2020-11-25 23:27 ` Jakub Kicinski
0 siblings, 1 reply; 4+ messages in thread
From: Denis Kirjanov @ 2020-11-24 12:24 UTC (permalink / raw)
To: netdev; +Cc: kuba, davem
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(-)
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)
--
2.16.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] net/af_unix: don't create a path for a binded socket
2020-11-24 12:24 [PATCH] net/af_unix: don't create a path for a binded socket Denis Kirjanov
@ 2020-11-25 23:27 ` Jakub Kicinski
2020-11-26 14:22 ` Denis Kirjanov
0 siblings, 1 reply; 4+ messages in thread
From: Jakub Kicinski @ 2020-11-25 23:27 UTC (permalink / raw)
To: Denis Kirjanov; +Cc: netdev, davem
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?
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] net/af_unix: don't create a path for a binded socket
2020-11-25 23:27 ` Jakub Kicinski
@ 2020-11-26 14:22 ` Denis Kirjanov
2020-11-27 22:48 ` Jakub Kicinski
0 siblings, 1 reply; 4+ messages in thread
From: Denis Kirjanov @ 2020-11-26 14:22 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, davem
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!
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] net/af_unix: don't create a path for a binded socket
2020-11-26 14:22 ` Denis Kirjanov
@ 2020-11-27 22:48 ` Jakub Kicinski
0 siblings, 0 replies; 4+ messages in thread
From: Jakub Kicinski @ 2020-11-27 22:48 UTC (permalink / raw)
To: Denis Kirjanov; +Cc: netdev, davem
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?
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-11-27 22:51 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-24 12:24 [PATCH] net/af_unix: don't create a path for a binded socket Denis Kirjanov
2020-11-25 23:27 ` Jakub Kicinski
2020-11-26 14:22 ` Denis Kirjanov
2020-11-27 22:48 ` Jakub Kicinski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).