Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 2 Mar 2018 04:34:53 +0000 (UTC)
From:      Kirk McKusick <mckusick@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r330264 - in head: lib/libufs stand/libsa sys/geom sys/geom/journal sys/geom/label sys/ufs/ffs
Message-ID:  <201803020434.w224Yr9J023438@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: mckusick
Date: Fri Mar  2 04:34:53 2018
New Revision: 330264
URL: https://svnweb.freebsd.org/changeset/base/330264

Log:
  This change is some refactoring of Mark Johnston's changes in r329375
  to fix the memory leak that I introduced in r328426. Instead of
  trying to clear up the possible memory leak in all the clients, I
  ensure that it gets cleaned up in the source (e.g., ffs_sbget ensures
  that memory is always freed if it returns an error).
  
  The original change in r328426 was a bit sparse in its description.
  So I am expanding on its description here (thanks cem@ and rgrimes@
  for your encouragement for my longer commit messages).
  
  In preparation for adding check hashing to superblocks, r328426 is
  a refactoring of the code to get the reading/writing of the superblock
  into one place. Unlike the cylinder group reading/writing which
  ends up in two places (ffs_getcg/ffs_geom_strategy in the kernel
  and cgget/cgput in libufs), I have the core superblock functions
  just in the kernel (ffs_sbfetch/ffs_sbput in ffs_subr.c which is
  already imported into utilities like fsck_ffs as well as libufs to
  implement sbget/sbput). The ffs_sbfetch and ffs_sbput functions
  take a function pointer to do the actual I/O for which there are
  four variants:
  
      ffs_use_bread / ffs_use_bwrite for the in-kernel filesystem
  
      g_use_g_read_data / g_use_g_write_data for kernel geom clients
  
      ufs_use_sa_read for the standalone code (stand/libsa/ufs.c
  	but not stand/libsa/ufsread.c which is size constrained)
  
      use_pread / use_pwrite for libufs
  
  Uses of these interfaces are in the UFS filesystem, geoms journal &
  label, libsa changes, and libufs. They also permeate out into the
  filesystem utilities fsck_ffs, newfs, growfs, clri, dump, quotacheck,
  fsirand, fstyp, and quot. Some of these utilities should probably be
  converted to directly use libufs (like dumpfs was for example), but
  there does not seem to be much win in doing so.
  
  Tested by: Peter Holm (pho@)

Modified:
  head/lib/libufs/sblock.c
  head/stand/libsa/ufs.c
  head/sys/geom/geom_io.c
  head/sys/geom/journal/g_journal_ufs.c
  head/sys/geom/label/g_label_ufs.c
  head/sys/ufs/ffs/ffs_subr.c
  head/sys/ufs/ffs/ffs_vfsops.c

Modified: head/lib/libufs/sblock.c
==============================================================================
--- head/lib/libufs/sblock.c	Fri Mar  2 03:05:36 2018	(r330263)
+++ head/lib/libufs/sblock.c	Fri Mar  2 04:34:53 2018	(r330264)
@@ -150,7 +150,6 @@ use_pread(void *devfd, off_t loc, void **bufp, int siz
 	int fd;
 
 	fd = *(int *)devfd;
-	free(*bufp);
 	if ((*bufp = malloc(size)) == NULL)
 		return (ENOSPC);
 	if (pread(fd, *bufp, size, loc) != size)

Modified: head/stand/libsa/ufs.c
==============================================================================
--- head/stand/libsa/ufs.c	Fri Mar  2 03:05:36 2018	(r330263)
+++ head/stand/libsa/ufs.c	Fri Mar  2 04:34:53 2018	(r330264)
@@ -518,7 +518,7 @@ ufs_open(upath, f)
 
 	/* read super block */
 	twiddle(1);
-	if ((rc = ffs_sbget(f, &fs, -1, 0, ufs_use_sa_read)) != 0)
+	if ((rc = ffs_sbget(f, &fs, -1, "stand", ufs_use_sa_read)) != 0)
 		goto out;
 	fp->f_fs = fs;
 	/*
@@ -688,7 +688,6 @@ ufs_use_sa_read(void *devfd, off_t loc, void **bufp, i
 	int error;
 
 	f = (struct open_file *)devfd;
-	free(*bufp);
 	if ((*bufp = malloc(size)) == NULL)
 		return (ENOSPC);
 	error = (f->f_dev->dv_strategy)(f->f_devdata, F_READ, loc / DEV_BSIZE,

Modified: head/sys/geom/geom_io.c
==============================================================================
--- head/sys/geom/geom_io.c	Fri Mar  2 03:05:36 2018	(r330263)
+++ head/sys/geom/geom_io.c	Fri Mar  2 04:34:53 2018	(r330264)
@@ -957,6 +957,9 @@ g_use_g_read_data(void *devfd, off_t loc, void **bufp,
 {
 	struct g_consumer *cp;
 
+	KASSERT(*bufp == NULL,
+	    ("g_use_g_read_data: non-NULL *bufp %p\n", *bufp));
+
 	cp = (struct g_consumer *)devfd;
 	/*
 	 * Take care not to issue an invalid I/O request. The offset of
@@ -966,8 +969,6 @@ g_use_g_read_data(void *devfd, off_t loc, void **bufp,
 	 */
 	if (loc % cp->provider->sectorsize != 0)
 		return (ENOENT);
-	if (*bufp != NULL)
-		g_free(*bufp);
 	*bufp = g_read_data(cp, loc, size, NULL);
 	if (*bufp == NULL)
 		return (ENOENT);

Modified: head/sys/geom/journal/g_journal_ufs.c
==============================================================================
--- head/sys/geom/journal/g_journal_ufs.c	Fri Mar  2 03:05:36 2018	(r330263)
+++ head/sys/geom/journal/g_journal_ufs.c	Fri Mar  2 04:34:53 2018	(r330264)
@@ -72,11 +72,11 @@ g_journal_ufs_dirty(struct g_consumer *cp)
 
 	fs = NULL;
 	if (SBLOCKSIZE % cp->provider->sectorsize != 0 ||
-	    ffs_sbget(cp, &fs, -1, NULL, g_use_g_read_data) != 0) {
+	    ffs_sbget(cp, &fs, -1, M_GEOM, g_use_g_read_data) != 0) {
 		GJ_DEBUG(0, "Cannot find superblock to mark file system %s "
 		    "as dirty.", cp->provider->name);
-		if (fs != NULL)
-			g_free(fs);
+		KASSERT(fs == NULL,
+		    ("g_journal_ufs_dirty: non-NULL fs %p\n", fs));
 		return;
 	}
 	GJ_DEBUG(0, "clean=%d flags=0x%x", fs->fs_clean, fs->fs_flags);

Modified: head/sys/geom/label/g_label_ufs.c
==============================================================================
--- head/sys/geom/label/g_label_ufs.c	Fri Mar  2 03:05:36 2018	(r330263)
+++ head/sys/geom/label/g_label_ufs.c	Fri Mar  2 04:34:53 2018	(r330264)
@@ -77,9 +77,9 @@ g_label_ufs_taste_common(struct g_consumer *cp, char *
 
 	fs = NULL;
 	if (SBLOCKSIZE % pp->sectorsize != 0 ||
-	    ffs_sbget(cp, &fs, -1, NULL, g_use_g_read_data) != 0) {
-		if (fs != NULL)
-			g_free(fs);
+	    ffs_sbget(cp, &fs, -1, M_GEOM, g_use_g_read_data) != 0) {
+		KASSERT(fs == NULL,
+		    ("g_label_ufs_taste_common: non-NULL fs %p\n",fs);
 		return;
 	}
 

Modified: head/sys/ufs/ffs/ffs_subr.c
==============================================================================
--- head/sys/ufs/ffs/ffs_subr.c	Fri Mar  2 03:05:36 2018	(r330263)
+++ head/sys/ufs/ffs/ffs_subr.c	Fri Mar  2 04:34:53 2018	(r330264)
@@ -151,6 +151,7 @@ static int readsuper(void *, struct fs **, off_t, int,
  * superblock. Memory is allocated for the superblock by the readfunc and
  * is returned. If filltype is non-NULL, additional memory is allocated
  * of type filltype and filled in with the superblock summary information.
+ * All memory is freed when any error is returned.
  *
  * If a superblock is found, zero is returned. Otherwise one of the
  * following error values is returned:
@@ -172,16 +173,24 @@ ffs_sbget(void *devfd, struct fs **fsp, off_t altsbloc
 	int32_t *lp;
 	char *buf;
 
+	fs = NULL;
 	*fsp = NULL;
 	if (altsblock != -1) {
-		if ((error = readsuper(devfd, fsp, altsblock, 1,
-		     readfunc)) != 0)
+		if ((error = readsuper(devfd, &fs, altsblock, 1,
+		     readfunc)) != 0) {
+			if (fs != NULL)
+				UFS_FREE(fs, filltype);
 			return (error);
+		}
 	} else {
 		for (i = 0; sblock_try[i] != -1; i++) {
-			if ((error = readsuper(devfd, fsp, sblock_try[i], 0,
+			if ((error = readsuper(devfd, &fs, sblock_try[i], 0,
 			     readfunc)) == 0)
 				break;
+			if (fs != NULL) {
+				UFS_FREE(fs, filltype);
+				fs = NULL;
+			}
 			if (error == ENOENT)
 				continue;
 			return (error);
@@ -190,20 +199,18 @@ ffs_sbget(void *devfd, struct fs **fsp, off_t altsbloc
 			return (ENOENT);
 	}
 	/*
-	 * If not filling in summary information, return.
-	 */
-	if (filltype == NULL)
-		return (0);
-	/*
 	 * Read in the superblock summary information.
 	 */
-	fs = *fsp;
 	size = fs->fs_cssize;
 	blks = howmany(size, fs->fs_fsize);
 	if (fs->fs_contigsumsize > 0)
 		size += fs->fs_ncg * sizeof(int32_t);
 	size += fs->fs_ncg * sizeof(u_int8_t);
-	space = UFS_MALLOC(size, filltype, M_WAITOK);
+	/* When running in libufs or libsa, UFS_MALLOC may fail */
+	if ((space = UFS_MALLOC(size, filltype, M_WAITOK)) == NULL) {
+		UFS_FREE(fs, filltype);
+		return (ENOSPC);
+	}
 	fs->fs_csp = (struct csum *)space;
 	for (i = 0; i < blks; i += fs->fs_frag) {
 		size = fs->fs_bsize;
@@ -213,9 +220,10 @@ ffs_sbget(void *devfd, struct fs **fsp, off_t altsbloc
 		error = (*readfunc)(devfd,
 		    dbtob(fsbtodb(fs, fs->fs_csaddr + i)), (void **)&buf, size);
 		if (error) {
-			UFS_FREE(buf, filltype);
+			if (buf != NULL)
+				UFS_FREE(buf, filltype);
 			UFS_FREE(fs->fs_csp, filltype);
-			fs->fs_csp = NULL;
+			UFS_FREE(fs, filltype);
 			return (error);
 		}
 		memcpy(space, buf, size);
@@ -231,6 +239,7 @@ ffs_sbget(void *devfd, struct fs **fsp, off_t altsbloc
 	size = fs->fs_ncg * sizeof(u_int8_t);
 	fs->fs_contigdirs = (u_int8_t *)space;
 	bzero(fs->fs_contigdirs, size);
+	*fsp = fs;
 	return (0);
 }
 
@@ -246,8 +255,6 @@ readsuper(void *devfd, struct fs **fsp, off_t sblocklo
 	int error;
 
 	error = (*readfunc)(devfd, sblockloc, (void **)fsp, SBLOCKSIZE);
-	if (*fsp != NULL)
-		(*fsp)->fs_csp = NULL;	/* Not yet any summary information */
 	if (error != 0)
 		return (error);
 	fs = *fsp;
@@ -263,6 +270,8 @@ readsuper(void *devfd, struct fs **fsp, off_t sblocklo
 	    fs->fs_bsize >= roundup(sizeof(struct fs), DEV_BSIZE)) {
 		/* Have to set for old filesystems that predate this field */
 		fs->fs_sblockactualloc = sblockloc;
+		/* Not yet any summary information */
+		fs->fs_csp = NULL;
 		return (0);
 	}
 	return (ENOENT);

Modified: head/sys/ufs/ffs/ffs_vfsops.c
==============================================================================
--- head/sys/ufs/ffs/ffs_vfsops.c	Fri Mar  2 03:05:36 2018	(r330263)
+++ head/sys/ufs/ffs/ffs_vfsops.c	Fri Mar  2 04:34:53 2018	(r330264)
@@ -1075,14 +1075,11 @@ ffs_use_bread(void *devfd, off_t loc, void **bufp, int
 	struct buf *bp;
 	int error;
 
-	free(*bufp, M_UFSMNT);
+	KASSERT(*bufp == NULL, ("ffs_use_bread: non-NULL *bufp %p\n", *bufp));
 	*bufp = malloc(size, M_UFSMNT, M_WAITOK);
 	if ((error = bread((struct vnode *)devfd, btodb(loc), size, NOCRED,
-	    &bp)) != 0) {
-		free(*bufp, M_UFSMNT);
-		*bufp = NULL;
+	    &bp)) != 0)
 		return (error);
-	}
 	bcopy(bp->b_data, *bufp, size);
 	bp->b_flags |= B_INVAL | B_NOCACHE;
 	brelse(bp);



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201803020434.w224Yr9J023438>