| Message ID | 20200116213641.24064-1-christian.brauner@ubuntu.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,DISCO,EOAN,FOCAL] UBUNTU: SAUCE: shiftfs: prevent lower dentries from going negative during unlink | expand |
On 16.01.20 22:36, Christian Brauner wrote: > BugLink: https://bugs.launchpad.net/bugs/1860041 > > All non-special files (For shiftfs this only includes fifos and - for > this case - unix sockets - since we don't allow character and block > devices to be created.) go through shiftfs_open() and have their dentry > pinned through this codepath preventing it from going negative. But > fifos don't use the shiftfs fops but rather use the pipefifo_fops which > means they do not go through shiftfs_open() and thus don't have their > dentry pinned that way. Thus, the lower dentries for such files can go > negative on unlink causing segfaults. The following C program can be > used to reproduce the crash: > > #include <stdio.h> > #include <fcntl.h> > #include <unistd.h> > #include <sys/types.h> > #include <sys/stat.h> > #include <unistd.h> > #include <stdlib.h> > > int main(int argc, char *argv[]) > { > struct stat stat; > > unlink("./bbb"); > > int ret = mknod("./bbb", S_IFIFO|0666, 0); > if (ret < 0) > exit(1); > > int fd = open("./bbb", O_RDWR); > if (fd < 0) > exit(2); > > if (unlink("./bbb")) > exit(4); > > fstat(fd, &stat); > > return 0; > } > > Similar to ecryptfs we need to dget() the lower dentry before calling > vfs_unlink() on it and dput() it afterwards. > > Link: https://travis-ci.community/t/arm64-ppc64le-segfaults/6158/3 > Signed-off-by: Seth Forshee <seth.forshee@canonical.com> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- > fs/shiftfs.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/shiftfs.c b/fs/shiftfs.c > index 04fba4689eb6..897e9f1b90db 100644 > --- a/fs/shiftfs.c > +++ b/fs/shiftfs.c > @@ -583,6 +583,7 @@ static int shiftfs_rm(struct inode *dir, struct dentry *dentry, bool rmdir) > int err; > const struct cred *oldcred; > > + dget(lowerd) > oldcred = shiftfs_override_creds(dentry->d_sb); > inode_lock_nested(loweri, I_MUTEX_PARENT); > if (rmdir) > @@ -602,6 +603,7 @@ static int shiftfs_rm(struct inode *dir, struct dentry *dentry, bool rmdir) > inode_unlock(loweri); > > shiftfs_copyattr(loweri, dir); > + dput(lowerd); > > return err; > } >
On 16.01.20 23:36, Christian Brauner wrote: > BugLink: https://bugs.launchpad.net/bugs/1860041 > > All non-special files (For shiftfs this only includes fifos and - for > this case - unix sockets - since we don't allow character and block > devices to be created.) go through shiftfs_open() and have their dentry > pinned through this codepath preventing it from going negative. But > fifos don't use the shiftfs fops but rather use the pipefifo_fops which > means they do not go through shiftfs_open() and thus don't have their > dentry pinned that way. Thus, the lower dentries for such files can go > negative on unlink causing segfaults. The following C program can be > used to reproduce the crash: > > #include <stdio.h> > #include <fcntl.h> > #include <unistd.h> > #include <sys/types.h> > #include <sys/stat.h> > #include <unistd.h> > #include <stdlib.h> > > int main(int argc, char *argv[]) > { > struct stat stat; > > unlink("./bbb"); > > int ret = mknod("./bbb", S_IFIFO|0666, 0); > if (ret < 0) > exit(1); > > int fd = open("./bbb", O_RDWR); > if (fd < 0) > exit(2); > > if (unlink("./bbb")) > exit(4); > > fstat(fd, &stat); > > return 0; > } > > Similar to ecryptfs we need to dget() the lower dentry before calling > vfs_unlink() on it and dput() it afterwards. > > Link: https://travis-ci.community/t/arm64-ppc64le-segfaults/6158/3 > Signed-off-by: Seth Forshee <seth.forshee@canonical.com> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> > --- > fs/shiftfs.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/shiftfs.c b/fs/shiftfs.c > index 04fba4689eb6..897e9f1b90db 100644 > --- a/fs/shiftfs.c > +++ b/fs/shiftfs.c > @@ -583,6 +583,7 @@ static int shiftfs_rm(struct inode *dir, struct dentry *dentry, bool rmdir) > int err; > const struct cred *oldcred; > > + dget(lowerd) > oldcred = shiftfs_override_creds(dentry->d_sb); > inode_lock_nested(loweri, I_MUTEX_PARENT); > if (rmdir) > @@ -602,6 +603,7 @@ static int shiftfs_rm(struct inode *dir, struct dentry *dentry, bool rmdir) > inode_unlock(loweri); > > shiftfs_copyattr(loweri, dir); > + dput(lowerd); > > return err; > } > There is a v2 that was sent.
diff --git a/fs/shiftfs.c b/fs/shiftfs.c index 04fba4689eb6..897e9f1b90db 100644 --- a/fs/shiftfs.c +++ b/fs/shiftfs.c @@ -583,6 +583,7 @@ static int shiftfs_rm(struct inode *dir, struct dentry *dentry, bool rmdir) int err; const struct cred *oldcred; + dget(lowerd) oldcred = shiftfs_override_creds(dentry->d_sb); inode_lock_nested(loweri, I_MUTEX_PARENT); if (rmdir) @@ -602,6 +603,7 @@ static int shiftfs_rm(struct inode *dir, struct dentry *dentry, bool rmdir) inode_unlock(loweri); shiftfs_copyattr(loweri, dir); + dput(lowerd); return err; }
BugLink: https://bugs.launchpad.net/bugs/1860041 All non-special files (For shiftfs this only includes fifos and - for this case - unix sockets - since we don't allow character and block devices to be created.) go through shiftfs_open() and have their dentry pinned through this codepath preventing it from going negative. But fifos don't use the shiftfs fops but rather use the pipefifo_fops which means they do not go through shiftfs_open() and thus don't have their dentry pinned that way. Thus, the lower dentries for such files can go negative on unlink causing segfaults. The following C program can be used to reproduce the crash: #include <stdio.h> #include <fcntl.h> #include <unistd.h> #include <sys/types.h> #include <sys/stat.h> #include <unistd.h> #include <stdlib.h> int main(int argc, char *argv[]) { struct stat stat; unlink("./bbb"); int ret = mknod("./bbb", S_IFIFO|0666, 0); if (ret < 0) exit(1); int fd = open("./bbb", O_RDWR); if (fd < 0) exit(2); if (unlink("./bbb")) exit(4); fstat(fd, &stat); return 0; } Similar to ecryptfs we need to dget() the lower dentry before calling vfs_unlink() on it and dput() it afterwards. Link: https://travis-ci.community/t/arm64-ppc64le-segfaults/6158/3 Signed-off-by: Seth Forshee <seth.forshee@canonical.com> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> --- fs/shiftfs.c | 2 ++ 1 file changed, 2 insertions(+)