From mboxrd@z Thu Jan  1 00:00:00 1970
MIME-Version: 1.0
In-Reply-To: <CAOQ4uxg54AedZZtDpkGrinUcOt9kiKoG2s9+RAjz0f+2CGVPrg@mail.gmail.com>
References: <1526632177-28832-1-git-send-email-amir73il@gmail.com>
	<1526632177-28832-10-git-send-email-amir73il@gmail.com>
	<20180518150353.GS30522@ZenIV.linux.org.uk>
	<CAJfpegvAx1yNkDJNcCa-YZOCpz1OgeWK8SE341-iE8L5E8wsKw@mail.gmail.com>
	<CAOQ4uxg54AedZZtDpkGrinUcOt9kiKoG2s9+RAjz0f+2CGVPrg@mail.gmail.com>
Date: Fri, 18 May 2018 17:57:08 +0200
Message-ID: <CAJfpegtWNh8gQ+-vG0hsNWiW9FKH7u9C+HGig35CD2CdPzHf4w@mail.gmail.com>
Subject: Re: [PATCH v4 9/9] ovl: use iget5_prealloc() to hash a newly created inode
From: Miklos Szeredi <miklos@szeredi.hu>
Content-Type: text/plain; charset="UTF-8"
To: Amir Goldstein <amir73il@gmail.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>, Vivek Goyal <vgoyal@redhat.com>, overlayfs <linux-unionfs@vger.kernel.org>
List-ID: <linux-unionfs@vger.kernel.org>

On Fri, May 18, 2018 at 5:36 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Fri, May 18, 2018 at 6:11 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Fri, May 18, 2018 at 5:03 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>>> On Fri, May 18, 2018 at 11:29:37AM +0300, Amir Goldstein wrote:
>>>> Currently, there is a small window where ovl_obtain_alias() can
>>>> race with ovl_instantiate() and create two different overlay inodes
>>>> with the same underlying real non-dir non-hardlink inode.
>>>>
>>>> The race requires an adversary to guess the file handle of the
>>>> yet to be created upper inode and decode the guessed file handle
>>>> after ovl_creat_real(), but before ovl_instantiate().
>>>> This race does not affect overlay directory inodes, because those
>>>> are decoded via ovl_lookup_real() and not with ovl_obtain_alias().
>>>>
>>>> This patch fixes the race, by using iget5_prealloc() to add a newly
>>>> created inode to cache.
>>>
>>> Mind explaining what the hell is wrong with insert_inode_locked4()?
>>
>> That it doesn't return the old inode if found.
>>
>
> FYI, I have set a side a version I was working on before iget5_prealloc()
> that uses insert_inode_locked5 (runner up for ugliest function name):
>
> +int insert_inode_locked4(struct inode *inode, unsigned long hashval,
> +               int (*test)(struct inode *, void *), void *data)
> +{
> +       struct inode *old = insert_inode_locked5(inode, hashval, test, data);
>
> +       if (old) {
> +               iput(old);
> +               return -EBUSY;
> +       }
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(insert_inode_locked4);

Can do exact same thing with iget5_prealloc(), just need to move
inode_sb_list_add() out to iget5_locked() (meaning, overlayfs can
continue to use new_inode()/iput() instead of having to do
alloc/destroy_inode()).

Thanks,
Miklos