All of lore.kernel.org
 help / color / mirror / Atom feed
From: Petr Vorel <pvorel@suse.cz>
To: ltp@lists.linux.it
Subject: [LTP] RFC: changed error code when binding unix socket twice
Date: Fri, 31 Aug 2018 13:14:36 +0200	[thread overview]
Message-ID: <20180831111436.GA23780@dell5510> (raw)
In-Reply-To: <20170630073448.GA9546@unicorn.suse.cz>

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);

WARNING: multiple messages have this Message-ID (diff)
From: Petr Vorel <pvorel@suse.cz>
To: Michal Kubecek <mkubecek@suse.cz>,
	"David S. Miller" <davem@davemloft.net>
Cc: netdev@vger.kernel.org, WANG Cong <xiyou.wangcong@gmail.com>,
	Rainer Weikusat <rweikusat@mobileactivedefense.com>,
	linux-kernel@vger.kernel.org, ltp@lists.linux.it,
	Cyril Hrubis <chrubis@suse.cz>,
	Junchi Chen <junchi.chen@intel.com>
Subject: Re: RFC: changed error code when binding unix socket twice
Date: Fri, 31 Aug 2018 13:14:36 +0200	[thread overview]
Message-ID: <20180831111436.GA23780@dell5510> (raw)
In-Reply-To: <20170630073448.GA9546@unicorn.suse.cz>

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);

  reply	other threads:[~2018-08-31 11:14 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-06-30  7:34 RFC: changed error code when binding unix socket twice Michal Kubecek
2018-08-31 11:14 ` Petr Vorel [this message]
2018-08-31 11:14   ` Petr Vorel
2018-10-29 13:03   ` [LTP] " Arnd Bergmann
2018-10-29 13:03     ` Arnd Bergmann
2018-10-29 16:33     ` [LTP] " Petr Vorel
2018-10-29 16:33       ` Petr Vorel
2018-10-29 20:48       ` [LTP] " Arnd Bergmann
2018-10-29 20:48         ` Arnd Bergmann
2018-11-07 15:56         ` [LTP] " Petr Vorel
2018-11-07 15:56           ` Petr Vorel
2018-11-29 12:36           ` [LTP] " gregkh
2018-11-29 12:36             ` gregkh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180831111436.GA23780@dell5510 \
    --to=pvorel@suse.cz \
    --cc=ltp@lists.linux.it \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.