Re: [PATCH v3 1/4] ovl: use insert_inode_locked4() to hash a newly created inode

[Amir Goldstein <amir73il@gmail.com>]

On Wed, May 16, 2018 at 1:14 PM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> On Wed, May 16, 2018 at 11:51 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>> On Wed, May 16, 2018 at 11:34 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>> On Tue, May 15, 2018 at 3:37 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>> On Tue, May 15, 2018 at 4:23 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>>>>> On Tue, May 15, 2018 at 01:26:09PM +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 patch fixes the race, by using insert_inode_locked4() to add
>>>>>> a newly created inode to icache.
>>>>>>
>>>>>> If the newly created inode apears to already exist in icache (hashed
>>>>>> by the same real upper inode), we export this error to user instead
>>>>>> of silently not hashing the new inode.
>>>>>
>>>>> So we might return an error to user saying operation failed, but still
>>>>> create file on upper. Does that sound little odd?
>>>>>
>>>>
>>>> Yes, but I don't see a better solution.
>>>
>>> Might be better to kick the other, offending inode out, instead of
>>> returning an error.  It would also simplify the error handling.
>>>
>>> We can do that by creating an ovl_inode_test_kick() variant that
>>> unhashes the inode on match.  Also needs insert_inode_locked4() to use
>>> hlist_for_each_entry_safe() instead of hlist_for_each_entry().
>>>
>>
>> Do you really think that this corner use case calls for such actions,
>> as creating flavors of inode cache helpers?
>
> Yes, if it simplifies error handling.
>
>> Remember that the so called "offending" inode, is not offending in
>> a way that is wrong or incomplete in any way.
>
> Right, so what about just using that inode instead of erroring out?
>

OK. I'll see what comes out.

Thanks,
Amir.