Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
In-Reply-To: <YClpVIfHYyzd6EWu@zeniv-ca.linux.org.uk>
References: <20210125154937.26479-1-kda@linux-powerpc.org> <20210127175742.GA1744861@infradead.org>
 <CAOJe8K0MC-TCURE2Gpci1SLnLXCbUkE7q6SS0fznzBA+Pf-B8Q@mail.gmail.com>
 <20210129082524.GA2282796@infradead.org> <CAOJe8K0iG91tm8YBRmE_rdMMMbc4iRsMGYNxJk0p9vEedNHEkg@mail.gmail.com>
 <20210129131855.GA2346744@infradead.org> <YClpVIfHYyzd6EWu@zeniv-ca.linux.org.uk>
From:   Denis Kirjanov <kda@linux-powerpc.org>
Date:   Tue, 16 Feb 2021 17:31:33 +0300
Message-ID: <CAOJe8K00srtuD+VAJOFcFepOqgNUm0mC8C=hLq2=qhUFSfhpuw@mail.gmail.com>
Subject: Re: [PATCH] fs: export kern_path_locked
To:     Al Viro <viro@zeniv.linux.org.uk>
Cc:     Christoph Hellwig <hch@infradead.org>,
        linux-kernel@vger.kernel.org, Jakub Kicinski <kuba@kernel.org>,
        linux-fsdevel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On 2/14/21, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Fri, Jan 29, 2021 at 01:18:55PM +0000, Christoph Hellwig wrote:
>> On Fri, Jan 29, 2021 at 04:11:05PM +0300, Denis Kirjanov wrote:
>> > Do you mean just:
>>
>> We'll still need to lock the parent inode.
>
> Not just "lock", we wouldd need to have the lock _held_ across the
> entire sequence.  Without that there's no warranty that it will refer
> to the same object we'd created.
>
> In any case, unlink in any potentially public area is pretty much
> never the right approach.  Once mknod has happened, that's it - too
> late to bail out.
>
> IIRC, most of the PITA in that area is due to unix_autobind()
> iteractions.  Basically, we try to bind() an unbound socket and
> another thread does sendmsg() on the same while we are in the
> middle of ->mknod().  Who should wait for whom?
>
> ->mknod() really should be a point of no return - any games with
> "so we unlink it" are unreliable in the best case, and that's
> only if we do _not_ unlock the parent through the entire sequence.
>
> Seeing that we have separate bindlock and iolock now...  How about
> this (completely untested) delta?

We had a change like that:
Author: WANG Cong <xiyou.wangcong@gmail.com>
Date:   Mon Jan 23 11:17:35 2017 -0800

    af_unix: move unix_mknod() out of bindlock

    Dmitry reported a deadlock scenario:

    unix_bind() path:
    u->bindlock ==> sb_writer

    do_splice() path:
    sb_writer ==> pipe->mutex ==> u->bindlock

    In the unix_bind() code path, unix_mknod() does not have to
    be done with u->bindlock held, since it is a pure fs operation,
    so we can just move unix_mknod() out.


>
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 41c3303c3357..c21038b15836 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,18 +1049,10 @@ 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)
> @@ -1090,7 +1090,6 @@ static int unix_bind(struct socket *sock, struct
> sockaddr *uaddr, int addr_len)
>  	spin_unlock(&unix_table_lock);
>  out_up:
>  	mutex_unlock(&u->bindlock);
> -out_put:
>  	if (err)
>  		path_put(&path);
>  out:
>