Message ID | 20200117151706.30559-1-christian.brauner@ubuntu.com |
---|---|
State | New |
Headers | show |
Series | [v2] UBUNTU: SAUCE: shiftfs: prevent lower dentries from going negative during unlink | expand |
On 17.01.20 17:17, 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. > > Acked-by: Stefan Bader <stefan.bader@canonical.com> If you send a V2, then its no longer acked and you should really send a self-nack on the v1. > 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> > --- > /* v1 */ > Link: https://lists.ubuntu.com/archives/kernel-team/2020-January/106965.html > > /* v2 */ > - Christian Brauner <christian.brauner@ubuntu.com>: > - add missing ; after dget(lowerd) > --- > fs/shiftfs.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/shiftfs.c b/fs/shiftfs.c > index 04fba4689eb6..3623d02b061e 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; > } > > base-commit: 5c6810aad94a79c86bee07cd19dba90b92f461b4 >
On 17.01.20 17:17, 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. > > Acked-by: Stefan Bader <stefan.bader@canonical.com> > 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> > --- > /* v1 */ > Link: https://lists.ubuntu.com/archives/kernel-team/2020-January/106965.html > > /* v2 */ > - Christian Brauner <christian.brauner@ubuntu.com>: > - add missing ; after dget(lowerd) Agreed that I probably should have seen this but also test compiling before submitting (even more so for simple things) does help (and that is from self-experinece). > --- > fs/shiftfs.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/shiftfs.c b/fs/shiftfs.c > index 04fba4689eb6..3623d02b061e 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; > } > > base-commit: 5c6810aad94a79c86bee07cd19dba90b92f461b4 >
On Mon, Jan 20, 2020 at 04:08:10PM +0200, Stefan Bader wrote: > On 17.01.20 17:17, 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. > > > > Acked-by: Stefan Bader <stefan.bader@canonical.com> > > 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> > > --- > > /* v1 */ > > Link: https://lists.ubuntu.com/archives/kernel-team/2020-January/106965.html > > > > /* v2 */ > > - Christian Brauner <christian.brauner@ubuntu.com>: > > - add missing ; after dget(lowerd) > > Agreed that I probably should have seen this but also test compiling before > submitting (even more so for simple things) does help (and that is from > self-experinece). Yeah, I did but I had printk() statements in there and my intention was to do a git fetch + git reset --hard but alas I forget the latter. So I did test a correct change just not the final patch without the printks. :/ Christian
On Fri, Jan 17, 2020 at 04:17:06PM +0100, 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. > > Acked-by: Stefan Bader <stefan.bader@canonical.com> > 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> Applied to focal/master-next and unstable/master, thanks!
On Fri, Jan 17, 2020 at 04:17:06PM +0100, 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. > > Acked-by: Stefan Bader <stefan.bader@canonical.com> > 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> > --- > /* v1 */ > Link: https://lists.ubuntu.com/archives/kernel-team/2020-January/106965.html > > /* v2 */ > - Christian Brauner <christian.brauner@ubuntu.com>: > - add missing ; after dget(lowerd) > --- > fs/shiftfs.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/shiftfs.c b/fs/shiftfs.c > index 04fba4689eb6..3623d02b061e 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; > } > > base-commit: 5c6810aad94a79c86bee07cd19dba90b92f461b4 > -- > 2.25.0 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team A late ack, but this looks fine. Acked-by: Sultan Alsawaf <sultan.alsawaf@canonical.com>
On 2020-01-20 15:47:42 , Stefan Bader wrote: > On 17.01.20 17:17, 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. > > > > Acked-by: Stefan Bader <stefan.bader@canonical.com> > > If you send a V2, then its no longer acked and you should really send a > self-nack on the v1. > > > 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> > > --- > > /* v1 */ > > Link: https://lists.ubuntu.com/archives/kernel-team/2020-January/106965.html > > > > /* v2 */ > > - Christian Brauner <christian.brauner@ubuntu.com>: > > - add missing ; after dget(lowerd) > > --- > > fs/shiftfs.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/fs/shiftfs.c b/fs/shiftfs.c > > index 04fba4689eb6..3623d02b061e 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; > > } > > > > base-commit: 5c6810aad94a79c86bee07cd19dba90b92f461b4 > > > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
diff --git a/fs/shiftfs.c b/fs/shiftfs.c index 04fba4689eb6..3623d02b061e 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; }