losetup: cleanup device node modes

The current code follows ro/rw mode not only set mode of the new
device, but also to open the device node for ioctls.

Linux kernel does not care and it seems O_RDONLY is good enough
for all cases (ioctls).

Unfortunately, udevd is sensitive as it monitors devices by inotify
and IN_CLOSE_WRITE event is expected to apply udev rules for the
device.

Changes:

* remove LOOPDEV_FL_{RDONLY,RDWR} private flags, it's too complex and
  unnecessary

* use mode_t for open() modes (rater than int)

* re-open only if O_RDWR requested otherwise default to O_RDONLY

* make sure O_RDWR is used on device setup

Fixes: https://github.com/util-linux/util-linux/issues/2434
Signed-off-by: Karel Zak <kzak@redhat.com>

diff --git a/include/loopdev.h b/include/loopdev.h
index 49e38f2115dbe651a4c1993d1968b59d87791cc0..0ec8aeeab1d5fa701e72e311081b56b8c3871e4b 100644 (file)
--- a/include/loopdev.h
+++ b/include/loopdev.h
@@ -112,7 +112,7 @@ struct loopdev_cxt {
        char            device[128];    /* device path (e.g. /dev/loop<N>) */
        char            *filename;      /* backing file for loopcxt_set_... */
        int             fd;             /* open(/dev/looo<N>) */
-       int             mode;           /* fd mode O_{RDONLY,RDWR} */
+       mode_t          mode;           /* fd mode O_{RDONLY,RDWR} */
        uint64_t        blocksize;      /* used by loopcxt_setup_device() */
 
        int             flags;          /* LOOPDEV_FL_* flags */
@@ -132,8 +132,6 @@ struct loopdev_cxt {
  * loopdev_cxt.flags
  */
 enum {
-       LOOPDEV_FL_RDONLY       = (1 << 0),     /* open(/dev/loop) mode; default */
-       LOOPDEV_FL_RDWR         = (1 << 1),     /* necessary for loop setup only */
        LOOPDEV_FL_OFFSET       = (1 << 4),
        LOOPDEV_FL_NOSYSFS      = (1 << 5),
        LOOPDEV_FL_NOIOCTL      = (1 << 6),
@@ -176,7 +174,7 @@ extern const char *loopcxt_get_device(struct loopdev_cxt *lc);
 extern struct loop_info64 *loopcxt_get_info(struct loopdev_cxt *lc);
 
 extern int loopcxt_get_fd(struct loopdev_cxt *lc);
-extern int loopcxt_set_fd(struct loopdev_cxt *lc, int fd, int mode);
+extern int loopcxt_set_fd(struct loopdev_cxt *lc, int fd, mode_t mode);
 
 extern int loopcxt_init_iterator(struct loopdev_cxt *lc, int flags);
 extern int loopcxt_deinit_iterator(struct loopdev_cxt *lc);

diff --git a/lib/loopdev.c b/lib/loopdev.c
index b3c537cd583b3df28a73191132576d6de2d2bd84..323f7bd50d39b92cebf9a51477621e68e4705fb4 100644 (file)
--- a/lib/loopdev.c
+++ b/lib/loopdev.c
@@ -116,7 +116,7 @@ int loopcxt_set_device(struct loopdev_cxt *lc, const char *device)
                DBG(CXT, ul_debugobj(lc, "closing old open fd"));
        }
        lc->fd = -1;
-       lc->mode = 0;
+       lc->mode = O_RDONLY;
        lc->blocksize = 0;
        lc->has_info = 0;
        lc->info_failed = 0;
@@ -165,12 +165,6 @@ int loopcxt_has_device(struct loopdev_cxt *lc)
  *
  *     * LO_FLAGS_* are kernel flags used for LOOP_{SET,GET}_STAT64 ioctls
  *
- * Note about LOOPDEV_FL_{RDONLY,RDWR} flags. These flags are used for open(2)
- * syscall to open loop device. By default is the device open read-only.
- *
- * The exception is loopcxt_setup_device(), where the device is open read-write
- * if LO_FLAGS_READ_ONLY flags is not set (see loopcxt_set_flags()).
- *
  * Returns: <0 on error, 0 on success.
  */
 int loopcxt_init(struct loopdev_cxt *lc, int flags)
@@ -285,28 +279,50 @@ static struct path_cxt *loopcxt_get_sysfs(struct loopdev_cxt *lc)
        return lc->sysfs;
 }
 
-/*
- * @lc: context
- *
- * Returns: file descriptor to the open loop device or <0 on error. The mode
- *          depends on LOOPDEV_FL_{RDWR,RDONLY} context flags. Default is
- *          read-only.
- */
-int loopcxt_get_fd(struct loopdev_cxt *lc)
+static int __loopcxt_get_fd(struct loopdev_cxt *lc, mode_t mode)
 {
+       int old = -1;
+
        if (!lc || !*lc->device)
                return -EINVAL;
 
+       /* It's okay to return a FD with read-write permissions if someone
+        * asked for read-only, but you shouldn't do the opposite.
+        *
+        * (O_RDONLY is a widely usable default.)
+        */
+       if (lc->fd >= 0 && mode == O_RDWR && lc->mode == O_RDONLY) {
+               DBG(CXT, ul_debugobj(lc, "closing already open device (mode mismatch)"));
+               old = lc->fd;
+               lc->fd = -1;
+       }
+
        if (lc->fd < 0) {
-               lc->mode = lc->flags & LOOPDEV_FL_RDWR ? O_RDWR : O_RDONLY;
+               lc->mode = mode;
                lc->fd = open(lc->device, lc->mode | O_CLOEXEC);
                DBG(CXT, ul_debugobj(lc, "open %s [%s]: %m", lc->device,
-                               lc->flags & LOOPDEV_FL_RDWR ? "rw" : "ro"));
+                               mode == O_RDONLY ? "ro" :
+                               mode == O_RDWR ? "rw" : "??"));
+
+               if (lc->fd < 0 && old >= 0) {
+                       /* restore original on error */
+                       lc->fd = old;
+                       old = -1;
+               }
        }
+
+       if (old >= 0)
+               close(old);
        return lc->fd;
 }
 
-int loopcxt_set_fd(struct loopdev_cxt *lc, int fd, int mode)
+/* default is read-only file descriptor, it's enough for all ioctls */
+int loopcxt_get_fd(struct loopdev_cxt *lc)
+{
+       return __loopcxt_get_fd(lc, O_RDONLY);
+}
+
+int loopcxt_set_fd(struct loopdev_cxt *lc, int fd, mode_t mode)
 {
        if (!lc)
                return -EINVAL;
@@ -1357,7 +1373,8 @@ static int loopcxt_check_size(struct loopdev_cxt *lc, int file_fd)
  */
 int loopcxt_setup_device(struct loopdev_cxt *lc)
 {
-       int file_fd, dev_fd, mode = O_RDWR, flags = O_CLOEXEC;
+       int file_fd, dev_fd;
+       mode_t flags = O_CLOEXEC, mode = O_RDWR;
        int rc = -1, cnt = 0;
        int errsv = 0;
        int fallback = 0;
@@ -1387,25 +1404,22 @@ int loopcxt_setup_device(struct loopdev_cxt *lc)
        }
        DBG(SETUP, ul_debugobj(lc, "backing file open: OK"));
 
-       if (lc->fd != -1 && lc->mode != mode) {
-               DBG(SETUP, ul_debugobj(lc, "closing already open device (mode mismatch)"));
-               close(lc->fd);
-               lc->fd = -1;
-               lc->mode = 0;
-       }
-
-       if (mode == O_RDONLY) {
-               lc->flags |= LOOPDEV_FL_RDONLY;                 /* open() mode */
+       if (mode == O_RDONLY)
                lc->config.info.lo_flags |= LO_FLAGS_READ_ONLY; /* kernel loopdev mode */
-       } else {
-               lc->flags |= LOOPDEV_FL_RDWR;                   /* open() mode */
+       else
                lc->config.info.lo_flags &= ~LO_FLAGS_READ_ONLY;
-               lc->flags &= ~LOOPDEV_FL_RDONLY;
-       }
 
        do {
                errno = 0;
-               dev_fd = loopcxt_get_fd(lc);
+
+               /* For the ioctls, it's enough to use O_RDONLY, but udevd
+                * monitor devices by inotify, and udevd needs IN_CLOSE_WRITE
+                * event to trigger probing of the new device.
+                *
+                * The mode used for the device does not have to match the mode
+                * used for the backing file.
+                */
+               dev_fd = __loopcxt_get_fd(lc, O_RDWR);
                if (dev_fd >= 0 || lc->control_ok == 0)
                        break;
                if (errno != EACCES && errno != ENOENT)