| Message ID | 1526379972-20923-2-git-send-email-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? I am wondering why can't we call ovl_get_inode() in object creation path. That should take care of race between creation path and file handle decode and only one of the paths will get to instantiate and initialize ovl_inode and other path will wait. Thanks Vivek > > This race does not affect overlay directory inodes, because those > are decoded via ovl_lookup_real() and not with ovl_obtain_alias(), > so avoid using the new helper d_instantiate_new() to reduce backport > dependencies. > > Backporting only makes sense for v4.16 where NFS export was introduced. > > Cc: Al Viro <viro@zeniv.linux.org.uk> > Cc: <stable@vger.kernel.org> #v4.16 > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > fs/overlayfs/dir.c | 24 ++++++++++++++++++------ > fs/overlayfs/inode.c | 18 ++++++++++++++++++ > fs/overlayfs/overlayfs.h | 1 + > 3 files changed, 37 insertions(+), 6 deletions(-) > > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c > index 47dc980e8b33..62e6733b755c 100644 > --- a/fs/overlayfs/dir.c > +++ b/fs/overlayfs/dir.c > @@ -183,14 +183,24 @@ static int ovl_set_opaque(struct dentry *dentry, struct dentry *upperdentry) > } > > /* Common operations required to be done after creation of file on upper */ > -static void ovl_instantiate(struct dentry *dentry, struct inode *inode, > - struct dentry *newdentry, bool hardlink) > +static int ovl_instantiate(struct dentry *dentry, struct inode *inode, > + struct dentry *newdentry, bool hardlink) > { > ovl_dir_modified(dentry->d_parent, false); > - ovl_copyattr(d_inode(newdentry), inode); > ovl_dentry_set_upper_alias(dentry); > if (!hardlink) { > - ovl_inode_update(inode, newdentry); > + int err; > + > + ovl_inode_init(inode, newdentry, NULL); > + /* > + * XXX: if we ever use ovl_obtain_alias() to decode directory > + * file handles, need to use ovl_insert_inode_locked() and > + * d_instantiate_new() here to prevent ovl_obtain_alias() > + * from sneaking in before d_instantiate(). > + */ > + err = ovl_insert_inode(inode, d_inode(newdentry)); > + if (err) > + return err; > } else { > WARN_ON(ovl_inode_real(inode) != d_inode(newdentry)); > dput(newdentry); > @@ -200,6 +210,8 @@ static void ovl_instantiate(struct dentry *dentry, struct inode *inode, > /* Force lookup of new upper hardlink to find its lower */ > if (hardlink) > d_drop(dentry); > + > + return 0; > } > > static bool ovl_type_merge(struct dentry *dentry) > @@ -238,7 +250,7 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode, > ovl_set_opaque(dentry, newdentry); > } > > - ovl_instantiate(dentry, inode, newdentry, !!hardlink); > + err = ovl_instantiate(dentry, inode, newdentry, !!hardlink); > newdentry = NULL; > out_dput: > dput(newdentry); > @@ -439,7 +451,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode, > if (err) > goto out_cleanup; > } > - ovl_instantiate(dentry, inode, newdentry, !!hardlink); > + err = ovl_instantiate(dentry, inode, newdentry, !!hardlink); > newdentry = NULL; > out_dput2: > dput(upper); > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c > index 7abcf96e94fc..060c534998d1 100644 > --- a/fs/overlayfs/inode.c > +++ b/fs/overlayfs/inode.c > @@ -741,6 +741,24 @@ static bool ovl_verify_inode(struct inode *inode, struct dentry *lowerdentry, > return true; > } > > +static int ovl_insert_inode_locked(struct inode *inode, struct inode *realinode) > +{ > + return insert_inode_locked4(inode, (unsigned long) realinode, > + ovl_inode_test, realinode); > +} > + > +int ovl_insert_inode(struct inode *inode, struct inode *realinode) > +{ > + int err; > + > + err = ovl_insert_inode_locked(inode, realinode); > + if (err) > + return err; > + > + unlock_new_inode(inode); > + return 0; > +} > + > struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real, > bool is_upper) > { > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h > index caaa47cea2aa..642b25702092 100644 > --- a/fs/overlayfs/overlayfs.h > +++ b/fs/overlayfs/overlayfs.h > @@ -343,6 +343,7 @@ int ovl_update_time(struct inode *inode, struct timespec *ts, int flags); > bool ovl_is_private_xattr(const char *name); > > struct inode *ovl_new_inode(struct super_block *sb, umode_t mode, dev_t rdev); > +int ovl_insert_inode(struct inode *inode, struct inode *realinode); > struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real, > bool is_upper); > struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry, > -- > 2.7.4 >
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. > I am wondering why can't we call ovl_get_inode() in object creation > path. That should take care of race between creation path and file > handle decode and only one of the paths will get to instantiate and > initialize ovl_inode and other path will wait. > I don't even want to think if what you wrote makes sense. Remember that the use case we are talking about is quite imaginary. Ensuring internal structures consistency in our code and returning error to user is the right thing to do for imaginary use cases IMO. Thanks, Amir.
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(). Thanks, Miklos
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? 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.
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? Thanks, Miklos
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.
On Tue, May 15, 2018 at 4: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. > >> I am wondering why can't we call ovl_get_inode() in object creation >> path. That should take care of race between creation path and file >> handle decode and only one of the paths will get to instantiate and >> initialize ovl_inode and other path will wait. >> > > I don't even want to think if what you wrote makes sense. > Remember that the use case we are talking about is quite imaginary. > Ensuring internal structures consistency in our code and returning > error to user is the right thing to do for imaginary use cases IMO. > Having being forced to think about it ;-), I think using ovl_get_inode() in create code does make a weird sort of sense. The reason it is weird is because we will always be throwing away the new inode that we allocated in ovl_create_object(). AFAICS, if only reason we need to allocate new inode in ovl_create_object() is to calculate i_mode with inode_init_owner() and that calculation can be factored out to not need an inode. The reason it does makes sense to use ovl_get_inode(), is because the alternative that Miklos suggested is to insert the new inode on the very very likely use case and throw away the new inode in the very very unlikely use case of collision. That alternative creates a somewhat complicated code path that is rarely to never executed - not the best practice. Thanks, Amir.
On Thu, May 17, 2018 at 8:03 AM, Amir Goldstein <amir73il@gmail.com> wrote: > On Tue, May 15, 2018 at 4: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. >> >>> I am wondering why can't we call ovl_get_inode() in object creation >>> path. That should take care of race between creation path and file >>> handle decode and only one of the paths will get to instantiate and >>> initialize ovl_inode and other path will wait. >>> >> >> I don't even want to think if what you wrote makes sense. >> Remember that the use case we are talking about is quite imaginary. >> Ensuring internal structures consistency in our code and returning >> error to user is the right thing to do for imaginary use cases IMO. >> > > Having being forced to think about it ;-), I think using ovl_get_inode() > in create code does make a weird sort of sense. Going through the same code-path very much makes sense. > The reason it is weird is because we will always be throwing away > the new inode that we allocated in ovl_create_object(). > AFAICS, if only reason we need to allocate new inode in > ovl_create_object() is to calculate i_mode with inode_init_owner() > and that calculation can be factored out to not need an inode. Not the only reason: we don't want inode allocation to fail after successful creation. Solution: add a preallocated inode argument to ovl_get_inode() and deal with allocation failure there. > The reason it does makes sense to use ovl_get_inode(), is because > the alternative that Miklos suggested is to insert the new inode on > the very very likely use case and throw away the new inode in the > very very unlikely use case of collision. That alternative creates a > somewhat complicated code path that is rarely to never executed - > not the best practice. Agreed completely. Thanks, Miklos
On Thu, May 17, 2018 at 11:10 AM, Miklos Szeredi <miklos@szeredi.hu> wrote: > On Thu, May 17, 2018 at 8:03 AM, Amir Goldstein <amir73il@gmail.com> wrote: >> On Tue, May 15, 2018 at 4: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. >>> >>>> I am wondering why can't we call ovl_get_inode() in object creation >>>> path. That should take care of race between creation path and file >>>> handle decode and only one of the paths will get to instantiate and >>>> initialize ovl_inode and other path will wait. >>>> >>> >>> I don't even want to think if what you wrote makes sense. >>> Remember that the use case we are talking about is quite imaginary. >>> Ensuring internal structures consistency in our code and returning >>> error to user is the right thing to do for imaginary use cases IMO. >>> >> >> Having being forced to think about it ;-), I think using ovl_get_inode() >> in create code does make a weird sort of sense. > > Going through the same code-path very much makes sense. > >> The reason it is weird is because we will always be throwing away >> the new inode that we allocated in ovl_create_object(). >> AFAICS, if only reason we need to allocate new inode in >> ovl_create_object() is to calculate i_mode with inode_init_owner() >> and that calculation can be factored out to not need an inode. > > Not the only reason: we don't want inode allocation to fail after > successful creation. Solution: add a preallocated inode argument to > ovl_get_inode() and deal with allocation failure there. > IIUC that would be moving the problem to another place. Are you suggesting that only in ENOMEM case we resort to using the preallocated inode and inserting it safely to cache? I'll make a variant of iget5_locked() that takes a preallocated inode argument to use instead of alloc_inode() and will always use that code path for creating objects. Or maybe that is what you meant. Thanks, Amir.
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c index 47dc980e8b33..62e6733b755c 100644 --- a/fs/overlayfs/dir.c +++ b/fs/overlayfs/dir.c @@ -183,14 +183,24 @@ static int ovl_set_opaque(struct dentry *dentry, struct dentry *upperdentry) } /* Common operations required to be done after creation of file on upper */ -static void ovl_instantiate(struct dentry *dentry, struct inode *inode, - struct dentry *newdentry, bool hardlink) +static int ovl_instantiate(struct dentry *dentry, struct inode *inode, + struct dentry *newdentry, bool hardlink) { ovl_dir_modified(dentry->d_parent, false); - ovl_copyattr(d_inode(newdentry), inode); ovl_dentry_set_upper_alias(dentry); if (!hardlink) { - ovl_inode_update(inode, newdentry); + int err; + + ovl_inode_init(inode, newdentry, NULL); + /* + * XXX: if we ever use ovl_obtain_alias() to decode directory + * file handles, need to use ovl_insert_inode_locked() and + * d_instantiate_new() here to prevent ovl_obtain_alias() + * from sneaking in before d_instantiate(). + */ + err = ovl_insert_inode(inode, d_inode(newdentry)); + if (err) + return err; } else { WARN_ON(ovl_inode_real(inode) != d_inode(newdentry)); dput(newdentry); @@ -200,6 +210,8 @@ static void ovl_instantiate(struct dentry *dentry, struct inode *inode, /* Force lookup of new upper hardlink to find its lower */ if (hardlink) d_drop(dentry); + + return 0; } static bool ovl_type_merge(struct dentry *dentry) @@ -238,7 +250,7 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode, ovl_set_opaque(dentry, newdentry); } - ovl_instantiate(dentry, inode, newdentry, !!hardlink); + err = ovl_instantiate(dentry, inode, newdentry, !!hardlink); newdentry = NULL; out_dput: dput(newdentry); @@ -439,7 +451,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode, if (err) goto out_cleanup; } - ovl_instantiate(dentry, inode, newdentry, !!hardlink); + err = ovl_instantiate(dentry, inode, newdentry, !!hardlink); newdentry = NULL; out_dput2: dput(upper); diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c index 7abcf96e94fc..060c534998d1 100644 --- a/fs/overlayfs/inode.c +++ b/fs/overlayfs/inode.c @@ -741,6 +741,24 @@ static bool ovl_verify_inode(struct inode *inode, struct dentry *lowerdentry, return true; } +static int ovl_insert_inode_locked(struct inode *inode, struct inode *realinode) +{ + return insert_inode_locked4(inode, (unsigned long) realinode, + ovl_inode_test, realinode); +} + +int ovl_insert_inode(struct inode *inode, struct inode *realinode) +{ + int err; + + err = ovl_insert_inode_locked(inode, realinode); + if (err) + return err; + + unlock_new_inode(inode); + return 0; +} + struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real, bool is_upper) { diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h index caaa47cea2aa..642b25702092 100644 --- a/fs/overlayfs/overlayfs.h +++ b/fs/overlayfs/overlayfs.h @@ -343,6 +343,7 @@ int ovl_update_time(struct inode *inode, struct timespec *ts, int flags); bool ovl_is_private_xattr(const char *name); struct inode *ovl_new_inode(struct super_block *sb, umode_t mode, dev_t rdev); +int ovl_insert_inode(struct inode *inode, struct inode *realinode); struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real, bool is_upper); struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
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. This race does not affect overlay directory inodes, because those are decoded via ovl_lookup_real() and not with ovl_obtain_alias(), so avoid using the new helper d_instantiate_new() to reduce backport dependencies. Backporting only makes sense for v4.16 where NFS export was introduced. Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: <stable@vger.kernel.org> #v4.16 Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/overlayfs/dir.c | 24 ++++++++++++++++++------ fs/overlayfs/inode.c | 18 ++++++++++++++++++ fs/overlayfs/overlayfs.h | 1 + 3 files changed, 37 insertions(+), 6 deletions(-)