On Sun, Sep 29, 2013 at 11:10 AM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> FWIW, right now I'm reviewing the subset of fs code that can be hit in
> RCU mode. Not a pretty sight, that... ;-/ First catch: in
> fuse_dentry_revalidate() we have a case (reachable with LOOKUP_RCU) where
> we do this:
> } else if (inode) {
> fc = get_fuse_conn(inode);
> if (fc->readdirplus_auto) {
> parent = dget_parent(entry);
> fuse_advise_use_readdirplus(parent->d_inode);
> dput(parent);
> }
> }
Ugh, yes, that dget/dput(parent) looks wrong in RCU mode.
That said, in RCU mode you simply shouldn't _need_ it at all, you
should be able to just use dentry->d_parent without any refcount
games. Put an ACCESS_ONCE there to be safe. You might want to make
sure that you do the same for the inode, and check for NULL, to be
safe against racing with a cross-directory rename/rmdir. I don't know
if there are then any internal fuse races with the whole
get_fuse_conn() etc, so...
It does look bad. In practice, of course, it will never hit anything.
> If my reading of that code is right, the proper fix would be to
> turn that else if (inode) into else if (inode && !(flags & LOOKUP_RCU))
That sounds safer, but then the fuse_advise_use_readdirplus() bit
wouldn't get set. But why _is_ that bit set there in the first place?
It sounds stupid. I think the bit should be set in the lookup path (or
the revalidation slow-path when the timeout is over and the thing gets
properly revalidated), why the hell does it do it in the fast-path
revalidation in the first place? That's just odd. Maybe there is some
odd internal fuse logic.
Miklos, please do give that a look..
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
