On Wed, May 16, 2018 at 11:34 AM, Miklos Szeredi <miklos@xxxxxxxxxx> wrote: > On Tue, May 15, 2018 at 3:37 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote: >> On Tue, May 15, 2018 at 4:23 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> 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? Remember that the so called "offending" inode, is not offending in a way that is wrong or incomplete in any way. So worst worst worst case this is a very temporal error. Thanks, Amir.
