On 2/14/21 9:40 AM, Al Viro wrote: > On Sun, Feb 14, 2021 at 04:05:22PM +0000, Al Viro wrote: >> On Sun, Feb 07, 2021 at 01:26:19PM -0700, Jens Axboe wrote: >> >>> Al, not sure if this is the right fix for the situation, but it's >>> definitely a problem. Observed by doing a LOOKUP_CACHED of something with >>> links, using /proc/self/comm as the example in the attached way to >>> demonstrate this problem. >> >> That's definitely not the right fix. What your analysis has missed is >> what legitimize_links() does to nd->depth when called. IOW, on transitions >> from RCU mode you want nd->depth to set according the number of links we'd >> grabbed references to. Flatly setting it to 0 on failure exit will lead >> to massive leaks. >> >> Could you check if the following fixes your reproducers? >> >> diff --git a/fs/namei.c b/fs/namei.c >> index 4cae88733a5c..afb293b39be7 100644 >> --- a/fs/namei.c >> +++ b/fs/namei.c >> @@ -687,7 +687,7 @@ static bool try_to_unlazy(struct nameidata *nd) >> >> nd->flags &= ~LOOKUP_RCU; >> if (nd->flags & LOOKUP_CACHED) >> - goto out1; >> + goto out2; >> if (unlikely(!legitimize_links(nd))) >> goto out1; >> if (unlikely(!legitimize_path(nd, &nd->path, nd->seq))) >> @@ -698,6 +698,8 @@ static bool try_to_unlazy(struct nameidata *nd) >> BUG_ON(nd->inode != parent->d_inode); >> return true; >> >> +out2: >> + nd->depth = 0; // as we hadn't gotten to legitimize_links() >> out1: >> nd->path.mnt = NULL; >> nd->path.dentry = NULL; >> @@ -725,7 +727,7 @@ static bool try_to_unlazy_next(struct nameidata *nd, struct dentry *dentry, unsi >> >> nd->flags &= ~LOOKUP_RCU; >> if (nd->flags & LOOKUP_CACHED) >> - goto out2; >> + goto out3; >> if (unlikely(!legitimize_links(nd))) >> goto out2; >> if (unlikely(!legitimize_mnt(nd->path.mnt, nd->m_seq))) >> @@ -753,6 +755,8 @@ static bool try_to_unlazy_next(struct nameidata *nd, struct dentry *dentry, unsi >> rcu_read_unlock(); >> return true; >> >> +out3: >> + nd->depth = 0; // as we hadn't gotten to legitimize_links() >> out2: >> nd->path.mnt = NULL; >> out1: > > Alternatively, we could use the fact that legitimize_links() is not > called anywhere other than these two places and have LOOKUP_CACHED > checked there. As in Both fix the issue for me, just tested them. The second one seems cleaner to me, would probably be nice to have a comment on that in either the two callers or at least in legitimize_links() though. -- Jens Axboe
