Bug 217405
| Summary: | overlayfs: ovl_permission: Null pointer dereference at realinode in rcu-walk mode | ||
|---|---|---|---|
| Product: | File System | Reporter: | Zhihao Cheng (chengzhihao1) |
| Component: | Other | Assignee: | fs_other |
| Status: | NEW --- | ||
| Severity: | normal | CC: | quic_bhaskarv, wnsdud1861 |
| Priority: | P3 | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Kernel Version: | Subsystem: | ||
| Regression: | No | Bisected commit-id: | |
| Attachments: |
test.sh
diff |
||
|
Description
Zhihao Cheng
2023-05-05 08:13:18 UTC
Created attachment 304220 [details]
test.sh
Created attachment 304221 [details]
diff
I am also facing this issue and I am checking if dentry is in KILLED state to fix the issue. I see it makes sense to check this instead of inode NULL check, comment? (In reply to Bhaskar Valaboju from comment #3) > I am also facing this issue and I am checking if dentry is in KILLED state > to fix the issue. I see it makes sense to check this instead of inode NULL > check, comment? Do you judge the KILLED state by checking d_unhashed(dentry)? When do the check? Could you please sendout the fix code? Thanks. Please find below the change I am testing:
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 541cf3717fc2..69c481cc820f 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -289,7 +289,7 @@ int ovl_permission(struct mnt_idmap *idmap,
/* Careful in RCU walk mode */
ovl_i_path_real(inode, &realpath);
- if (!realpath.dentry) {
+ if (!realpath.dentry || realpath.dentry->d_flags & DCACHE_DENTRY_KILLED) {
WARN_ON(!(mask & MAY_NOT_BLOCK));
return -ECHILD;
}
@@ -402,6 +402,9 @@ int ovl_xattr_get(struct dentry *dentry, struct inode *inode, const char *name,
struct path realpath;
ovl_i_path_real(inode, &realpath);
+ if (!realpath.dentry || realpath.dentry->d_flags & DCACHE_DENTRY_KILLED)
+ return -ECHILD;
+
old_cred = ovl_override_creds(dentry->d_sb);
res = vfs_getxattr(mnt_idmap(realpath.mnt), realpath.dentry, name, value, size);
revert_creds(old_cred);
@@ -568,7 +571,7 @@ struct posix_acl *do_ovl_get_acl(struct mnt_idmap *idmap,
/* Careful in RCU walk mode */
ovl_i_path_real(inode, &realpath);
- if (!realpath.dentry) {
+ if (!realpath.dentry || realpath.dentry->d_flags & DCACHE_DENTRY_KILLED) {
WARN_ON(!rcu);
return ERR_PTR(-ECHILD);
}
(In reply to Bhaskar Valaboju from comment #5) > Please find below the change I am testing: > > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c > index 541cf3717fc2..69c481cc820f 100644 > --- a/fs/overlayfs/inode.c > +++ b/fs/overlayfs/inode.c > @@ -289,7 +289,7 @@ int ovl_permission(struct mnt_idmap *idmap, > > /* Careful in RCU walk mode */ > ovl_i_path_real(inode, &realpath); > - if (!realpath.dentry) { > + if (!realpath.dentry || realpath.dentry->d_flags & > DCACHE_DENTRY_KILLED) { > WARN_ON(!(mask & MAY_NOT_BLOCK)); > return -ECHILD; > } We'd better hold realpath.dentry->d_lock to check 'd_flags' and get 'd_inode', otherwise: P1 P2 ovl_permission if (realpath.dentry->d_flags & DCACHE_DENTRY_KILLED) // false drop_cache __dentry_kill dentry_unlist dentry->d_flags |= DCACHE_DENTRY_KILLED dentry_unlink_inode dentry->d_inode = NULL realinode = d_inode(realpath.dentry) // NULL Am I understanding right? > @@ -402,6 +402,9 @@ int ovl_xattr_get(struct dentry *dentry, struct inode > *inode, const char *name, > struct path realpath; > > ovl_i_path_real(inode, &realpath); > + if (!realpath.dentry || realpath.dentry->d_flags & > DCACHE_DENTRY_KILLED) > + return -ECHILD; > + > old_cred = ovl_override_creds(dentry->d_sb); > res = vfs_getxattr(mnt_idmap(realpath.mnt), realpath.dentry, name, > value, size); > revert_creds(old_cred); BTW, path_getxattr user_path_at(path) // hold refcnt for path.dentry getxattr(path.dentry) vfs_getxattr ovl_xattr_get // realpath.dentry cannot be killed path_put(&path) Are there any other paths to trigger null-ptr-deference in ovl_xattr_get()? > @@ -568,7 +571,7 @@ struct posix_acl *do_ovl_get_acl(struct mnt_idmap *idmap, > > /* Careful in RCU walk mode */ > ovl_i_path_real(inode, &realpath); > - if (!realpath.dentry) { > + if (!realpath.dentry || realpath.dentry->d_flags & > DCACHE_DENTRY_KILLED) { > WARN_ON(!rcu); > return ERR_PTR(-ECHILD); > } (In reply to Zhihao Cheng from comment #6) > (In reply to Bhaskar Valaboju from comment #5) > > Please find below the change I am testing: > > > > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c > > index 541cf3717fc2..69c481cc820f 100644 > > --- a/fs/overlayfs/inode.c > > +++ b/fs/overlayfs/inode.c > > @@ -289,7 +289,7 @@ int ovl_permission(struct mnt_idmap *idmap, > > > > /* Careful in RCU walk mode */ > > ovl_i_path_real(inode, &realpath); > > - if (!realpath.dentry) { > > + if (!realpath.dentry || realpath.dentry->d_flags & > > DCACHE_DENTRY_KILLED) { > > WARN_ON(!(mask & MAY_NOT_BLOCK)); > > return -ECHILD; > > } > > We'd better hold realpath.dentry->d_lock to check 'd_flags' and get > 'd_inode', otherwise: > P1 P2 > ovl_permission > if (realpath.dentry->d_flags & DCACHE_DENTRY_KILLED) // false > drop_cache > __dentry_kill > dentry_unlist > dentry->d_flags |= DCACHE_DENTRY_KILLED > dentry_unlink_inode > dentry->d_inode = NULL > realinode = d_inode(realpath.dentry) // NULL > > Am I understanding right? Makes sense. > > > @@ -402,6 +402,9 @@ int ovl_xattr_get(struct dentry *dentry, struct inode > > *inode, const char *name, > > struct path realpath; > > > > ovl_i_path_real(inode, &realpath); > > + if (!realpath.dentry || realpath.dentry->d_flags & > > DCACHE_DENTRY_KILLED) > > + return -ECHILD; > > + > > old_cred = ovl_override_creds(dentry->d_sb); > > res = vfs_getxattr(mnt_idmap(realpath.mnt), realpath.dentry, name, > > value, size); > > revert_creds(old_cred); > > BTW, > path_getxattr > user_path_at(path) // hold refcnt for path.dentry > getxattr(path.dentry) > vfs_getxattr > ovl_xattr_get // realpath.dentry cannot be killed > path_put(&path) > > Are there any other paths to trigger null-ptr-deference in ovl_xattr_get()? > There are no other parts, we can ignore this change. > > @@ -568,7 +571,7 @@ struct posix_acl *do_ovl_get_acl(struct mnt_idmap > *idmap, > > > > /* Careful in RCU walk mode */ > > ovl_i_path_real(inode, &realpath); > > - if (!realpath.dentry) { > > + if (!realpath.dentry || realpath.dentry->d_flags & > > DCACHE_DENTRY_KILLED) { > > WARN_ON(!rcu); > > return ERR_PTR(-ECHILD); > > } |