| Message ID | 20201201093306.32638-1-kda@linux-powerpc.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v3] 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, 1 Dec 2020 12:33:06 +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 > > 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> Is the deadlock fixed by the patch Michal pointed out no longer present?
On 12/1/20, Jakub Kicinski <kuba@kernel.org> wrote: > On Tue, 1 Dec 2020 12:33:06 +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 >> >> 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> > > Is the deadlock fixed by the patch Michal pointed out no longer present? The thing that I'm not satisfied is that we're holding bindlock during unix_mknod(). I'll send a next version >
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index 41c3303c3357..70861e9bcfd9 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -1034,6 +1034,14 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len) goto out; addr_len = err; + err = mutex_lock_interruptible(&u->bindlock); + if (err) + goto out; + + err = -EINVAL; + if (u->addr) + goto out_up; + if (sun_path[0]) { umode_t mode = S_IFSOCK | (SOCK_INODE(sock)->i_mode & ~current_umask()); @@ -1041,22 +1049,14 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len) if (err) { if (err == -EEXIST) err = -EADDRINUSE; - goto out; + goto out_up; } } - err = mutex_lock_interruptible(&u->bindlock); - 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) - goto out_up; + goto out_put; memcpy(addr->name, sunaddr, addr_len); addr->len = addr_len; @@ -1088,11 +1088,11 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len) out_unlock: spin_unlock(&unix_table_lock); -out_up: - mutex_unlock(&u->bindlock); out_put: if (err) path_put(&path); +out_up: + mutex_unlock(&u->bindlock); out: return err; }
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> v2: move a new path creation after the address assignment check v3: fixed goto labels on the error path --- net/unix/af_unix.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)