From 9c4f551e98baec5d8adf2d078832f9185efdd52e Mon Sep 17 00:00:00 2001 From: Kirk McKusick Date: Wed, 28 Jun 2017 17:32:09 +0000 Subject: [PATCH] Create a new function ffs_getcg() to read in and verify a cylinder group. Change all code points that open-coded this functionality to use the new function. This commit is a refactoring with no change in functionality. In the future this change allows more robust checking of cylinder group reads along the lines discussed in the hardening UFS session at BSDCan (retry I/O, add checksums, etc). For more detail see the session notes at https://wiki.freebsd.org/DevSummit/201706/HardeningUFS Reviewed by: kib --- sys/ufs/ffs/ffs_alloc.c | 145 +++++++++++++++---------------------- sys/ufs/ffs/ffs_extern.h | 2 + sys/ufs/ffs/ffs_snapshot.c | 11 +-- sys/ufs/ffs/ffs_vfsops.c | 7 +- sys/ufs/ufs/ufs_gjournal.c | 10 +-- 5 files changed, 66 insertions(+), 109 deletions(-) diff --git a/sys/ufs/ffs/ffs_alloc.c b/sys/ufs/ffs/ffs_alloc.c index cbf7966ca1f..9e0178e2bb8 100644 --- a/sys/ufs/ffs/ffs_alloc.c +++ b/sys/ufs/ffs/ffs_alloc.c @@ -1602,15 +1602,8 @@ ffs_fragextend(ip, cg, bprev, osize, nsize) return (0); } UFS_UNLOCK(ump); - error = bread(ump->um_devvp, fsbtodb(fs, cgtod(fs, cg)), - (int)fs->fs_cgsize, NOCRED, &bp); - if (error) + if ((error = ffs_getcg(fs, ump->um_devvp, cg, &bp, &cgp)) != 0) goto fail; - cgp = (struct cg *)bp->b_data; - if (!cg_chkmagic(cgp)) - goto fail; - bp->b_xflags |= BX_BKGRDWRITE; - cgp->cg_old_time = cgp->cg_time = time_second; bno = dtogd(fs, bprev); blksfree = cg_blksfree(cgp); for (i = numfrags(fs, osize); i < frags; i++) @@ -1680,16 +1673,9 @@ ffs_alloccg(ip, cg, bpref, size, rsize) if (fs->fs_cs(fs, cg).cs_nbfree == 0 && size == fs->fs_bsize) return (0); UFS_UNLOCK(ump); - error = bread(ump->um_devvp, fsbtodb(fs, cgtod(fs, cg)), - (int)fs->fs_cgsize, NOCRED, &bp); - if (error) + if ((error = ffs_getcg(fs, ump->um_devvp, cg, &bp, &cgp)) != 0 || + (cgp->cg_cs.cs_nbfree == 0 && size == fs->fs_bsize)) goto fail; - cgp = (struct cg *)bp->b_data; - if (!cg_chkmagic(cgp) || - (cgp->cg_cs.cs_nbfree == 0 && size == fs->fs_bsize)) - goto fail; - bp->b_xflags |= BX_BKGRDWRITE; - cgp->cg_old_time = cgp->cg_time = time_second; if (size == fs->fs_bsize) { UFS_LOCK(ump); blkno = ffs_alloccgblk(ip, bp, bpref, rsize); @@ -1857,7 +1843,7 @@ ffs_clusteralloc(ip, cg, bpref, len) struct cg *cgp; struct buf *bp; struct ufsmount *ump; - int i, run, bit, map, got; + int i, run, bit, map, got, error; ufs2_daddr_t bno; u_char *mapp; int32_t *lp; @@ -1868,13 +1854,10 @@ ffs_clusteralloc(ip, cg, bpref, len) if (fs->fs_maxcluster[cg] < len) return (0); UFS_UNLOCK(ump); - if (bread(ump->um_devvp, fsbtodb(fs, cgtod(fs, cg)), (int)fs->fs_cgsize, - NOCRED, &bp)) - goto fail_lock; - cgp = (struct cg *)bp->b_data; - if (!cg_chkmagic(cgp)) - goto fail_lock; - bp->b_xflags |= BX_BKGRDWRITE; + if ((error = ffs_getcg(fs, ump->um_devvp, cg, &bp, &cgp)) != 0) { + UFS_LOCK(ump); + return (0); + } /* * Check to see if a cluster of the needed size (or bigger) is * available in this cylinder group. @@ -1897,7 +1880,8 @@ ffs_clusteralloc(ip, cg, bpref, len) break; UFS_LOCK(ump); fs->fs_maxcluster[cg] = i; - goto fail; + brelse(bp); + return (0); } /* * Search the cluster map to find a big enough cluster. @@ -1933,8 +1917,11 @@ ffs_clusteralloc(ip, cg, bpref, len) bit = 1; } } - if (got >= cgp->cg_nclusterblks) - goto fail_lock; + if (got >= cgp->cg_nclusterblks) { + UFS_LOCK(ump); + brelse(bp); + return (0); + } /* * Allocate the cluster that we have found. */ @@ -1954,12 +1941,6 @@ ffs_clusteralloc(ip, cg, bpref, len) UFS_UNLOCK(ump); bdwrite(bp); return (bno); - -fail_lock: - UFS_LOCK(ump); -fail: - brelse(bp); - return (0); } static inline struct buf * @@ -2005,21 +1986,16 @@ ffs_nodealloccg(ip, cg, ipref, mode, unused) if (fs->fs_cs(fs, cg).cs_nifree == 0) return (0); UFS_UNLOCK(ump); - error = bread(ump->um_devvp, fsbtodb(fs, cgtod(fs, cg)), - (int)fs->fs_cgsize, NOCRED, &bp); - if (error) { - brelse(bp); + if ((error = ffs_getcg(fs, ump->um_devvp, cg, &bp, &cgp)) != 0) { UFS_LOCK(ump); return (0); } - cgp = (struct cg *)bp->b_data; restart: - if (!cg_chkmagic(cgp) || cgp->cg_cs.cs_nifree == 0) { + if (cgp->cg_cs.cs_nifree == 0) { brelse(bp); UFS_LOCK(ump); return (0); } - bp->b_xflags |= BX_BKGRDWRITE; inosused = cg_inosused(cgp); if (ipref) { ipref %= fs->fs_ipg; @@ -2103,21 +2079,16 @@ ffs_nodealloccg(ip, cg, ipref, mode, unused) * to it, then leave it unchanged as the other thread * has already set it correctly. */ - error = bread(ump->um_devvp, fsbtodb(fs, cgtod(fs, cg)), - (int)fs->fs_cgsize, NOCRED, &bp); + error = ffs_getcg(fs, ump->um_devvp, cg, &bp, &cgp); UFS_LOCK(ump); ACTIVECLEAR(fs, cg); UFS_UNLOCK(ump); - if (error != 0) { - brelse(bp); + if (error != 0) return (error); - } - cgp = (struct cg *)bp->b_data; if (cgp->cg_initediblk == old_initediblk) cgp->cg_initediblk += INOPB(fs); goto restart; } - cgp->cg_old_time = cgp->cg_time = time_second; cgp->cg_irotor = ipref; UFS_LOCK(ump); ACTIVECLEAR(fs, cg); @@ -2160,7 +2131,7 @@ ffs_blkfree_cg(ump, fs, devvp, bno, size, inum, dephd) struct buf *bp; ufs1_daddr_t fragno, cgbno; ufs2_daddr_t cgblkno; - int i, blk, frags, bbase; + int i, blk, frags, bbase, error; u_int cg; u_int8_t *blksfree; struct cdev *dev; @@ -2193,17 +2164,8 @@ ffs_blkfree_cg(ump, fs, devvp, bno, size, inum, dephd) ffs_fserr(fs, inum, "bad block"); return; } - if (bread(devvp, cgblkno, (int)fs->fs_cgsize, NOCRED, &bp)) { - brelse(bp); + if ((error = ffs_getcg(fs, devvp, cg, &bp, &cgp)) != 0) return; - } - cgp = (struct cg *)bp->b_data; - if (!cg_chkmagic(cgp)) { - brelse(bp); - return; - } - bp->b_xflags |= BX_BKGRDWRITE; - cgp->cg_old_time = cgp->cg_time = time_second; cgbno = dtogd(fs, bno); blksfree = cg_blksfree(cgp); UFS_LOCK(ump); @@ -2408,14 +2370,9 @@ ffs_checkblk(ip, bno, size) } if ((u_int)bno >= fs->fs_size) panic("ffs_checkblk: bad block %jd", (intmax_t)bno); - error = bread(ITODEVVP(ip), fsbtodb(fs, cgtod(fs, dtog(fs, bno))), - (int)fs->fs_cgsize, NOCRED, &bp); + error = ffs_getcg(fs, ITODEVVP(ip), dtog(fs, bno), &bp, &cgp); if (error) - panic("ffs_checkblk: cg bread failed"); - cgp = (struct cg *)bp->b_data; - if (!cg_chkmagic(cgp)) - panic("ffs_checkblk: cg magic mismatch"); - bp->b_xflags |= BX_BKGRDWRITE; + panic("ffs_checkblk: cylinder group read failed"); blksfree = cg_blksfree(cgp); cgbno = dtogd(fs, bno); if (size == fs->fs_bsize) { @@ -2492,17 +2449,8 @@ ffs_freefile(ump, fs, devvp, ino, mode, wkhd) if (ino >= fs->fs_ipg * fs->fs_ncg) panic("ffs_freefile: range: dev = %s, ino = %ju, fs = %s", devtoname(dev), (uintmax_t)ino, fs->fs_fsmnt); - if ((error = bread(devvp, cgbno, (int)fs->fs_cgsize, NOCRED, &bp))) { - brelse(bp); + if ((error = ffs_getcg(fs, devvp, cg, &bp, &cgp)) != 0) return (error); - } - cgp = (struct cg *)bp->b_data; - if (!cg_chkmagic(cgp)) { - brelse(bp); - return (0); - } - bp->b_xflags |= BX_BKGRDWRITE; - cgp->cg_old_time = cgp->cg_time = time_second; inosused = cg_inosused(cgp); ino %= fs->fs_ipg; if (isclr(inosused, ino)) { @@ -2535,6 +2483,7 @@ ffs_freefile(ump, fs, devvp, ino, mode, wkhd) /* * Check to see if a file is free. + * Used to check for allocated files in snapshots. */ int ffs_checkfreefile(fs, devvp, ino) @@ -2545,7 +2494,7 @@ ffs_checkfreefile(fs, devvp, ino) struct cg *cgp; struct buf *bp; ufs2_daddr_t cgbno; - int ret; + int ret, error; u_int cg; u_int8_t *inosused; @@ -2561,15 +2510,8 @@ ffs_checkfreefile(fs, devvp, ino) } if (ino >= fs->fs_ipg * fs->fs_ncg) return (1); - if (bread(devvp, cgbno, (int)fs->fs_cgsize, NOCRED, &bp)) { - brelse(bp); + if ((error = ffs_getcg(fs, devvp, cg, &bp, &cgp)) != 0) return (1); - } - cgp = (struct cg *)bp->b_data; - if (!cg_chkmagic(cgp)) { - brelse(bp); - return (1); - } inosused = cg_inosused(cgp); ino %= fs->fs_ipg; ret = isclr(inosused, ino); @@ -2644,6 +2586,39 @@ ffs_mapsearch(fs, cgp, bpref, allocsiz) return (-1); } +/* + * Fetch and verify a cylinder group. + */ +int +ffs_getcg(fs, devvp, cg, bpp, cgpp) + struct fs *fs; + struct vnode *devvp; + u_int cg; + struct buf **bpp; + struct cg **cgpp; +{ + struct buf *bp; + struct cg *cgp; + int error; + + *bpp = NULL; + *cgpp = NULL; + error = bread(devvp, fsbtodb(fs, cgtod(fs, cg)), + (int)fs->fs_cgsize, NOCRED, &bp); + if (error != 0) + return (error); + cgp = (struct cg *)bp->b_data; + if (!cg_chkmagic(cgp) || cgp->cg_cgx != cg) { + brelse(bp); + return (EIO); + } + bp->b_xflags |= BX_BKGRDWRITE; + cgp->cg_old_time = cgp->cg_time = time_second; + *bpp = bp; + *cgpp = cgp; + return (0); +} + /* * Fserr prints the name of a filesystem with an error diagnostic. * diff --git a/sys/ufs/ffs/ffs_extern.h b/sys/ufs/ffs/ffs_extern.h index 399f755a88a..801af268196 100644 --- a/sys/ufs/ffs/ffs_extern.h +++ b/sys/ufs/ffs/ffs_extern.h @@ -74,6 +74,8 @@ void ffs_fragacct(struct fs *, int, int32_t [], int); int ffs_freefile(struct ufsmount *, struct fs *, struct vnode *, ino_t, int, struct workhead *); void ffs_fserr(struct fs *, ino_t, char *); +int ffs_getcg(struct fs *, struct vnode *, u_int, struct buf **, + struct cg **); int ffs_isblock(struct fs *, u_char *, ufs1_daddr_t); int ffs_isfreeblock(struct fs *, u_char *, ufs1_daddr_t); void ffs_load_inode(struct buf *, struct inode *, struct fs *, ino_t); diff --git a/sys/ufs/ffs/ffs_snapshot.c b/sys/ufs/ffs/ffs_snapshot.c index 09f972c097d..e9908a8b994 100644 --- a/sys/ufs/ffs/ffs_snapshot.c +++ b/sys/ufs/ffs/ffs_snapshot.c @@ -888,17 +888,8 @@ cgaccount(cg, vp, nbp, passno) ip = VTOI(vp); fs = ITOFS(ip); - error = bread(ITODEVVP(ip), fsbtodb(fs, cgtod(fs, cg)), - (int)fs->fs_cgsize, KERNCRED, &bp); - if (error) { - brelse(bp); + if ((error = ffs_getcg(fs, ITODEVVP(ip), cg, &bp, &cgp)) != 0) return (error); - } - cgp = (struct cg *)bp->b_data; - if (!cg_chkmagic(cgp)) { - brelse(bp); - return (EIO); - } UFS_LOCK(ITOUMP(ip)); ACTIVESET(fs, cg); /* diff --git a/sys/ufs/ffs/ffs_vfsops.c b/sys/ufs/ffs/ffs_vfsops.c index 1439dc720ad..d73d0ca0c46 100644 --- a/sys/ufs/ffs/ffs_vfsops.c +++ b/sys/ufs/ffs/ffs_vfsops.c @@ -1863,12 +1863,9 @@ ffs_fhtovp(mp, fhp, flags, vpp) if (fs->fs_magic != FS_UFS2_MAGIC) return (ufs_fhtovp(mp, ufhp, flags, vpp)); cg = ino_to_cg(fs, ino); - error = bread(ump->um_devvp, fsbtodb(fs, cgtod(fs, cg)), - (int)fs->fs_cgsize, NOCRED, &bp); - if (error) + if ((error = ffs_getcg(fs, ump->um_devvp, cg, &bp, &cgp)) != 0) return (error); - cgp = (struct cg *)bp->b_data; - if (!cg_chkmagic(cgp) || ino >= cg * fs->fs_ipg + cgp->cg_initediblk) { + if (ino >= cg * fs->fs_ipg + cgp->cg_initediblk) { brelse(bp); return (ESTALE); } diff --git a/sys/ufs/ufs/ufs_gjournal.c b/sys/ufs/ufs/ufs_gjournal.c index 807393dcb44..306797f1055 100644 --- a/sys/ufs/ufs/ufs_gjournal.c +++ b/sys/ufs/ufs/ufs_gjournal.c @@ -86,16 +86,8 @@ ufs_gjournal_modref(struct vnode *vp, int count) if ((u_int)ino >= fs->fs_ipg * fs->fs_ncg) panic("ufs_gjournal_modref: range: dev = %s, ino = %lu, fs = %s", devtoname(dev), (u_long)ino, fs->fs_fsmnt); - if ((error = bread(devvp, cgbno, (int)fs->fs_cgsize, NOCRED, &bp))) { - brelse(bp); + if ((error = ffs_getcg(fs, devvp, cg, &bp, &cgp)) != 0) return (error); - } - cgp = (struct cg *)bp->b_data; - if (!cg_chkmagic(cgp)) { - brelse(bp); - return (0); - } - bp->b_xflags |= BX_BKGRDWRITE; cgp->cg_unrefs += count; UFS_LOCK(ump); fs->fs_unrefs += count;