Messages in this thread | | | Date | Sun, 12 Oct 2014 05:29:25 +0100 | From | Al Viro <> | Subject | Re: fs/namei.c: Misuse of sequence counts? |
| |
On Sat, Oct 11, 2014 at 10:55:10PM -0500, Eric Biggers wrote: > On Sun, Oct 12, 2014 at 12:46:35AM +0100, Al Viro wrote: > > > > Nope. What we do is > > * pick parent inode and seqcount (in whatever order) > > * THEN check that child is still unchanged. > > The second part guarantees that parent dentry had been the parent of > > child all along, since the moment we'd first fetched _child's_ seqcount. > > And since a pinned positive dentry can't have its ->d_inode changed, > > we know that the value of parent's inode we'd fetched remained valid > > at least until we'd checked the child's seqcount and found it unchanged. > > Which means that we had it valid at some point after we'd fetched parent's > > seqcount. > > Ah, very tricky. And I take it that the other two fetches of d_inode in > follow_dotdot_rcu() can likewise be unordered with respect to > read_seqcount_begin(), because the underlying dentries are pinned as either > mnt_mountpoint or mnt_root --- which in RCU mode, is only guaranteed because of > the call to synchronize_rcu() in namespace_unlock() prior to dropping > references?
The last one is actually covered by read_seqretry(&mount_lock, nd->m_seq) - if it still matches, we know that whatever we got from __lookup_mnt() must have been valid through fetching ->d_inode and ->d_seq of its mnt_root. Which means that those two are consistent regardless of that synchronize_rcu().
The one before it would probably be better off with similar check on mount_lock as well. That code *is* correct for the reason you've mentioned, but I wonder if explicit check of mount_lock would be better - right now it's more subtle than I'd like it to be. I don't think the cost would be noticable - it's smp_rmb() + fetch + comparison when we cross a mountpoint while following .. in lazy pathwalk, but that needs profiling - handwaving is not good enough...
| |