Date: Thu, 7 Oct 2021 20:00:36 GMT From: Alan Somers <asomers@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org Subject: git: c858f3d89b3c - stable/12 - fusefs: don't panic if FUSE_GETATTR fails durint VOP_GETPAGES Message-ID: <202110072000.197K0aYs091071@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch stable/12 has been updated by asomers: URL: https://cgit.FreeBSD.org/src/commit/?id=c858f3d89b3c92cf3c0962c976dc6bd322ad8b9b commit c858f3d89b3c92cf3c0962c976dc6bd322ad8b9b Author: Konstantin Belousov <kib@FreeBSD.org> AuthorDate: 2021-09-16 23:53:58 +0000 Commit: Alan Somers <asomers@FreeBSD.org> CommitDate: 2021-10-07 19:51:35 +0000 fusefs: don't panic if FUSE_GETATTR fails durint VOP_GETPAGES During VOP_GETPAGES, fusefs needs to determine the file's length, which could require a FUSE_GETATTR operation. If that fails, it's better to SIGBUS than panic. Sponsored by: Axcient Reviewed by: markj, kib Differential Revision: https://reviews.freebsd.org/D31994 (cherry picked from commit 4f917847c9037d9b76de188c03e13b81224431b2) buffer pager: allow get_blksize method to return error Reported and reviewed by: asomers Sponsored by: The FreeBSD Foundation Differential revision: https://reviews.freebsd.org/D31998 (cherry picked from commit 197a4f29f39e6ae6215a6dbd28ef449d305e6d49) --- sys/fs/cd9660/cd9660_vnops.c | 5 +- sys/fs/fuse/fuse_vnops.c | 21 +++--- sys/fs/msdosfs/msdosfs_vnops.c | 5 +- sys/fs/nfsclient/nfs_clbio.c | 5 +- sys/kern/vfs_bio.c | 16 +++-- sys/sys/buf.h | 2 +- sys/ufs/ffs/ffs_vnops.c | 5 +- tests/sys/fs/fusefs/read.cc | 156 +++++++++++++++++++++++++++++++++++++++++ 8 files changed, 189 insertions(+), 26 deletions(-) diff --git a/sys/fs/cd9660/cd9660_vnops.c b/sys/fs/cd9660/cd9660_vnops.c index 27a186964a80..2007b62087a4 100644 --- a/sys/fs/cd9660/cd9660_vnops.c +++ b/sys/fs/cd9660/cd9660_vnops.c @@ -859,12 +859,13 @@ cd9660_gbp_getblkno(struct vnode *vp, vm_ooffset_t off) } static int -cd9660_gbp_getblksz(struct vnode *vp, daddr_t lbn) +cd9660_gbp_getblksz(struct vnode *vp, daddr_t lbn, long *sz) { struct iso_node *ip; ip = VTOI(vp); - return (blksize(ip->i_mnt, ip, lbn)); + *sz = blksize(ip->i_mnt, ip, lbn); + return (0); } static int diff --git a/sys/fs/fuse/fuse_vnops.c b/sys/fs/fuse/fuse_vnops.c index 9e79a376cccd..d13ae257e0a8 100644 --- a/sys/fs/fuse/fuse_vnops.c +++ b/sys/fs/fuse/fuse_vnops.c @@ -1961,25 +1961,24 @@ fuse_gbp_getblkno(struct vnode *vp, vm_ooffset_t off) } static int -fuse_gbp_getblksz(struct vnode *vp, daddr_t lbn) +fuse_gbp_getblksz(struct vnode *vp, daddr_t lbn, long *blksz) { off_t filesize; - int blksz, err; + int err; const int biosize = fuse_iosize(vp); err = fuse_vnode_size(vp, &filesize, NULL, NULL); - KASSERT(err == 0, ("vfs_bio_getpages can't handle errors here")); - if (err) - return biosize; - - if ((off_t)lbn * biosize >= filesize) { - blksz = 0; + if (err) { + /* This will turn into a SIGBUS */ + return (EIO); + } else if ((off_t)lbn * biosize >= filesize) { + *blksz = 0; } else if ((off_t)(lbn + 1) * biosize > filesize) { - blksz = filesize - (off_t)lbn *biosize; + *blksz = filesize - (off_t)lbn *biosize; } else { - blksz = biosize; + *blksz = biosize; } - return (blksz); + return (0); } /* diff --git a/sys/fs/msdosfs/msdosfs_vnops.c b/sys/fs/msdosfs/msdosfs_vnops.c index d700f6a62d5c..c93754a0e251 100644 --- a/sys/fs/msdosfs/msdosfs_vnops.c +++ b/sys/fs/msdosfs/msdosfs_vnops.c @@ -1805,10 +1805,11 @@ msdosfs_gbp_getblkno(struct vnode *vp, vm_ooffset_t off) } static int -msdosfs_gbp_getblksz(struct vnode *vp, daddr_t lbn) +msdosfs_gbp_getblksz(struct vnode *vp, daddr_t lbn, long *sz) { - return (VTODE(vp)->de_pmp->pm_bpcluster); + *sz = VTODE(vp)->de_pmp->pm_bpcluster; + return (0); } static int diff --git a/sys/fs/nfsclient/nfs_clbio.c b/sys/fs/nfsclient/nfs_clbio.c index 5ba75491a800..c61cb1db9b95 100644 --- a/sys/fs/nfsclient/nfs_clbio.c +++ b/sys/fs/nfsclient/nfs_clbio.c @@ -94,7 +94,7 @@ ncl_gbp_getblkno(struct vnode *vp, vm_ooffset_t off) } static int -ncl_gbp_getblksz(struct vnode *vp, daddr_t lbn) +ncl_gbp_getblksz(struct vnode *vp, daddr_t lbn, long *sz) { struct nfsnode *np; u_quad_t nsize; @@ -111,7 +111,8 @@ ncl_gbp_getblksz(struct vnode *vp, daddr_t lbn) bcount = 0; else if ((off_t)(lbn + 1) * biosize > nsize) bcount = nsize - (off_t)lbn * biosize; - return (bcount); + *sz = bcount; + return (0); } int diff --git a/sys/kern/vfs_bio.c b/sys/kern/vfs_bio.c index 6c5286d30871..d58a13cf43e1 100644 --- a/sys/kern/vfs_bio.c +++ b/sys/kern/vfs_bio.c @@ -5143,8 +5143,8 @@ vfs_bio_getpages(struct vnode *vp, vm_page_t *ma, int count, struct mount *mp; daddr_t lbn, lbnp; vm_ooffset_t la, lb, poff, poffe; - long bsize; - int bo_bs, br_flags, error, i, pgsin, pgsin_a, pgsin_b; + long bo_bs, bsize; + int br_flags, error, i, pgsin, pgsin_a, pgsin_b; bool redo, lpart; object = vp->v_object; @@ -5161,7 +5161,10 @@ vfs_bio_getpages(struct vnode *vp, vm_page_t *ma, int count, */ la += PAGE_SIZE; lpart = la > object->un_pager.vnp.vnp_size; - bo_bs = get_blksize(vp, get_lblkno(vp, IDX_TO_OFF(ma[0]->pindex))); + error = get_blksize(vp, get_lblkno(vp, IDX_TO_OFF(ma[0]->pindex)), + &bo_bs); + if (error != 0) + return (VM_PAGER_ERROR); /* * Calculate read-ahead, behind and total pages. @@ -5219,9 +5222,10 @@ again: goto next_page; lbnp = lbn; - bsize = get_blksize(vp, lbn); - error = bread_gb(vp, lbn, bsize, curthread->td_ucred, - br_flags, &bp); + error = get_blksize(vp, lbn, &bsize); + if (error == 0) + error = bread_gb(vp, lbn, bsize, + curthread->td_ucred, br_flags, &bp); if (error != 0) goto end_pages; if (bp->b_rcred == curthread->td_ucred) { diff --git a/sys/sys/buf.h b/sys/sys/buf.h index 89c11a36bb42..2530a3540c23 100644 --- a/sys/sys/buf.h +++ b/sys/sys/buf.h @@ -576,7 +576,7 @@ void bwait(struct buf *, u_char, const char *); void bdone(struct buf *); typedef daddr_t (vbg_get_lblkno_t)(struct vnode *, vm_ooffset_t); -typedef int (vbg_get_blksize_t)(struct vnode *, daddr_t); +typedef int (vbg_get_blksize_t)(struct vnode *, daddr_t, long *); int vfs_bio_getpages(struct vnode *vp, struct vm_page **ma, int count, int *rbehind, int *rahead, vbg_get_lblkno_t get_lblkno, vbg_get_blksize_t get_blksize); diff --git a/sys/ufs/ffs/ffs_vnops.c b/sys/ufs/ffs/ffs_vnops.c index 9dae437f9a76..b7357cf0ecab 100644 --- a/sys/ufs/ffs/ffs_vnops.c +++ b/sys/ufs/ffs/ffs_vnops.c @@ -1739,10 +1739,11 @@ ffs_gbp_getblkno(struct vnode *vp, vm_ooffset_t off) } static int -ffs_gbp_getblksz(struct vnode *vp, daddr_t lbn) +ffs_gbp_getblksz(struct vnode *vp, daddr_t lbn, long *sz) { - return (blksize(VFSTOUFS(vp->v_mount)->um_fs, VTOI(vp), lbn)); + *sz = blksize(VFSTOUFS(vp->v_mount)->um_fs, VTOI(vp), lbn); + return (0); } static int diff --git a/tests/sys/fs/fusefs/read.cc b/tests/sys/fs/fusefs/read.cc index 22a26c2701fd..cb82d0a43b06 100644 --- a/tests/sys/fs/fusefs/read.cc +++ b/tests/sys/fs/fusefs/read.cc @@ -40,6 +40,8 @@ extern "C" { #include <aio.h> #include <fcntl.h> #include <semaphore.h> +#include <setjmp.h> +#include <signal.h> #include <unistd.h> } @@ -103,6 +105,33 @@ class ReadAhead: public Read, } }; +class ReadSigbus: public Read +{ +public: +static jmp_buf s_jmpbuf; +static sig_atomic_t s_si_addr; + +void TearDown() { + struct sigaction sa; + + bzero(&sa, sizeof(sa)); + sa.sa_handler = SIG_DFL; + sigaction(SIGBUS, &sa, NULL); + + FuseTest::TearDown(); +} + +}; + +static void +handle_sigbus(int signo __unused, siginfo_t *info, void *uap __unused) { + ReadSigbus::s_si_addr = (sig_atomic_t)info->si_addr; + longjmp(ReadSigbus::s_jmpbuf, 1); +} + +jmp_buf ReadSigbus::s_jmpbuf; +sig_atomic_t ReadSigbus::s_si_addr; + /* AIO reads need to set the header's pid field correctly */ /* https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=236379 */ TEST_F(AioRead, aio_read) @@ -584,6 +613,56 @@ TEST_F(Read, mmap) leak(fd); } +/* Read of an mmap()ed file fails */ +TEST_F(ReadSigbus, mmap_eio) +{ + const char FULLPATH[] = "mountpoint/some_file.txt"; + const char RELPATH[] = "some_file.txt"; + const char *CONTENTS = "abcdefgh"; + struct sigaction sa; + uint64_t ino = 42; + int fd; + ssize_t len; + size_t bufsize = strlen(CONTENTS); + void *p; + + len = getpagesize(); + + expect_lookup(RELPATH, ino, bufsize); + expect_open(ino, 0, 1); + EXPECT_CALL(*m_mock, process( + ResultOf([=](auto in) { + return (in.header.opcode == FUSE_READ && + in.header.nodeid == ino && + in.body.read.fh == Read::FH); + }, Eq(true)), + _) + ).WillRepeatedly(Invoke(ReturnErrno(EIO))); + + fd = open(FULLPATH, O_RDONLY); + ASSERT_LE(0, fd) << strerror(errno); + + p = mmap(NULL, len, PROT_READ, MAP_SHARED, fd, 0); + ASSERT_NE(MAP_FAILED, p) << strerror(errno); + + /* Accessing the mapped page should return SIGBUS. */ + + bzero(&sa, sizeof(sa)); + sa.sa_handler = SIG_DFL; + sa.sa_sigaction = handle_sigbus; + sa.sa_flags = SA_RESETHAND | SA_SIGINFO; + ASSERT_EQ(0, sigaction(SIGBUS, &sa, NULL)) << strerror(errno); + if (setjmp(ReadSigbus::s_jmpbuf) == 0) { + atomic_signal_fence(std::memory_order::memory_order_seq_cst); + volatile char x __unused = *(volatile char*)p; + FAIL() << "shouldn't get here"; + } + + ASSERT_EQ(p, (void*)ReadSigbus::s_si_addr); + ASSERT_EQ(0, munmap(p, len)) << strerror(errno); + leak(fd); +} + /* * A read via mmap comes up short, indicating that the file was truncated * server-side. @@ -634,6 +713,83 @@ TEST_F(Read, mmap_eof) leak(fd); } +/* + * During VOP_GETPAGES, the FUSE server fails a FUSE_GETATTR operation. This + * almost certainly indicates a buggy FUSE server, and our goal should be not + * to panic. Instead, generate SIGBUS. + */ +TEST_F(ReadSigbus, mmap_getblksz_fail) +{ + const char FULLPATH[] = "mountpoint/some_file.txt"; + const char RELPATH[] = "some_file.txt"; + const char *CONTENTS = "abcdefgh"; + struct sigaction sa; + Sequence seq; + uint64_t ino = 42; + int fd; + ssize_t len; + size_t bufsize = strlen(CONTENTS); + mode_t mode = S_IFREG | 0644; + void *p; + + len = getpagesize(); + + FuseTest::expect_lookup(RELPATH, ino, mode, bufsize, 1, 0); + /* Expect two GETATTR calls that succeed, followed by one that fail. */ + EXPECT_CALL(*m_mock, process( + ResultOf([=](auto in) { + return (in.header.opcode == FUSE_GETATTR && + in.header.nodeid == ino); + }, Eq(true)), + _) + ).Times(2) + .InSequence(seq) + .WillRepeatedly(Invoke(ReturnImmediate([=](auto i __unused, auto& out) { + SET_OUT_HEADER_LEN(out, attr); + out.body.attr.attr.ino = ino; + out.body.attr.attr.mode = mode; + out.body.attr.attr.size = bufsize; + out.body.attr.attr_valid = 0; + }))); + EXPECT_CALL(*m_mock, process( + ResultOf([=](auto in) { + return (in.header.opcode == FUSE_GETATTR && + in.header.nodeid == ino); + }, Eq(true)), + _) + ).InSequence(seq) + .WillRepeatedly(Invoke(ReturnErrno(EIO))); + expect_open(ino, 0, 1); + EXPECT_CALL(*m_mock, process( + ResultOf([=](auto in) { + return (in.header.opcode == FUSE_READ); + }, Eq(true)), + _) + ).Times(0); + + fd = open(FULLPATH, O_RDONLY); + ASSERT_LE(0, fd) << strerror(errno); + + p = mmap(NULL, len, PROT_READ, MAP_SHARED, fd, 0); + ASSERT_NE(MAP_FAILED, p) << strerror(errno); + + /* Accessing the mapped page should return SIGBUS. */ + bzero(&sa, sizeof(sa)); + sa.sa_handler = SIG_DFL; + sa.sa_sigaction = handle_sigbus; + sa.sa_flags = SA_RESETHAND | SA_SIGINFO; + ASSERT_EQ(0, sigaction(SIGBUS, &sa, NULL)) << strerror(errno); + if (setjmp(ReadSigbus::s_jmpbuf) == 0) { + atomic_signal_fence(std::memory_order::memory_order_seq_cst); + volatile char x __unused = *(volatile char*)p; + FAIL() << "shouldn't get here"; + } + + ASSERT_EQ(p, (void*)ReadSigbus::s_si_addr); + ASSERT_EQ(0, munmap(p, len)) << strerror(errno); + leak(fd); +} + /* * Just as when FOPEN_DIRECT_IO is used, reads with O_DIRECT should bypass * cache and to straight to the daemon
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202110072000.197K0aYs091071>