On Fri, Nov 23, 2018 at 07:58:15AM +0200, Amir Goldstein wrote: > On Fri, Nov 23, 2018 at 5:16 AM Pan Bian <bianpan2...@163.com> wrote: > > > > The function dentry_connected calls dput(dentry) to drop the previously > > acquired reference to dentry. In this case, dentry can be released. > > After that, IS_ROOT(dentry) checks the condition > > (dentry == dentry->d_parent), which may result in a use-after-free bug. > > This patch directly compares dentry with its parent obtained before > > dropping the reference. > > > > Fixes: a056cc8934c("exportfs: stop retrying once we race with > > rename/remove") > > > > CC Fixes patch author/reviewers > > How did you find this? by code review or did this actually happen? > > Normally a IS_ROOT dentry would be either DCACHE_DISCONNECTED or > pinned to some super block, but I guess there may be corner cases?
I found this by code review, and I have not yet observed crash. > > > Signed-off-by: Pan Bian <bianpan2...@163.com> > > --- > > fs/exportfs/expfs.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c > > index 645158d..a69aaf5 100644 > > --- a/fs/exportfs/expfs.c > > +++ b/fs/exportfs/expfs.c > > @@ -77,7 +77,7 @@ static bool dentry_connected(struct dentry *dentry) > > struct dentry *parent = dget_parent(dentry); > > > > dput(dentry); > > - if (IS_ROOT(dentry)) { > > + if (dentry == parent) { /* is root entry */ > > dput(parent); > > return false; > > } > > The change itself looks right, but the name IS_ROOT is confusing > enough as it is. The explicit comment is just plain wrong. > If it was really a root dentry, it wouldn't have been DCACHE_DISCONNECTED > (unless it is a filesystem bug). I will remove the comment and resubmit the patch. Thanks a lot, Pan > > Thanks, > Amir.