| Message ID | 20201203081844.3205-1-kda@linux-powerpc.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v4] net/af_unix: don't create a path for a bound 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'? WARNING: Duplicate signature |
| 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 Thu, 3 Dec 2020 11:18:44 +0300 Denis Kirjanov wrote: > in the case of a socket which is already 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..489d49a1739c 100644 > --- a/net/unix/af_unix.c > +++ b/net/unix/af_unix.c > @@ -1029,6 +1029,16 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len) > goto out; > } > > + /* check if we're already bound to a path */ > + err = mutex_lock_interruptible(&u->bindlock); > + if (err) > + goto out; > + if (u->addr) > + err = -EINVAL; > + mutex_unlock(&u->bindlock); This, like v1, is not atomic with the creation, two threads can pass this check and then try to assign to u->addr. Naive question - can't we add the removal of the unnecessary node in the error path, in the kernel? > + if (err) > + goto out; > + > err = unix_mkname(sunaddr, addr_len, &hash); > if (err < 0) > goto out; > @@ -1049,10 +1059,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)
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 41c3303c3357..489d49a1739c 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -1029,6 +1029,16 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len) goto out; } + /* check if we're already bound to a path */ + err = mutex_lock_interruptible(&u->bindlock); + if (err) + goto out; + if (u->addr) + err = -EINVAL; + mutex_unlock(&u->bindlock); + if (err) + goto out; + err = unix_mkname(sunaddr, addr_len, &hash); if (err < 0) goto out; @@ -1049,10 +1059,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 a socket which is already 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> v2: move a new path creation after the address assignment check v3: fixed goto labels on the error path v4: check the assigned address with bindlock held Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org> --- net/unix/af_unix.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)