| Message ID | 20201124122421.9859-1-kda@linux-powerpc.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/af_unix: don't create a path for a binded socket | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 4 this patch: 4 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: 'adress' may be misspelled - perhaps 'address'? |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 4 this patch: 4 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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?
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)
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(-)