sdcardfs: fix stale permission error

This patch fixes a bug showing EACCES of open() which is caused by missing inode and dentry object. Bug: 142101178 Signed-off-by: Jaegeuk Kim <jaegeuk@google.com> Change-Id: I4cab9a3554fad9e2c58a0840fc968257b796e156
2020-10-01 00:53:59 -07:00
parent a0b560f596
commit 082eabab5a
5 changed files with 67 additions and 48 deletions
--- a/fs/sdcardfs/derived_perm.c
+++ b/fs/sdcardfs/derived_perm.c
@@ -51,11 +51,11 @@ void setup_derived_state(struct inode *inode, perm_t perm, userid_t userid,
 /* While renaming, there is a point where we want the path from dentry,
 * but the name from newdentry
 */
-void get_derived_permission_new(struct dentry *parent, struct dentry *dentry,
+void get_derived_permission_new(struct inode *parent, struct inode *inode,
 				const struct qstr *name)
 {
-	struct sdcardfs_inode_info *info = SDCARDFS_I(d_inode(dentry));
-	struct sdcardfs_inode_info *parent_info = SDCARDFS_I(d_inode(parent));
+	struct sdcardfs_inode_info *info = SDCARDFS_I(inode);
+	struct sdcardfs_inode_info *parent_info = SDCARDFS_I(parent);
 	struct sdcardfs_inode_data *parent_data = parent_info->data;
 	appid_t appid;
 	unsigned long user_num;
@@ -75,10 +75,10 @@ void get_derived_permission_new(struct dentry *parent, struct dentry *dentry,
 	 * of using the inode permissions.
 	 */

-	inherit_derived_state(d_inode(parent), d_inode(dentry));
+	inherit_derived_state(parent, inode);

 	/* Files don't get special labels */
-	if (!S_ISDIR(d_inode(dentry)->i_mode)) {
+	if (!S_ISDIR(inode->i_mode)) {
 		set_top(info, parent_info);
 		return;
 	}
@@ -145,9 +145,9 @@ void get_derived_permission_new(struct dentry *parent, struct dentry *dentry,
 	}
 }

-void get_derived_permission(struct dentry *parent, struct dentry *dentry)
+void get_derived_permission(struct inode *parent, struct dentry *dentry)
 {
-	get_derived_permission_new(parent, dentry, &dentry->d_name);
+	get_derived_permission_new(parent, d_inode(dentry), &dentry->d_name);
 }

 static appid_t get_type(const char *name)
@@ -306,9 +306,11 @@ static void __fixup_perms_recursive(struct dentry *dentry, struct limit_search *
 	if (needs_fixup(info->data->perm)) {
 		list_for_each_entry(child, &dentry->d_subdirs, d_child) {
 			spin_lock_nested(&child->d_lock, depth + 1);
-			if (!(limit->flags & BY_NAME) || qstr_case_eq(&child->d_name, &limit->name)) {
+			if (!(limit->flags & BY_NAME) ||
+				qstr_case_eq(&child->d_name, &limit->name)) {
 				if (d_inode(child)) {
-					get_derived_permission(dentry, child);
+					get_derived_permission(d_inode(dentry),
+									child);
 					fixup_tmp_permissions(d_inode(child));
 					spin_unlock(&child->d_lock);
 					break;
@@ -330,25 +332,16 @@ void fixup_perms_recursive(struct dentry *dentry, struct limit_search *limit)
 }

 /* main function for updating derived permission */
-inline void update_derived_permission_lock(struct dentry *dentry)
+inline void update_derived_permission_lock(struct inode *dir,
+					struct inode *inode,
+					struct dentry *dentry)
 {
-	struct dentry *parent;
-
-	if (!dentry || !d_inode(dentry)) {
-		pr_err("sdcardfs: %s: invalid dentry\n", __func__);
-		return;
-	}
 	/* FIXME:
 	 * 1. need to check whether the dentry is updated or not
 	 * 2. remove the root dentry update
 	 */
-	if (!IS_ROOT(dentry)) {
-		parent = dget_parent(dentry);
-		if (parent) {
-			get_derived_permission(parent, dentry);
-			dput(parent);
-		}
-	}
+	if (!IS_ROOT(dentry))
+		get_derived_permission(dir, dentry);
 	fixup_tmp_permissions(d_inode(dentry));
 }

--- a/fs/sdcardfs/inode.c
+++ b/fs/sdcardfs/inode.c
@@ -105,7 +105,7 @@ static int sdcardfs_create(struct inode *dir, struct dentry *dentry,
 	if (err)
 		goto out;

-	err = sdcardfs_interpose(dentry, dir->i_sb, &lower_path,
+	err = sdcardfs_interpose(dir, dentry, dir->i_sb, &lower_path,
 			SDCARDFS_I(dir)->data->userid);
 	if (err)
 		goto out;
@@ -287,7 +287,8 @@ static int sdcardfs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode
 			make_nomedia_in_obb = 1;
 	}

-	err = sdcardfs_interpose(dentry, dir->i_sb, &lower_path, pd->userid);
+	err = sdcardfs_interpose(dir, dentry, dir->i_sb,
+				&lower_path, pd->userid);
 	if (err) {
 		unlock_dir(lower_parent_dentry);
 		goto out;
@@ -455,7 +456,9 @@ static int sdcardfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 		sdcardfs_copy_and_fix_attrs(old_dir, d_inode(lower_old_dir_dentry));
 		fsstack_copy_inode_size(old_dir, d_inode(lower_old_dir_dentry));
 	}
-	get_derived_permission_new(new_dentry->d_parent, old_dentry, &new_dentry->d_name);
+	get_derived_permission_new(d_inode(new_dentry->d_parent),
+					d_inode(old_dentry),
+					&new_dentry->d_name);
 	fixup_tmp_permissions(d_inode(old_dentry));
 	fixup_lower_ownership(old_dentry, new_dentry->d_name.name);
 	d_invalidate(old_dentry); /* Can't fixup ownership recursively :( */
@@ -556,12 +559,10 @@ static int sdcardfs_permission(struct vfsmount *mnt, struct inode *inode, int ma
 {
 	int err;
 	struct inode tmp;
-	struct sdcardfs_inode_data *top = top_data_get(SDCARDFS_I(inode));
+	struct sdcardfs_inode_data *top;

 	if (IS_ERR(mnt))
 		return PTR_ERR(mnt);
-	if (!top)
-		return -EINVAL;

 	/*
 	 * Permission check on sdcardfs inode.
@@ -575,11 +576,21 @@ static int sdcardfs_permission(struct vfsmount *mnt, struct inode *inode, int ma
 	 * locks must be dealt with to avoid undefined behavior.
 	 */
 	copy_attrs(&tmp, inode);
+
+	mutex_lock(&SDCARDFS_I(inode)->top_mutex);
+	top = top_data_get(SDCARDFS_I(inode));
+	if (!top) {
+		mutex_unlock(&SDCARDFS_I(inode)->top_mutex);
+		return -EINVAL;
+	}
+
 	tmp.i_uid = make_kuid(&init_user_ns, top->d_uid);
 	tmp.i_gid = make_kgid(&init_user_ns, get_gid(mnt, inode->i_sb, top));
 	tmp.i_mode = (inode->i_mode & S_IFMT)
 			| get_mode(mnt, SDCARDFS_I(inode), top);
 	data_put(top);
+	mutex_unlock(&SDCARDFS_I(inode)->top_mutex);
+
 	tmp.i_sb = inode->i_sb;
 	if (IS_POSIXACL(inode))
 		pr_warn("%s: This may be undefined behavior...\n", __func__);
--- a/fs/sdcardfs/lookup.c
+++ b/fs/sdcardfs/lookup.c
@@ -165,10 +165,11 @@ struct inode *sdcardfs_iget(struct super_block *sb, struct inode *lower_inode, u
 * Helper interpose routine, called directly by ->lookup to handle
 * spliced dentries.
 */
-static struct dentry *__sdcardfs_interpose(struct dentry *dentry,
-					 struct super_block *sb,
-					 struct path *lower_path,
-					 userid_t id)
+static struct dentry *__sdcardfs_interpose(struct inode *dir,
+					struct dentry *dentry,
+					struct super_block *sb,
+					struct path *lower_path,
+					userid_t id)
 {
 	struct inode *inode;
 	struct inode *lower_inode;
@@ -196,10 +197,15 @@ static struct dentry *__sdcardfs_interpose(struct dentry *dentry,
 		goto out;
 	}

+	mutex_lock(&SDCARDFS_I(inode)->top_mutex);
+
 	ret_dentry = d_splice_alias(inode, dentry);
 	dentry = ret_dentry ?: dentry;
+
 	if (!IS_ERR(dentry))
-		update_derived_permission_lock(dentry);
+		update_derived_permission_lock(dir, inode, dentry);
+
+	mutex_unlock(&SDCARDFS_I(inode)->top_mutex);
 out:
 	return ret_dentry;
 }
@@ -212,12 +218,13 @@ out:
 * @sb: sdcardfs's super_block
 * @lower_path: the lower path (caller does path_get/put)
 */
-int sdcardfs_interpose(struct dentry *dentry, struct super_block *sb,
-		     struct path *lower_path, userid_t id)
+int sdcardfs_interpose(struct inode *dir, struct dentry *dentry,
+			struct super_block *sb,
+			struct path *lower_path, userid_t id)
 {
 	struct dentry *ret_dentry;

-	ret_dentry = __sdcardfs_interpose(dentry, sb, lower_path, id);
+	ret_dentry = __sdcardfs_interpose(dir, dentry, sb, lower_path, id);
 	return PTR_ERR(ret_dentry);
 }

@@ -249,8 +256,11 @@ static int sdcardfs_name_match(struct dir_context *ctx, const char *name,
 * Returns: NULL (ok), ERR_PTR if an error occurred.
 * Fills in lower_parent_path with <dentry,mnt> on success.
 */
-static struct dentry *__sdcardfs_lookup(struct dentry *dentry,
-		unsigned int flags, struct path *lower_parent_path, userid_t id)
+static struct dentry *__sdcardfs_lookup(struct inode *dir,
+					struct dentry *dentry,
+					unsigned int flags,
+					struct path *lower_parent_path,
+					userid_t id)
 {
 	int err = 0;
 	struct vfsmount *lower_dir_mnt;
@@ -347,8 +357,8 @@ put_name:
 		}

 		sdcardfs_set_lower_path(dentry, &lower_path);
-		ret_dentry =
-			__sdcardfs_interpose(dentry, dentry->d_sb, &lower_path, id);
+		ret_dentry = __sdcardfs_interpose(dir, dentry,
+					dentry->d_sb, &lower_path, id);
 		if (IS_ERR(ret_dentry)) {
 			err = PTR_ERR(ret_dentry);
 			 /* path_put underlying path on error */
@@ -450,7 +460,7 @@ struct dentry *sdcardfs_lookup(struct inode *dir, struct dentry *dentry,
 		goto out;
 	}

-	ret = __sdcardfs_lookup(dentry, flags, &lower_parent_path,
+	ret = __sdcardfs_lookup(dir, dentry, flags, &lower_parent_path,
 				SDCARDFS_I(dir)->data->userid);
 	if (IS_ERR(ret))
 		goto out;
@@ -460,7 +470,7 @@ struct dentry *sdcardfs_lookup(struct inode *dir, struct dentry *dentry,
 		fsstack_copy_attr_times(d_inode(dentry),
 					sdcardfs_lower_inode(d_inode(dentry)));
 		/* get derived permission */
-		get_derived_permission(parent, dentry);
+		get_derived_permission(dir, dentry);
 		fixup_tmp_permissions(d_inode(dentry));
 		fixup_lower_ownership(dentry, dentry->d_name.name);
 	}
--- a/fs/sdcardfs/sdcardfs.h
+++ b/fs/sdcardfs/sdcardfs.h
@@ -148,8 +148,9 @@ extern struct dentry *sdcardfs_lookup(struct inode *dir, struct dentry *dentry,
 				unsigned int flags);
 extern struct inode *sdcardfs_iget(struct super_block *sb,
 				 struct inode *lower_inode, userid_t id);
-extern int sdcardfs_interpose(struct dentry *dentry, struct super_block *sb,
-			    struct path *lower_path, userid_t id);
+extern int sdcardfs_interpose(struct inode *dir, struct dentry *dentry,
+				struct super_block *sb,
+				struct path *lower_path, userid_t id);

 /* file private data */
 struct sdcardfs_file_info {
@@ -177,6 +178,7 @@ struct sdcardfs_inode_info {

 	/* top folder for ownership */
 	spinlock_t top_lock;
+	struct mutex top_mutex;
 	struct sdcardfs_inode_data *top_data;

 	struct inode vfs_inode;
@@ -505,11 +507,13 @@ struct limit_search {

 extern void setup_derived_state(struct inode *inode, perm_t perm,
 			userid_t userid, uid_t uid);
-extern void get_derived_permission(struct dentry *parent, struct dentry *dentry);
-extern void get_derived_permission_new(struct dentry *parent, struct dentry *dentry, const struct qstr *name);
+extern void get_derived_permission(struct inode *parent, struct dentry *dentry);
+extern void get_derived_permission_new(struct inode *parent,
+			struct inode *inode, const struct qstr *name);
 extern void fixup_perms_recursive(struct dentry *dentry, struct limit_search *limit);

-extern void update_derived_permission_lock(struct dentry *dentry);
+extern void update_derived_permission_lock(struct inode *dir,
+			struct inode *inode, struct dentry *dentry);
 void fixup_lower_ownership(struct dentry *dentry, const char *name);
 extern int need_graft_path(struct dentry *dentry);
 extern int is_base_obbpath(struct dentry *dentry);
--- a/fs/sdcardfs/super.c
+++ b/fs/sdcardfs/super.c
@@ -218,6 +218,7 @@ static struct inode *sdcardfs_alloc_inode(struct super_block *sb)
 	i->top_data = d;
 	spin_lock_init(&i->top_lock);
 	kref_get(&d->refcount);
+	mutex_init(&i->top_mutex);

 	i->vfs_inode.i_version = 1;
 	return &i->vfs_inode;