Merge pull request #4991 from kzys/no-auto-clear

mount: setupLoop() doesn't work with with Autoclear

mount/losetup_linux.go

@@ -69,7 +69,10 @@ func getFreeLoopDev() (uint32, error) {
 	return uint32(num), nil
 }
 
-func setupLoopDev(backingFile, loopDev string, param LoopParams) error {
+// setupLoopDev attaches the backing file to the loop device and returns
+// the file handle for the loop device. The caller is responsible for
+// closing the file handle.
+func setupLoopDev(backingFile, loopDev string, param LoopParams) (*os.File, error) {
 	// 1. Open backing file and loop device
 	flags := os.O_RDWR
 	if param.Readonly {
@@ -78,19 +81,20 @@ func setupLoopDev(backingFile, loopDev string, param LoopParams) error {
 
 	back, err := os.OpenFile(backingFile, flags, 0)
 	if err != nil {
-		return errors.Wrapf(err, "could not open backing file: %s", backingFile)
+		return nil, errors.Wrapf(err, "could not open backing file: %s", backingFile)
 	}
 	defer back.Close()
 
+	// To make Autoclear (LO_FLAGS_AUTOCLEAR) work, this function intentionally
+	// doesn't close this file handle on defer.
 	loop, err := os.OpenFile(loopDev, flags, 0)
 	if err != nil {
-		return errors.Wrapf(err, "could not open loop device: %s", loopDev)
+		return nil, errors.Wrapf(err, "could not open loop device: %s", loopDev)
 	}
-	defer loop.Close()
 
 	// 2. Set FD
 	if _, _, err = ioctl(loop.Fd(), unix.LOOP_SET_FD, back.Fd()); err != nil {
-		return errors.Wrapf(err, "could not set loop fd for device: %s", loopDev)
+		return nil, errors.Wrapf(err, "could not set loop fd for device: %s", loopDev)
 	}
 
 	// 3. Set Info
@@ -110,7 +114,7 @@ func setupLoopDev(backingFile, loopDev string, param LoopParams) error {
 
 	_, _, err = ioctl(loop.Fd(), unix.LOOP_SET_STATUS64, uintptr(unsafe.Pointer(&info)))
 	if err == nil {
-		return nil
+		return loop, nil
 	}
 
 	if param.Direct {
@@ -119,13 +123,16 @@ func setupLoopDev(backingFile, loopDev string, param LoopParams) error {
 		info.Flags &= ^(uint32(unix.LO_FLAGS_DIRECT_IO))
 		_, _, err = ioctl(loop.Fd(), unix.LOOP_SET_STATUS64, uintptr(unsafe.Pointer(&info)))
 		if err == nil {
-			return nil
+			return loop, nil
 		}
 	}
 
-	// Cleanup loop fd and return error
+	// Cleanup loop fd and return error.
+	// If this function doesn't return this file handle, it must close the handle to
+	// prevent file descriptor leaks.
+	loop.Close()
 	_, _, _ = ioctl(loop.Fd(), unix.LOOP_CLR_FD, 0)
-	return errors.Errorf("failed to set loop device info: %v", err)
+	return nil, errors.Errorf("failed to set loop device info: %v", err)
 }
 
 // setupLoop looks for (and possibly creates) a free loop device, and
@@ -142,15 +149,16 @@ func setupLoopDev(backingFile, loopDev string, param LoopParams) error {
 // the loop device when done with it.
 //
 // Upon success, the file handle to the loop device is returned.
-func setupLoop(backingFile string, param LoopParams) (string, error) {
+func setupLoop(backingFile string, param LoopParams) (*os.File, error) {
 	for retry := 1; retry < 100; retry++ {
 		num, err := getFreeLoopDev()
 		if err != nil {
-			return "", err
+			return nil, err
 		}
 
 		loopDev := fmt.Sprintf(loopDevFormat, num)
-		if err := setupLoopDev(backingFile, loopDev, param); err != nil {
+		file, err := setupLoopDev(backingFile, loopDev, param)
+		if err != nil {
 			// Per util-linux/sys-utils/losetup.c:create_loop(),
 			// free loop device can race and we end up failing
 			// with EBUSY when trying to set it up.
@@ -159,13 +167,13 @@ func setupLoop(backingFile string, param LoopParams) (string, error) {
 				time.Sleep(time.Millisecond * time.Duration(rand.Intn(retry*10)))
 				continue
 			}
-			return "", err
+			return nil, err
 		}
 
-		return loopDev, nil
+		return file, nil
 	}
 
-	return "", errors.New("timeout creating new loopback device")
+	return nil, errors.New("timeout creating new loopback device")
 }
 
 func removeLoop(loopdev string) error {
@@ -181,7 +189,12 @@ func removeLoop(loopdev string) error {
 
 // Attach a specified backing file to a loop device
 func AttachLoopDevice(backingFile string) (string, error) {
-	return setupLoop(backingFile, LoopParams{})
+	file, err := setupLoop(backingFile, LoopParams{})
+	if err != nil {
+		return "", err
+	}
+	defer file.Close()
+	return file.Name(), nil
 }
 
 // Detach a loop device

mount/losetup_linux_test.go

@@ -64,12 +64,13 @@ func TestRoLoop(t *testing.T) {
 		}
 	}()
 
-	path, err := setupLoop(backingFile, LoopParams{Readonly: true, Autoclear: true})
+	file, err := setupLoop(backingFile, LoopParams{Readonly: true, Autoclear: true})
 	if err != nil {
 		t.Fatal(err)
 	}
+	defer file.Close()
 
-	if err := ioutil.WriteFile(path, randomData, os.ModePerm); err == nil {
+	if _, err := file.Write(randomData); err == nil {
 		t.Fatalf("writing to readonly loop device should fail")
 	}
 }
@@ -84,12 +85,13 @@ func TestRwLoop(t *testing.T) {
 		}
 	}()
 
-	path, err := setupLoop(backingFile, LoopParams{Autoclear: true})
+	file, err := setupLoop(backingFile, LoopParams{Autoclear: false})
 	if err != nil {
 		t.Fatal(err)
 	}
+	defer file.Close()
 
-	if err := ioutil.WriteFile(path, randomData, os.ModePerm); err != nil {
+	if _, err := file.Write(randomData); err != nil {
 		t.Fatal(err)
 	}
 }

mount/mount_linux.go

@@ -79,14 +79,16 @@ func (m *Mount) Mount(target string) (err error) {
 		// or remount with changed data
 		source := m.Source
 		if losetup {
-			devFile, err := setupLoop(m.Source, LoopParams{
+			loFile, err := setupLoop(m.Source, LoopParams{
 				Readonly:  oflags&unix.MS_RDONLY == unix.MS_RDONLY,
 				Autoclear: true})
 			if err != nil {
 				return err
 			}
+			defer loFile.Close()
+
 			// Mount the loop device instead
-			source = devFile
+			source = loFile.Name()
 		}
 		if err := mountAt(chdir, source, target, m.Type, uintptr(oflags), data); err != nil {
 			return err