Skip to content

Commit

Permalink
fuse: fix race between getattr and write
Browse files Browse the repository at this point in the history 
Getattr and lookup operations can be running in parallel to attribute changing
operations, such as write and setattr.

This means, that if for example getattr was slower than a write, the cached
size attribute could be set to a stale value.

To prevent this race, introduce a per-filesystem attribute version counter.
This counter is incremented whenever cached attributes are modified, and the
incremented value stored in the inode.

Before storing new attributes in the cache, getattr and lookup check, using
the version number, whether the attributes have been modified during the
request's lifetime.  If so, the returned attributes are not cached, because
they might be stale.

Thanks to Jakub Bogusz for the bug report and test program.

[akpm@linux-foundation.org: coding-style fixes]
Signed-off-by: Miklos Szeredi <mszeredi@suse.cz>
Cc: Jakub Bogusz <jakub.bogusz@gemius.pl>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
  • Loading branch information
Miklos Szeredi authored and Linus Torvalds committed Oct 18, 2007
1 parent e57ac68 commit 1fb69e7
Show file tree
Hide file tree
Showing 4 changed files with 111 additions and 50 deletions.
125 changes: 82 additions & 43 deletions fs/fuse/dir.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,21 @@ static u64 time_to_jiffies(unsigned long sec, unsigned long nsec)
* Set dentry and possibly attribute timeouts from the lookup/mk*
* replies
*/
static void fuse_change_timeout(struct dentry *entry, struct fuse_entry_out *o)
static void fuse_change_entry_timeout(struct dentry *entry,
struct fuse_entry_out *o)
{
fuse_dentry_settime(entry,
time_to_jiffies(o->entry_valid, o->entry_valid_nsec));
if (entry->d_inode)
get_fuse_inode(entry->d_inode)->i_time =
time_to_jiffies(o->attr_valid, o->attr_valid_nsec);
}

static u64 attr_timeout(struct fuse_attr_out *o)
{
return time_to_jiffies(o->attr_valid, o->attr_valid_nsec);
}

static u64 entry_attr_timeout(struct fuse_entry_out *o)
{
return time_to_jiffies(o->attr_valid, o->attr_valid_nsec);
}

/*
Expand Down Expand Up @@ -140,6 +148,7 @@ static int fuse_dentry_revalidate(struct dentry *entry, struct nameidata *nd)
struct fuse_req *req;
struct fuse_req *forget_req;
struct dentry *parent;
u64 attr_version;

/* For negative dentries, always do a fresh lookup */
if (!inode)
Expand All @@ -156,6 +165,10 @@ static int fuse_dentry_revalidate(struct dentry *entry, struct nameidata *nd)
return 0;
}

spin_lock(&fc->lock);
attr_version = fc->attr_version;
spin_unlock(&fc->lock);

parent = dget_parent(entry);
fuse_lookup_init(req, parent->d_inode, entry, &outarg);
request_send(fc, req);
Expand All @@ -180,8 +193,10 @@ static int fuse_dentry_revalidate(struct dentry *entry, struct nameidata *nd)
if (err || (outarg.attr.mode ^ inode->i_mode) & S_IFMT)
return 0;

fuse_change_attributes(inode, &outarg.attr);
fuse_change_timeout(entry, &outarg);
fuse_change_attributes(inode, &outarg.attr,
entry_attr_timeout(&outarg),
attr_version);
fuse_change_entry_timeout(entry, &outarg);
}
return 1;
}
Expand Down Expand Up @@ -228,6 +243,7 @@ static struct dentry *fuse_lookup(struct inode *dir, struct dentry *entry,
struct fuse_conn *fc = get_fuse_conn(dir);
struct fuse_req *req;
struct fuse_req *forget_req;
u64 attr_version;

if (entry->d_name.len > FUSE_NAME_MAX)
return ERR_PTR(-ENAMETOOLONG);
Expand All @@ -242,6 +258,10 @@ static struct dentry *fuse_lookup(struct inode *dir, struct dentry *entry,
return ERR_PTR(PTR_ERR(forget_req));
}

spin_lock(&fc->lock);
attr_version = fc->attr_version;
spin_unlock(&fc->lock);

fuse_lookup_init(req, dir, entry, &outarg);
request_send(fc, req);
err = req->out.h.error;
Expand All @@ -253,7 +273,8 @@ static struct dentry *fuse_lookup(struct inode *dir, struct dentry *entry,
err = -EIO;
if (!err && outarg.nodeid) {
inode = fuse_iget(dir->i_sb, outarg.nodeid, outarg.generation,
&outarg.attr);
&outarg.attr, entry_attr_timeout(&outarg),
attr_version);
if (!inode) {
fuse_send_forget(fc, forget_req, outarg.nodeid, 1);
return ERR_PTR(-ENOMEM);
Expand All @@ -276,7 +297,7 @@ static struct dentry *fuse_lookup(struct inode *dir, struct dentry *entry,

entry->d_op = &fuse_dentry_operations;
if (!err)
fuse_change_timeout(entry, &outarg);
fuse_change_entry_timeout(entry, &outarg);
else
fuse_invalidate_entry_cache(entry);
return NULL;
Expand Down Expand Up @@ -363,7 +384,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry, int mode,

fuse_put_request(fc, req);
inode = fuse_iget(dir->i_sb, outentry.nodeid, outentry.generation,
&outentry.attr);
&outentry.attr, entry_attr_timeout(&outentry), 0);
if (!inode) {
flags &= ~(O_CREAT | O_EXCL | O_TRUNC);
ff->fh = outopen.fh;
Expand All @@ -373,7 +394,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry, int mode,
}
fuse_put_request(fc, forget_req);
d_instantiate(entry, inode);
fuse_change_timeout(entry, &outentry);
fuse_change_entry_timeout(entry, &outentry);
file = lookup_instantiate_filp(nd, entry, generic_file_open);
if (IS_ERR(file)) {
ff->fh = outopen.fh;
Expand Down Expand Up @@ -428,7 +449,7 @@ static int create_new_entry(struct fuse_conn *fc, struct fuse_req *req,
goto out_put_forget_req;

inode = fuse_iget(dir->i_sb, outarg.nodeid, outarg.generation,
&outarg.attr);
&outarg.attr, entry_attr_timeout(&outarg), 0);
if (!inode) {
fuse_send_forget(fc, forget_req, outarg.nodeid, 1);
return -ENOMEM;
Expand All @@ -451,7 +472,7 @@ static int create_new_entry(struct fuse_conn *fc, struct fuse_req *req,
} else
d_instantiate(entry, inode);

fuse_change_timeout(entry, &outarg);
fuse_change_entry_timeout(entry, &outarg);
fuse_invalidate_attr(dir);
return 0;

Expand Down Expand Up @@ -663,15 +684,43 @@ static int fuse_link(struct dentry *entry, struct inode *newdir,
return err;
}

static int fuse_do_getattr(struct inode *inode)
static void fuse_fillattr(struct inode *inode, struct fuse_attr *attr,
struct kstat *stat)
{
stat->dev = inode->i_sb->s_dev;
stat->ino = attr->ino;
stat->mode = (inode->i_mode & S_IFMT) | (attr->mode & 07777);
stat->nlink = attr->nlink;
stat->uid = attr->uid;
stat->gid = attr->gid;
stat->rdev = inode->i_rdev;
stat->atime.tv_sec = attr->atime;
stat->atime.tv_nsec = attr->atimensec;
stat->mtime.tv_sec = attr->mtime;
stat->mtime.tv_nsec = attr->mtimensec;
stat->ctime.tv_sec = attr->ctime;
stat->ctime.tv_nsec = attr->ctimensec;
stat->size = attr->size;
stat->blocks = attr->blocks;
stat->blksize = (1 << inode->i_blkbits);
}

static int fuse_do_getattr(struct inode *inode, struct kstat *stat)
{
int err;
struct fuse_attr_out arg;
struct fuse_conn *fc = get_fuse_conn(inode);
struct fuse_req *req = fuse_get_req(fc);
struct fuse_req *req;
u64 attr_version;

req = fuse_get_req(fc);
if (IS_ERR(req))
return PTR_ERR(req);

spin_lock(&fc->lock);
attr_version = fc->attr_version;
spin_unlock(&fc->lock);

req->in.h.opcode = FUSE_GETATTR;
req->in.h.nodeid = get_node_id(inode);
req->out.numargs = 1;
Expand All @@ -685,29 +734,16 @@ static int fuse_do_getattr(struct inode *inode)
make_bad_inode(inode);
err = -EIO;
} else {
struct fuse_inode *fi = get_fuse_inode(inode);
fuse_change_attributes(inode, &arg.attr);
fi->i_time = time_to_jiffies(arg.attr_valid,
arg.attr_valid_nsec);
fuse_change_attributes(inode, &arg.attr,
attr_timeout(&arg),
attr_version);
if (stat)
fuse_fillattr(inode, &arg.attr, stat);
}
}
return err;
}

/*
* Check if attributes are still valid, and if not send a GETATTR
* request to refresh them.
*/
static int fuse_refresh_attributes(struct inode *inode)
{
struct fuse_inode *fi = get_fuse_inode(inode);

if (fi->i_time < get_jiffies_64())
return fuse_do_getattr(inode);
else
return 0;
}

/*
* Calling into a user-controlled filesystem gives the filesystem
* daemon ptrace-like capabilities over the requester process. This
Expand Down Expand Up @@ -795,11 +831,14 @@ static int fuse_permission(struct inode *inode, int mask, struct nameidata *nd)
*/
if ((fc->flags & FUSE_DEFAULT_PERMISSIONS) ||
((mask & MAY_EXEC) && S_ISREG(inode->i_mode))) {
err = fuse_refresh_attributes(inode);
if (err)
return err;
struct fuse_inode *fi = get_fuse_inode(inode);
if (fi->i_time < get_jiffies_64()) {
err = fuse_do_getattr(inode, NULL);
if (err)
return err;

refreshed = true;
refreshed = true;
}
}

if (fc->flags & FUSE_DEFAULT_PERMISSIONS) {
Expand All @@ -809,7 +848,7 @@ static int fuse_permission(struct inode *inode, int mask, struct nameidata *nd)
attributes. This is also needed, because the root
node will at first have no permissions */
if (err == -EACCES && !refreshed) {
err = fuse_do_getattr(inode);
err = fuse_do_getattr(inode, NULL);
if (!err)
err = generic_permission(inode, mask, NULL);
}
Expand All @@ -825,7 +864,7 @@ static int fuse_permission(struct inode *inode, int mask, struct nameidata *nd)
if (refreshed)
return -EACCES;

err = fuse_do_getattr(inode);
err = fuse_do_getattr(inode, NULL);
if (!err && !(inode->i_mode & S_IXUGO))
return -EACCES;
}
Expand Down Expand Up @@ -999,7 +1038,6 @@ static int fuse_setattr(struct dentry *entry, struct iattr *attr)
{
struct inode *inode = entry->d_inode;
struct fuse_conn *fc = get_fuse_conn(inode);
struct fuse_inode *fi = get_fuse_inode(inode);
struct fuse_req *req;
struct fuse_setattr_in inarg;
struct fuse_attr_out outarg;
Expand Down Expand Up @@ -1053,8 +1091,7 @@ static int fuse_setattr(struct dentry *entry, struct iattr *attr)
return -EIO;
}

fuse_change_attributes(inode, &outarg.attr);
fi->i_time = time_to_jiffies(outarg.attr_valid, outarg.attr_valid_nsec);
fuse_change_attributes(inode, &outarg.attr, attr_timeout(&outarg), 0);
return 0;
}

Expand All @@ -1069,8 +1106,10 @@ static int fuse_getattr(struct vfsmount *mnt, struct dentry *entry,
if (!fuse_allow_task(fc, current))
return -EACCES;

err = fuse_refresh_attributes(inode);
if (!err) {
if (fi->i_time < get_jiffies_64())
err = fuse_do_getattr(inode, stat);
else {
err = 0;
generic_fillattr(inode, stat);
stat->mode = fi->orig_i_mode;
}
Expand Down
2 changes: 2 additions & 0 deletions fs/fuse/file.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,7 @@ static int fuse_buffered_write(struct file *file, struct inode *inode,
int err;
size_t nres;
struct fuse_conn *fc = get_fuse_conn(inode);
struct fuse_inode *fi = get_fuse_inode(inode);
unsigned offset = pos & (PAGE_CACHE_SIZE - 1);
struct fuse_req *req;

Expand All @@ -499,6 +500,7 @@ static int fuse_buffered_write(struct file *file, struct inode *inode,
if (!err) {
pos += nres;
spin_lock(&fc->lock);
fi->attr_version = ++fc->attr_version;
if (pos > inode->i_size)
i_size_write(inode, pos);
spin_unlock(&fc->lock);
Expand Down
12 changes: 10 additions & 2 deletions fs/fuse/fuse_i.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ struct fuse_inode {
/** The sticky bit in inode->i_mode may have been removed, so
preserve the original mode */
mode_t orig_i_mode;

/** Version of last attribute change */
u64 attr_version;
};

/** FUSE specific file data */
Expand Down Expand Up @@ -387,6 +390,9 @@ struct fuse_conn {

/** Reserved request for the DESTROY message */
struct fuse_req *destroy_req;

/** Version counter for attribute changes */
u64 attr_version;
};

static inline struct fuse_conn *get_fuse_conn_super(struct super_block *sb)
Expand Down Expand Up @@ -416,7 +422,8 @@ extern const struct file_operations fuse_dev_operations;
* Get a filled in inode
*/
struct inode *fuse_iget(struct super_block *sb, unsigned long nodeid,
int generation, struct fuse_attr *attr);
int generation, struct fuse_attr *attr,
u64 attr_valid, u64 attr_version);

/**
* Send FORGET command
Expand Down Expand Up @@ -477,7 +484,8 @@ void fuse_init_symlink(struct inode *inode);
/**
* Change attributes of an inode
*/
void fuse_change_attributes(struct inode *inode, struct fuse_attr *attr);
void fuse_change_attributes(struct inode *inode, struct fuse_attr *attr,
u64 attr_valid, u64 attr_version);

/**
* Initialize the client device
Expand Down
Loading

0 comments on commit 1fb69e7

Please sign in to comment.