From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1526379972-20923-1-git-send-email-amir73il@gmail.com> <1526379972-20923-2-git-send-email-amir73il@gmail.com> <20180515132328.GA11678@redhat.com> From: Amir Goldstein Date: Wed, 16 May 2018 14:03:52 +0300 Message-ID: Subject: Re: [PATCH v3 1/4] ovl: use insert_inode_locked4() to hash a newly created inode To: Miklos Szeredi Cc: Vivek Goyal , Al Viro , overlayfs , linux-fsdevel , "#v4 . 16" Content-Type: text/plain; charset="UTF-8" Sender: stable-owner@vger.kernel.org List-ID: On Wed, May 16, 2018 at 1:14 PM, Miklos Szeredi wrote: > On Wed, May 16, 2018 at 11:51 AM, Amir Goldstein wrote: >> On Wed, May 16, 2018 at 11:34 AM, Miklos Szeredi wrote: >>> On Tue, May 15, 2018 at 3:37 PM, Amir Goldstein wrote: >>>> On Tue, May 15, 2018 at 4:23 PM, Vivek Goyal 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.