Re: [PATCH v4 9/9] ovl: use iget5_prealloc() to hash a newly created inode

[Amir Goldstein <amir73il@gmail.com>]

On Fri, May 18, 2018 at 6:57 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> 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()).
>

Yeh, using alloc/destroy_inode() isn't pretty.
I'll let you untangle iget5_prealloc() and inode_sb_list.

Let me know if you have something to test.

Thanks,
Amir.