LKML Archive on lore.kernel.org
 help / Atom feed
* RFC: changed error code when binding unix socket twice
@ 2017-06-30  7:34 Michal Kubecek
  2018-08-31 11:14 ` Petr Vorel
  0 siblings, 1 reply; 7+ messages in thread
From: Michal Kubecek @ 2017-06-30  7:34 UTC (permalink / raw)
  To: netdev; +Cc: WANG Cong, Rainer Weikusat, linux-kernel

Hello,

commit 0fb44559ffd6 ("af_unix: move unix_mknod() out of bindlock") moves
the special file creation in unix_bind() before u->bindlock is taken in
order to avoid an ABBA deadlock with do_splice(). As a side effect, it
also moves the check for existence of the special file (which would
result in -EADDRINUSE) before the check of u->addr (which would result
in -EINVAL if socket is already bound). This means that the error
returned for an attempt to bind a unix socket to the same path twice
changed from -EINVAL to -EADDRINUSE with this commit.

One way to restore the old error code is indicated below but before
submitting it, I would like to ask if we need/want it.

Pro:
  - in general, we do not want to change return code for given testcase
  - old error (-EINVAL) is consistent with AF_INET(6)
Con:
  - both POSIX and Linux man page only list error conditions without
    stating which should take precedence if more of them apply so
    neither of them seems wrong, strictly speaking


diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 1a0c961f4ffe..509292bdf7ed 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -992,7 +992,7 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 	struct unix_sock *u = unix_sk(sk);
 	struct sockaddr_un *sunaddr = (struct sockaddr_un *)uaddr;
 	char *sun_path = sunaddr->sun_path;
-	int err;
+	int err, mknod_err;
 	unsigned int hash;
 	struct unix_address *addr;
 	struct hlist_head *list;
@@ -1016,12 +1016,10 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 	if (sun_path[0]) {
 		umode_t mode = S_IFSOCK |
 		       (SOCK_INODE(sock)->i_mode & ~current_umask());
-		err = unix_mknod(sun_path, mode, &path);
-		if (err) {
-			if (err == -EEXIST)
-				err = -EADDRINUSE;
-			goto out;
-		}
+		mknod_err = unix_mknod(sun_path, mode, &path);
+		/* do not exit on error until after u->addr check */
+		if (mknod_err == -EEXIST)
+			mknod_err = -EADDRINUSE;
 	}
 
 	err = mutex_lock_interruptible(&u->bindlock);
@@ -1031,6 +1029,10 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
 	err = -EINVAL;
 	if (u->addr)
 		goto out_up;
+	if (mknod_err) {
+		err = mknod_err;
+		goto out_up;
+	}
 
 	err = -ENOMEM;
 	addr = kmalloc(sizeof(*addr)+addr_len, GFP_KERNEL);

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: RFC: changed error code when binding unix socket twice
  2017-06-30  7:34 RFC: changed error code when binding unix socket twice Michal Kubecek
@ 2018-08-31 11:14 ` Petr Vorel
  2018-10-29 13:03   ` Arnd Bergmann
  0 siblings, 1 reply; 7+ messages in thread
From: Petr Vorel @ 2018-08-31 11:14 UTC (permalink / raw)
  To: Michal Kubecek, David S. Miller
  Cc: netdev, WANG Cong, Rainer Weikusat, linux-kernel, ltp,
	Cyril Hrubis, Junchi Chen

Hi,

> commit 0fb44559ffd6 ("af_unix: move unix_mknod() out of bindlock") moves
> the special file creation in unix_bind() before u->bindlock is taken in
> order to avoid an ABBA deadlock with do_splice(). As a side effect, it
> also moves the check for existence of the special file (which would
> result in -EADDRINUSE) before the check of u->addr (which would result
> in -EINVAL if socket is already bound). This means that the error
> returned for an attempt to bind a unix socket to the same path twice
> changed from -EINVAL to -EADDRINUSE with this commit.

> One way to restore the old error code is indicated below but before
> submitting it, I would like to ask if we need/want it.

> Pro:
>   - in general, we do not want to change return code for given testcase
>   - old error (-EINVAL) is consistent with AF_INET(6)
> Con:
>   - both POSIX and Linux man page only list error conditions without
>     stating which should take precedence if more of them apply so
>     neither of them seems wrong, strictly speaking

I'd be for restoring the original behavior (be conservative + looks like as not intended).

Any comment from netdev maintainers?


Kind regards,
Petr


> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 1a0c961f4ffe..509292bdf7ed 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -992,7 +992,7 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>  	struct unix_sock *u = unix_sk(sk);
>  	struct sockaddr_un *sunaddr = (struct sockaddr_un *)uaddr;
>  	char *sun_path = sunaddr->sun_path;
> -	int err;
> +	int err, mknod_err;
>  	unsigned int hash;
>  	struct unix_address *addr;
>  	struct hlist_head *list;
> @@ -1016,12 +1016,10 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>  	if (sun_path[0]) {
>  		umode_t mode = S_IFSOCK |
>  		       (SOCK_INODE(sock)->i_mode & ~current_umask());
> -		err = unix_mknod(sun_path, mode, &path);
> -		if (err) {
> -			if (err == -EEXIST)
> -				err = -EADDRINUSE;
> -			goto out;
> -		}
> +		mknod_err = unix_mknod(sun_path, mode, &path);
> +		/* do not exit on error until after u->addr check */
> +		if (mknod_err == -EEXIST)
> +			mknod_err = -EADDRINUSE;
>  	}

>  	err = mutex_lock_interruptible(&u->bindlock);
> @@ -1031,6 +1029,10 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
>  	err = -EINVAL;
>  	if (u->addr)
>  		goto out_up;
> +	if (mknod_err) {
> +		err = mknod_err;
> +		goto out_up;
> +	}

>  	err = -ENOMEM;
>  	addr = kmalloc(sizeof(*addr)+addr_len, GFP_KERNEL);

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: RFC: changed error code when binding unix socket twice
  2018-08-31 11:14 ` Petr Vorel
@ 2018-10-29 13:03   ` Arnd Bergmann
  2018-10-29 16:33     ` Petr Vorel
  0 siblings, 1 reply; 7+ messages in thread
From: Arnd Bergmann @ 2018-10-29 13:03 UTC (permalink / raw)
  To: pvorel
  Cc: mkubecek, David Miller, Networking, Cong Wang, rweikusat,
	Linux Kernel Mailing List, ltp, Cyril Hrubis, junchi.chen,
	Dmitry Vyukov, Greg Kroah-Hartman, Naresh Kamboju

On Fri, Aug 31, 2018 at 1:17 PM Petr Vorel <pvorel@suse.cz> wrote:
> > commit 0fb44559ffd6 ("af_unix: move unix_mknod() out of bindlock") moves
> > the special file creation in unix_bind() before u->bindlock is taken in
> > order to avoid an ABBA deadlock with do_splice(). As a side effect, it
> > also moves the check for existence of the special file (which would
> > result in -EADDRINUSE) before the check of u->addr (which would result
> > in -EINVAL if socket is already bound). This means that the error
> > returned for an attempt to bind a unix socket to the same path twice
> > changed from -EINVAL to -EADDRINUSE with this commit.
>
> > One way to restore the old error code is indicated below but before
> > submitting it, I would like to ask if we need/want it.
>
> > Pro:
> >   - in general, we do not want to change return code for given testcase
> >   - old error (-EINVAL) is consistent with AF_INET(6)
> > Con:
> >   - both POSIX and Linux man page only list error conditions without
> >     stating which should take precedence if more of them apply so
> >     neither of them seems wrong, strictly speaking
>
> I'd be for restoring the original behavior (be conservative + looks like as not intended).
>
> Any comment from netdev maintainers?

Naresh noticed that LTP now has a version check to  detect linux-4.10+ and
expect a different return code from previous versions, but the 0fb44559ffd6
commit that changed the behavior got backported to stable linux-4.4 and 4.9,
so now LTP complains about those:

https://bugs.linaro.org/show_bug.cgi?id=4042

I don't care much which error code gets returned here, but I think we
should either handle this consistently in all kernel versions and check for
the one that is deemed the correct one on all versions, or change LTP
again to accept either return code.

        Arnd

> > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> > index 1a0c961f4ffe..509292bdf7ed 100644
> > --- a/net/unix/af_unix.c
> > +++ b/net/unix/af_unix.c
> > @@ -992,7 +992,7 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
> >       struct unix_sock *u = unix_sk(sk);
> >       struct sockaddr_un *sunaddr = (struct sockaddr_un *)uaddr;
> >       char *sun_path = sunaddr->sun_path;
> > -     int err;
> > +     int err, mknod_err;
> >       unsigned int hash;
> >       struct unix_address *addr;
> >       struct hlist_head *list;
> > @@ -1016,12 +1016,10 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
> >       if (sun_path[0]) {
> >               umode_t mode = S_IFSOCK |
> >                      (SOCK_INODE(sock)->i_mode & ~current_umask());
> > -             err = unix_mknod(sun_path, mode, &path);
> > -             if (err) {
> > -                     if (err == -EEXIST)
> > -                             err = -EADDRINUSE;
> > -                     goto out;
> > -             }
> > +             mknod_err = unix_mknod(sun_path, mode, &path);
> > +             /* do not exit on error until after u->addr check */
> > +             if (mknod_err == -EEXIST)
> > +                     mknod_err = -EADDRINUSE;
> >       }
>
> >       err = mutex_lock_interruptible(&u->bindlock);
> > @@ -1031,6 +1029,10 @@ static int unix_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
> >       err = -EINVAL;
> >       if (u->addr)
> >               goto out_up;
> > +     if (mknod_err) {
> > +             err = mknod_err;
> > +             goto out_up;
> > +     }
>
> >       err = -ENOMEM;
> >       addr = kmalloc(sizeof(*addr)+addr_len, GFP_KERNEL);

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: RFC: changed error code when binding unix socket twice
  2018-10-29 13:03   ` Arnd Bergmann
@ 2018-10-29 16:33     ` Petr Vorel
  2018-10-29 20:48       ` Arnd Bergmann
  0 siblings, 1 reply; 7+ messages in thread
From: Petr Vorel @ 2018-10-29 16:33 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: mkubecek, David Miller, Networking, Cong Wang, rweikusat,
	Linux Kernel Mailing List, ltp, Cyril Hrubis, junchi.chen,
	Dmitry Vyukov, Greg Kroah-Hartman, Naresh Kamboju

Hi Arnd,

> On Fri, Aug 31, 2018 at 1:17 PM Petr Vorel <pvorel@suse.cz> wrote:
> > > commit 0fb44559ffd6 ("af_unix: move unix_mknod() out of bindlock") moves
> > > the special file creation in unix_bind() before u->bindlock is taken in
> > > order to avoid an ABBA deadlock with do_splice(). As a side effect, it
> > > also moves the check for existence of the special file (which would
> > > result in -EADDRINUSE) before the check of u->addr (which would result
> > > in -EINVAL if socket is already bound). This means that the error
> > > returned for an attempt to bind a unix socket to the same path twice
> > > changed from -EINVAL to -EADDRINUSE with this commit.

> > > One way to restore the old error code is indicated below but before
> > > submitting it, I would like to ask if we need/want it.

> > > Pro:
> > >   - in general, we do not want to change return code for given testcase
> > >   - old error (-EINVAL) is consistent with AF_INET(6)
> > > Con:
> > >   - both POSIX and Linux man page only list error conditions without
> > >     stating which should take precedence if more of them apply so
> > >     neither of them seems wrong, strictly speaking

> > I'd be for restoring the original behavior (be conservative + looks like as not intended).

> > Any comment from netdev maintainers?

> Naresh noticed that LTP now has a version check to  detect linux-4.10+ and
> expect a different return code from previous versions, but the 0fb44559ffd6
> commit that changed the behavior got backported to stable linux-4.4 and 4.9,
> so now LTP complains about those:

> https://bugs.linaro.org/show_bug.cgi?id=4042
Thanks for report.

> I don't care much which error code gets returned here, but I think we
> should either handle this consistently in all kernel versions and check for
> the one that is deemed the correct one on all versions, or change LTP
> again to accept either return code.
Do you mean to apply this patch to 3.16.y? (The only still maintained LTS branch
which miss this fix). Although the patch don't apply and it's very old branch,
it'd be easy to adjust it and it looks to me deadlock can happen there as well.

I guess we need to adjust LTP test to accept either return code as EOL longterm
branches probably will not take this patch.

>         Arnd

Kind regards,
Petr


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: RFC: changed error code when binding unix socket twice
  2018-10-29 16:33     ` Petr Vorel
@ 2018-10-29 20:48       ` Arnd Bergmann
  2018-11-07 15:56         ` Petr Vorel
  0 siblings, 1 reply; 7+ messages in thread
From: Arnd Bergmann @ 2018-10-29 20:48 UTC (permalink / raw)
  To: pvorel
  Cc: mkubecek, David Miller, Networking, Cong Wang, rweikusat,
	Linux Kernel Mailing List, ltp, Cyril Hrubis, junchi.chen,
	Dmitry Vyukov, gregkh, Naresh Kamboju

On Mon, Oct 29, 2018 at 5:33 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> > On Fri, Aug 31, 2018 at 1:17 PM Petr Vorel <pvorel@suse.cz> wrote:
> > > > commit 0fb44559ffd6 ("af_unix: move unix_mknod() out of bindlock") moves
> > > > the special file creation in unix_bind() before u->bindlock is taken in
> > > > order to avoid an ABBA deadlock with do_splice(). As a side effect, it
> > > > also moves the check for existence of the special file (which would
> > > > result in -EADDRINUSE) before the check of u->addr (which would result
> > > > in -EINVAL if socket is already bound). This means that the error
> > > > returned for an attempt to bind a unix socket to the same path twice
> > > > changed from -EINVAL to -EADDRINUSE with this commit.
>
> > > > One way to restore the old error code is indicated below but before
> > > > submitting it, I would like to ask if we need/want it.
>
> > > > Pro:
> > > >   - in general, we do not want to change return code for given testcase
> > > >   - old error (-EINVAL) is consistent with AF_INET(6)
> > > > Con:
> > > >   - both POSIX and Linux man page only list error conditions without
> > > >     stating which should take precedence if more of them apply so
> > > >     neither of them seems wrong, strictly speaking
>
> > > I'd be for restoring the original behavior (be conservative + looks like as not intended).
>
> > > Any comment from netdev maintainers?
>
> > Naresh noticed that LTP now has a version check to  detect linux-4.10+ and
> > expect a different return code from previous versions, but the 0fb44559ffd6
> > commit that changed the behavior got backported to stable linux-4.4 and 4.9,
> > so now LTP complains about those:
>
> > https://bugs.linaro.org/show_bug.cgi?id=4042
> Thanks for report.
>
> > I don't care much which error code gets returned here, but I think we
> > should either handle this consistently in all kernel versions and check for
> > the one that is deemed the correct one on all versions, or change LTP
> > again to accept either return code.
> Do you mean to apply this patch to 3.16.y? (The only still maintained LTS branch
> which miss this fix). Although the patch don't apply and it's very old branch,
> it'd be easy to adjust it and it looks to me deadlock can happen there as well.

I forgot that 4.1 has ended a while ago. Greg also sometimes still takes patches
for 3.18, so that might be a candidate aside from 3.18

> I guess we need to adjust LTP test to accept either return code as EOL longterm
> branches probably will not take this patch.

I'd argue that if we decide that EADDRINUSE is the intended return value,
it would be appropriate for LTP to warn about kernels that never got the
backport.

The alternative would be to not backport the patch further, and then change LTP
to no longer warn. Note that the bug that got fixed by the 0fb44559ffd6 patch
is probably more important than the return code, so I would say
we want the patch backported to anything that people still run anyway,
especially if they are running LTP to make sure it works correctly.

        Arnd

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: RFC: changed error code when binding unix socket twice
  2018-10-29 20:48       ` Arnd Bergmann
@ 2018-11-07 15:56         ` Petr Vorel
  2018-11-29 12:36           ` gregkh
  0 siblings, 1 reply; 7+ messages in thread
From: Petr Vorel @ 2018-11-07 15:56 UTC (permalink / raw)
  To: gregkh, David Miller
  Cc: mkubecek, Networking, Cong Wang, rweikusat,
	Linux Kernel Mailing List, ltp, Cyril Hrubis, junchi.chen,
	Dmitry Vyukov, Naresh Kamboju, Arnd Bergmann

Hi

> I forgot that 4.1 has ended a while ago. Greg also sometimes still takes patches
> for 3.18, so that might be a candidate aside from 3.18

Gregkh, David, does it make sense to you to merge commit 0fb44559ffd6 ("af_unix:
move unix_mknod() out of bindlock") to 3.18? If yes, please do so.


> > I guess we need to adjust LTP test to accept either return code as EOL longterm
> > branches probably will not take this patch.

> I'd argue that if we decide that EADDRINUSE is the intended return value,
> it would be appropriate for LTP to warn about kernels that never got the
> backport.

> The alternative would be to not backport the patch further, and then change LTP
> to no longer warn. Note that the bug that got fixed by the 0fb44559ffd6 patch
> is probably more important than the return code, so I would say
> we want the patch backported to anything that people still run anyway,
> especially if they are running LTP to make sure it works correctly.

>         Arnd

Kind regards,
Petr

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: RFC: changed error code when binding unix socket twice
  2018-11-07 15:56         ` Petr Vorel
@ 2018-11-29 12:36           ` gregkh
  0 siblings, 0 replies; 7+ messages in thread
From: gregkh @ 2018-11-29 12:36 UTC (permalink / raw)
  To: Petr Vorel
  Cc: David Miller, mkubecek, Networking, Cong Wang, rweikusat,
	Linux Kernel Mailing List, ltp, Cyril Hrubis, junchi.chen,
	Dmitry Vyukov, Naresh Kamboju, Arnd Bergmann

On Wed, Nov 07, 2018 at 04:56:44PM +0100, Petr Vorel wrote:
> Hi
> 
> > I forgot that 4.1 has ended a while ago. Greg also sometimes still takes patches
> > for 3.18, so that might be a candidate aside from 3.18
> 
> Gregkh, David, does it make sense to you to merge commit 0fb44559ffd6 ("af_unix:
> move unix_mknod() out of bindlock") to 3.18? If yes, please do so.

Now queued up, thanks.

greg k-h

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-30  7:34 RFC: changed error code when binding unix socket twice Michal Kubecek
2018-08-31 11:14 ` Petr Vorel
2018-10-29 13:03   ` Arnd Bergmann
2018-10-29 16:33     ` Petr Vorel
2018-10-29 20:48       ` Arnd Bergmann
2018-11-07 15:56         ` Petr Vorel
2018-11-29 12:36           ` gregkh

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox