[Qemu-devel] [PATCH v2 0/3] block/file-posix: File locking during creation

[Qemu-devel] [PATCH v2 0/3] block/file-posix: File locking during creation

[Max Reitz]

[Unchanged text from v1 ahead]

Currently we do not take permissions on a file while it is being
created.  That is a bit sad.  The simplest way to test this is the
following:

    $ qemu-img create -f qcow2 foo.qcow2 64M
    Formatting 'foo.qcow2', fmt=qcow2 size=67108864 cluster_size=65536 lazy_refcounts=off refcount_bits=16
    $ qemu-img convert -f qcow2 -O qcow2 foo.qcow2 foo.qcow2
    qemu-img: foo.qcow2: error while converting qcow2: Failed to get "write" lock
    Is another process using the image?
    $ qemu-img info foo.qcow2
    image: foo.qcow2
    file format: raw
    virtual size: 0 (0 bytes)
    disk size: 0

(See also https://bugzilla.redhat.com/show_bug.cgi?id=1519144)

Here is what's happening: file-posix opens the file with
O_CREAT | O_TRUNC, thus truncating the file to 0.  Then qcow2 tries to
format it, but fails because it needs the WRITE permission to do so.  So
it cannot format it and the file stays empty.

We should actually take a WRITE and a RESIZE permission before we
truncate the file, and this is what this series does.

I personally consider the solution taken here a bit of a hack, but it
works and we don't have any locking in any drivers but file-posix
anyway, so it isn't lacking anything in that regard.  Integrating it in
blockdev-create might be possible, but there are two issues:
(1) It would be harder.
(2) What would we do anyway?  We'd advise protocol drivers to take WRITE
    and RESIZE permissions before they truncate a file to be empty...
    Well, and then they'd do exactly what file-posix is made to do in
    this series.

So basically what I consider a hack could be seen as exactly the right
way to tackle the issue: Protocol drivers have to ensure the correct
permissions (and they have to choose what those are) are applied before
changing any file -- which is what this series implements.


v2:
- Patch 2: [Fam]
  - Add a note that while not sharing the RESIZE permission does protect
    us from other block-layer-infused programs resizing the file while
    raw_co_create() is active; it does not protect against anyone
    resizing the file afterwards.
  - Drop the unnecessary second raw_apply_lock_bytes()


git-backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/3:[----] [--] 'block/file-posix: Pass FD to locking helpers'
002/3:[0016] [FC] 'block/file-posix: File locking during creation'
003/3:[----] [--] 'iotests: Add creation test to 153'


Max Reitz (3):
  block/file-posix: Pass FD to locking helpers
  block/file-posix: File locking during creation
  iotests: Add creation test to 153

 block/file-posix.c         | 64 +++++++++++++++++++++++++++++++++++-----------
 tests/qemu-iotests/153     | 18 +++++++++++++
 tests/qemu-iotests/153.out | 13 ++++++++++
 3 files changed, 80 insertions(+), 15 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH v2 1/3] block/file-posix: Pass FD to locking helpers

[Max Reitz]

raw_apply_lock_bytes() and raw_check_lock_bytes() currently take a
BDRVRawState *, but they only use the lock_fd field.  During image
creation, we do not have a BDRVRawState, but we do have an FD; so if we
want to reuse the functions there, we should modify them to receive only
the FD.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
 block/file-posix.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 3794c0007a..c98a4a1556 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -630,7 +630,7 @@ typedef enum {
  * file; if @unlock == true, also unlock the unneeded bytes.
  * @shared_perm_lock_bits is the mask of all permissions that are NOT shared.
  */
-static int raw_apply_lock_bytes(BDRVRawState *s,
+static int raw_apply_lock_bytes(int fd,
                                 uint64_t perm_lock_bits,
                                 uint64_t shared_perm_lock_bits,
                                 bool unlock, Error **errp)
@@ -641,13 +641,13 @@ static int raw_apply_lock_bytes(BDRVRawState *s,
     PERM_FOREACH(i) {
         int off = RAW_LOCK_PERM_BASE + i;
         if (perm_lock_bits & (1ULL << i)) {
-            ret = qemu_lock_fd(s->lock_fd, off, 1, false);
+            ret = qemu_lock_fd(fd, off, 1, false);
             if (ret) {
                 error_setg(errp, "Failed to lock byte %d", off);
                 return ret;
             }
         } else if (unlock) {
-            ret = qemu_unlock_fd(s->lock_fd, off, 1);
+            ret = qemu_unlock_fd(fd, off, 1);
             if (ret) {
                 error_setg(errp, "Failed to unlock byte %d", off);
                 return ret;
@@ -657,13 +657,13 @@ static int raw_apply_lock_bytes(BDRVRawState *s,
     PERM_FOREACH(i) {
         int off = RAW_LOCK_SHARED_BASE + i;
         if (shared_perm_lock_bits & (1ULL << i)) {
-            ret = qemu_lock_fd(s->lock_fd, off, 1, false);
+            ret = qemu_lock_fd(fd, off, 1, false);
             if (ret) {
                 error_setg(errp, "Failed to lock byte %d", off);
                 return ret;
             }
         } else if (unlock) {
-            ret = qemu_unlock_fd(s->lock_fd, off, 1);
+            ret = qemu_unlock_fd(fd, off, 1);
             if (ret) {
                 error_setg(errp, "Failed to unlock byte %d", off);
                 return ret;
@@ -674,8 +674,7 @@ static int raw_apply_lock_bytes(BDRVRawState *s,
 }
 
 /* Check "unshared" bytes implied by @perm and ~@shared_perm in the file. */
-static int raw_check_lock_bytes(BDRVRawState *s,
-                                uint64_t perm, uint64_t shared_perm,
+static int raw_check_lock_bytes(int fd, uint64_t perm, uint64_t shared_perm,
                                 Error **errp)
 {
     int ret;
@@ -685,7 +684,7 @@ static int raw_check_lock_bytes(BDRVRawState *s,
         int off = RAW_LOCK_SHARED_BASE + i;
         uint64_t p = 1ULL << i;
         if (perm & p) {
-            ret = qemu_lock_fd_test(s->lock_fd, off, 1, true);
+            ret = qemu_lock_fd_test(fd, off, 1, true);
             if (ret) {
                 char *perm_name = bdrv_perm_names(p);
                 error_setg(errp,
@@ -702,7 +701,7 @@ static int raw_check_lock_bytes(BDRVRawState *s,
         int off = RAW_LOCK_PERM_BASE + i;
         uint64_t p = 1ULL << i;
         if (!(shared_perm & p)) {
-            ret = qemu_lock_fd_test(s->lock_fd, off, 1, true);
+            ret = qemu_lock_fd_test(fd, off, 1, true);
             if (ret) {
                 char *perm_name = bdrv_perm_names(p);
                 error_setg(errp,
@@ -739,11 +738,11 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
 
     switch (op) {
     case RAW_PL_PREPARE:
-        ret = raw_apply_lock_bytes(s, s->perm | new_perm,
+        ret = raw_apply_lock_bytes(s->lock_fd, s->perm | new_perm,
                                    ~s->shared_perm | ~new_shared,
                                    false, errp);
         if (!ret) {
-            ret = raw_check_lock_bytes(s, new_perm, new_shared, errp);
+            ret = raw_check_lock_bytes(s->lock_fd, new_perm, new_shared, errp);
             if (!ret) {
                 return 0;
             }
@@ -751,7 +750,8 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
         op = RAW_PL_ABORT;
         /* fall through to unlock bytes. */
     case RAW_PL_ABORT:
-        raw_apply_lock_bytes(s, s->perm, ~s->shared_perm, true, &local_err);
+        raw_apply_lock_bytes(s->lock_fd, s->perm, ~s->shared_perm,
+                             true, &local_err);
         if (local_err) {
             /* Theoretically the above call only unlocks bytes and it cannot
              * fail. Something weird happened, report it.
@@ -760,7 +760,8 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
         }
         break;
     case RAW_PL_COMMIT:
-        raw_apply_lock_bytes(s, new_perm, ~new_shared, true, &local_err);
+        raw_apply_lock_bytes(s->lock_fd, new_perm, ~new_shared,
+                             true, &local_err);
         if (local_err) {
             /* Theoretically the above call only unlocks bytes and it cannot
              * fail. Something weird happened, report it.
-- 
2.14.3

[Qemu-devel] [PATCH v2 2/3] block/file-posix: File locking during creation

[Max Reitz]

When creating a file, we should take the WRITE and RESIZE permissions.
We do not need either for the creation itself, but we do need them for
clearing and resizing it.  So we can take the proper permissions by
replacing O_TRUNC with an explicit truncation to 0, and by taking the
appropriate file locks between those two steps.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/file-posix.c | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index c98a4a1556..370a4833e4 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1992,6 +1992,7 @@ static int raw_co_create(BlockdevCreateOptions *options, Error **errp)
 {
     BlockdevCreateOptionsFile *file_opts;
     int fd;
+    int perm, shared;
     int result = 0;
 
     /* Validate options and set default values */
@@ -2006,14 +2007,44 @@ static int raw_co_create(BlockdevCreateOptions *options, Error **errp)
     }
 
     /* Create file */
-    fd = qemu_open(file_opts->filename, O_RDWR | O_CREAT | O_TRUNC | O_BINARY,
-                   0644);
+    fd = qemu_open(file_opts->filename, O_RDWR | O_CREAT | O_BINARY, 0644);
     if (fd < 0) {
         result = -errno;
         error_setg_errno(errp, -result, "Could not create file");
         goto out;
     }
 
+    /* Take permissions: We want to discard everything, so we need
+     * BLK_PERM_WRITE; and truncation to the desired size requires
+     * BLK_PERM_RESIZE.
+     * On the other hand, we cannot share the RESIZE permission
+     * because we promise that after this function, the file has the
+     * size given in the options.  If someone else were to resize it
+     * concurrently, we could not guarantee that.
+     * Note that after this function, we can no longer guarantee that
+     * the file is not touched by a third party, so it may be resized
+     * then. */
+    perm = BLK_PERM_WRITE | BLK_PERM_RESIZE;
+    shared = BLK_PERM_ALL & ~BLK_PERM_RESIZE;
+
+    /* Step one: Take locks */
+    result = raw_apply_lock_bytes(fd, perm, shared, false, errp);
+    if (result < 0) {
+        goto out_close;
+    }
+
+    /* Step two: Check that nobody else has taken conflicting locks */
+    result = raw_check_lock_bytes(fd, perm, shared, errp);
+    if (result < 0) {
+        goto out_close;
+    }
+
+    /* Clear the file by truncating it to 0 */
+    result = raw_regular_truncate(fd, 0, PREALLOC_MODE_OFF, errp);
+    if (result < 0) {
+        goto out_close;
+    }
+
     if (file_opts->nocow) {
 #ifdef __linux__
         /* Set NOCOW flag to solve performance issue on fs like btrfs.
@@ -2029,6 +2060,8 @@ static int raw_co_create(BlockdevCreateOptions *options, Error **errp)
 #endif
     }
 
+    /* Resize and potentially preallocate the file to the desired
+     * final size */
     result = raw_regular_truncate(fd, file_opts->size, file_opts->preallocation,
                                   errp);
     if (result < 0) {
-- 
2.14.3

[Qemu-devel] [PATCH v2 3/3] iotests: Add creation test to 153

[Max Reitz]

This patch adds a test case to 153 which tries to overwrite an image
(using qemu-img create) while it is in use.  Without the original user
explicitly sharing the necessary permissions (writing and truncation),
this should not be allowed.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/153     | 18 ++++++++++++++++++
 tests/qemu-iotests/153.out | 13 +++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/tests/qemu-iotests/153 b/tests/qemu-iotests/153
index a0fd815483..1be83912f8 100755
--- a/tests/qemu-iotests/153
+++ b/tests/qemu-iotests/153
@@ -137,6 +137,24 @@ for opts1 in "" "read-only=on" "read-only=on,force-share=on"; do
         _run_cmd $QEMU_IMG dd          $L if="${TEST_IMG}" of="${TEST_IMG}.convert" bs=512 count=1
         _run_cmd $QEMU_IMG bench       $L -c 1 "${TEST_IMG}"
         _run_cmd $QEMU_IMG bench       $L -w -c 1 "${TEST_IMG}"
+
+        # qemu-img create does not support -U
+        if [ -z "$L" ]; then
+            _run_cmd $QEMU_IMG create -f $IMGFMT "${TEST_IMG}" \
+                                      -b ${TEST_IMG}.base
+            # Read the file format.  It used to be the case that
+            # file-posix simply truncated the file, but the qcow2
+            # driver then failed to format it because it was unable
+            # to acquire the necessary WRITE permission.  However, the
+            # truncation was already wrong, and the whole process
+            # resulted in the file being completely empty and thus its
+            # format would be detected to be raw.
+            # So we read it here to see that creation either completed
+            # successfully (thus the format is qcow2) or it aborted
+            # before the file was changed at all (thus the format stays
+            # qcow2).
+            _img_info -U | grep 'file format'
+        fi
     done
     _send_qemu_cmd $h "{ 'execute': 'quit', }" ""
     echo
diff --git a/tests/qemu-iotests/153.out b/tests/qemu-iotests/153.out
index bb721cb747..e8eb369cae 100644
--- a/tests/qemu-iotests/153.out
+++ b/tests/qemu-iotests/153.out
@@ -92,6 +92,11 @@ _qemu_img_wrapper bench -w -c 1 TEST_DIR/t.qcow2
 qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to get "write" lock
 Is another process using the image?
 
+_qemu_img_wrapper create -f qcow2 TEST_DIR/t.qcow2 -b TEST_DIR/t.qcow2.base
+qemu-img: TEST_DIR/t.qcow2: Failed to get "write" lock
+Is another process using the image?
+file format: IMGFMT
+
 == Running utility commands -U ==
 
 _qemu_io_wrapper -U -c read 0 512 TEST_DIR/t.qcow2
@@ -209,6 +214,11 @@ _qemu_img_wrapper bench -w -c 1 TEST_DIR/t.qcow2
 qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to get "write" lock
 Is another process using the image?
 
+_qemu_img_wrapper create -f qcow2 TEST_DIR/t.qcow2 -b TEST_DIR/t.qcow2.base
+qemu-img: TEST_DIR/t.qcow2: Failed to get "write" lock
+Is another process using the image?
+file format: IMGFMT
+
 == Running utility commands -U ==
 
 _qemu_io_wrapper -U -c read 0 512 TEST_DIR/t.qcow2
@@ -309,6 +319,9 @@ _qemu_img_wrapper bench -c 1 TEST_DIR/t.qcow2
 
 _qemu_img_wrapper bench -w -c 1 TEST_DIR/t.qcow2
 
+_qemu_img_wrapper create -f qcow2 TEST_DIR/t.qcow2 -b TEST_DIR/t.qcow2.base
+file format: IMGFMT
+
 == Running utility commands -U ==
 
 _qemu_io_wrapper -U -c read 0 512 TEST_DIR/t.qcow2
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 2/3] block/file-posix: File locking during creation

[Fam Zheng]

On Wed, 05/09 23:53, Max Reitz wrote:
> When creating a file, we should take the WRITE and RESIZE permissions.
> We do not need either for the creation itself, but we do need them for
> clearing and resizing it.  So we can take the proper permissions by
> replacing O_TRUNC with an explicit truncation to 0, and by taking the
> appropriate file locks between those two steps.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v2 0/3] block/file-posix: File locking during creation

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3301 bytes --]

On 2018-05-09 23:53, Max Reitz wrote:
> [Unchanged text from v1 ahead]
> 
> Currently we do not take permissions on a file while it is being
> created.  That is a bit sad.  The simplest way to test this is the
> following:
> 
>     $ qemu-img create -f qcow2 foo.qcow2 64M
>     Formatting 'foo.qcow2', fmt=qcow2 size=67108864 cluster_size=65536 lazy_refcounts=off refcount_bits=16
>     $ qemu-img convert -f qcow2 -O qcow2 foo.qcow2 foo.qcow2
>     qemu-img: foo.qcow2: error while converting qcow2: Failed to get "write" lock
>     Is another process using the image?
>     $ qemu-img info foo.qcow2
>     image: foo.qcow2
>     file format: raw
>     virtual size: 0 (0 bytes)
>     disk size: 0
> 
> (See also https://bugzilla.redhat.com/show_bug.cgi?id=1519144)
> 
> Here is what's happening: file-posix opens the file with
> O_CREAT | O_TRUNC, thus truncating the file to 0.  Then qcow2 tries to
> format it, but fails because it needs the WRITE permission to do so.  So
> it cannot format it and the file stays empty.
> 
> We should actually take a WRITE and a RESIZE permission before we
> truncate the file, and this is what this series does.
> 
> I personally consider the solution taken here a bit of a hack, but it
> works and we don't have any locking in any drivers but file-posix
> anyway, so it isn't lacking anything in that regard.  Integrating it in
> blockdev-create might be possible, but there are two issues:
> (1) It would be harder.
> (2) What would we do anyway?  We'd advise protocol drivers to take WRITE
>     and RESIZE permissions before they truncate a file to be empty...
>     Well, and then they'd do exactly what file-posix is made to do in
>     this series.
> 
> So basically what I consider a hack could be seen as exactly the right
> way to tackle the issue: Protocol drivers have to ensure the correct
> permissions (and they have to choose what those are) are applied before
> changing any file -- which is what this series implements.
> 
> 
> v2:
> - Patch 2: [Fam]
>   - Add a note that while not sharing the RESIZE permission does protect
>     us from other block-layer-infused programs resizing the file while
>     raw_co_create() is active; it does not protect against anyone
>     resizing the file afterwards.
>   - Drop the unnecessary second raw_apply_lock_bytes()
> 
> 
> git-backport-diff against v1:
> 
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
> 
> 001/3:[----] [--] 'block/file-posix: Pass FD to locking helpers'
> 002/3:[0016] [FC] 'block/file-posix: File locking during creation'
> 003/3:[----] [--] 'iotests: Add creation test to 153'
> 
> 
> Max Reitz (3):
>   block/file-posix: Pass FD to locking helpers
>   block/file-posix: File locking during creation
>   iotests: Add creation test to 153
> 
>  block/file-posix.c         | 64 +++++++++++++++++++++++++++++++++++-----------
>  tests/qemu-iotests/153     | 18 +++++++++++++
>  tests/qemu-iotests/153.out | 13 ++++++++++
>  3 files changed, 80 insertions(+), 15 deletions(-)

Applied to my block branch.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]