[Qemu-devel] [PATCH 1/1] block/file-posix.c: fix not reopened lock file descriptor

[Qemu-devel] [PATCH 1/1] block/file-posix.c: fix not reopened lock file descriptor

[Dion Bosschieter]

In commit 244a5668106297378391b768e7288eb157616f64 another
file descriptor to BDRVRawState is added. When we try to issue the
reopen command only s->fd is reopened; lock_fd could still hold an old
file descriptor "possibly" pointing to another file.

- change raw_reopen_prepare so it checks use_lock from BDRVRawState and
tries to reopen lock_fd accordingly
- change raw_reopen_commit so it closes the old lock_fd on use_lock

Signed-off-by: Dion Bosschieter <dionbosschieter@gmail.com>
---
 block/file-posix.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index d7fb772c14..16d83fc49e 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -167,6 +167,7 @@ typedef struct BDRVRawState {
 
 typedef struct BDRVRawReopenState {
     int fd;
+    int lock_fd;
     int open_flags;
 } BDRVRawReopenState;
 
@@ -795,6 +796,7 @@ static int raw_reopen_prepare(BDRVReopenState *state,
     raw_parse_flags(state->flags, &rs->open_flags);
 
     rs->fd = -1;
+    rs->lock_fd = -1;
 
     int fcntl_flags = O_APPEND | O_NONBLOCK;
 #ifdef O_NOATIME
@@ -820,6 +822,17 @@ static int raw_reopen_prepare(BDRVReopenState *state,
                 rs->fd = -1;
             }
         }
+
+        if (s->use_lock) {
+            rs->lock_fd = qemu_dup(s->lock_fd);
+            if (rs->lock_fd >= 0) {
+                ret = fcntl_setfl(rs->lock_fd, rs->open_flags);
+                if (ret) {
+                    qemu_close(rs->lock_fd);
+                    rs->lock_fd = -1;
+                }
+            }
+        }
     }
 
     /* If we cannot use fcntl, or fcntl failed, fall back to qemu_open() */
@@ -835,6 +848,14 @@ static int raw_reopen_prepare(BDRVReopenState *state,
                 error_setg_errno(errp, errno, "Could not reopen file");
                 ret = -1;
             }
+
+            if (s->use_lock) {
+                rs->lock_fd = qemu_open(normalized_filename, rs->open_flags);
+                if (rs->lock_fd == -1) {
+                    error_setg_errno(errp, errno, "Could not reopen file for locking");
+                    ret = -1;
+                }
+            }
         }
     }
 
@@ -861,7 +882,11 @@ static void raw_reopen_commit(BDRVReopenState *state)
     s->open_flags = rs->open_flags;
 
     qemu_close(s->fd);
+    if (s->use_lock) {
+        qemu_close(s->lock_fd);
+    }
     s->fd = rs->fd;
+    s->lock_fd = rs->lock_fd;
 
     g_free(state->opaque);
     state->opaque = NULL;
-- 
2.14.2

Re: [Qemu-devel] [PATCH 1/1] block/file-posix.c: fix not reopened lock file descriptor

[Kevin Wolf]

[ Cc: qemu-block ]

Am 22.03.2018 um 18:20 hat Dion Bosschieter geschrieben:
> In commit 244a5668106297378391b768e7288eb157616f64 another
> file descriptor to BDRVRawState is added. When we try to issue the
> reopen command only s->fd is reopened; lock_fd could still hold an old
> file descriptor "possibly" pointing to another file.
> 
> - change raw_reopen_prepare so it checks use_lock from BDRVRawState and
> tries to reopen lock_fd accordingly
> - change raw_reopen_commit so it closes the old lock_fd on use_lock
> 
> Signed-off-by: Dion Bosschieter <dionbosschieter@gmail.com>

bdrv_reopen() is not meant for opening a different file, it is meant to
change the flags and options of the same file. Do you have a use case
where you would actually need to switch to a different file?

As far as I know, lock_fd was specifically introduced _because_ it stays
the same across reopen, so we don't need a racy release/reacquire pair.
Fam (CCed) should know more.

In any case, doesn't your patch drop all the locks without reacquiring
them on the new lock_fd?

Kevin

>  block/file-posix.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index d7fb772c14..16d83fc49e 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -167,6 +167,7 @@ typedef struct BDRVRawState {
>  
>  typedef struct BDRVRawReopenState {
>      int fd;
> +    int lock_fd;
>      int open_flags;
>  } BDRVRawReopenState;
>  
> @@ -795,6 +796,7 @@ static int raw_reopen_prepare(BDRVReopenState *state,
>      raw_parse_flags(state->flags, &rs->open_flags);
>  
>      rs->fd = -1;
> +    rs->lock_fd = -1;
>  
>      int fcntl_flags = O_APPEND | O_NONBLOCK;
>  #ifdef O_NOATIME
> @@ -820,6 +822,17 @@ static int raw_reopen_prepare(BDRVReopenState *state,
>                  rs->fd = -1;
>              }
>          }
> +
> +        if (s->use_lock) {
> +            rs->lock_fd = qemu_dup(s->lock_fd);
> +            if (rs->lock_fd >= 0) {
> +                ret = fcntl_setfl(rs->lock_fd, rs->open_flags);
> +                if (ret) {
> +                    qemu_close(rs->lock_fd);
> +                    rs->lock_fd = -1;
> +                }
> +            }
> +        }
>      }
>  
>      /* If we cannot use fcntl, or fcntl failed, fall back to qemu_open() */
> @@ -835,6 +848,14 @@ static int raw_reopen_prepare(BDRVReopenState *state,
>                  error_setg_errno(errp, errno, "Could not reopen file");
>                  ret = -1;
>              }
> +
> +            if (s->use_lock) {
> +                rs->lock_fd = qemu_open(normalized_filename, rs->open_flags);
> +                if (rs->lock_fd == -1) {
> +                    error_setg_errno(errp, errno, "Could not reopen file for locking");
> +                    ret = -1;
> +                }
> +            }
>          }
>      }
>  
> @@ -861,7 +882,11 @@ static void raw_reopen_commit(BDRVReopenState *state)
>      s->open_flags = rs->open_flags;
>  
>      qemu_close(s->fd);
> +    if (s->use_lock) {
> +        qemu_close(s->lock_fd);
> +    }
>      s->fd = rs->fd;
> +    s->lock_fd = rs->lock_fd;
>  
>      g_free(state->opaque);
>      state->opaque = NULL;
> -- 
> 2.14.2
> 

Re: [Qemu-devel] [PATCH 1/1] block/file-posix.c: fix not reopened lock file descriptor

[Dion Bosschieter]

Yeah I have a use case, before a last sync on a storage migration we suspend a VM -> send the last diffs -> mount the new storage server and after that we change a symlink -> call reopen -> check if all file descriptors are changed before resuming the VM.

Dion

> Op 22 mrt. 2018 om 18:39 heeft Kevin Wolf <kwolf@redhat.com> het volgende geschreven:
> 
> [ Cc: qemu-block ]
> 
> Am 22.03.2018 um 18:20 hat Dion Bosschieter geschrieben:
>> In commit 244a5668106297378391b768e7288eb157616f64 another
>> file descriptor to BDRVRawState is added. When we try to issue the
>> reopen command only s->fd is reopened; lock_fd could still hold an old
>> file descriptor "possibly" pointing to another file.
>> 
>> - change raw_reopen_prepare so it checks use_lock from BDRVRawState and
>> tries to reopen lock_fd accordingly
>> - change raw_reopen_commit so it closes the old lock_fd on use_lock
>> 
>> Signed-off-by: Dion Bosschieter <dionbosschieter@gmail.com>
> 
> bdrv_reopen() is not meant for opening a different file, it is meant to
> change the flags and options of the same file. Do you have a use case
> where you would actually need to switch to a different file?
> 
> As far as I know, lock_fd was specifically introduced _because_ it stays
> the same across reopen, so we don't need a racy release/reacquire pair.
> Fam (CCed) should know more.
> 
> In any case, doesn't your patch drop all the locks without reacquiring
> them on the new lock_fd?
> 
> Kevin
> 
>> block/file-posix.c | 25 +++++++++++++++++++++++++
>> 1 file changed, 25 insertions(+)
>> 
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index d7fb772c14..16d83fc49e 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -167,6 +167,7 @@ typedef struct BDRVRawState {
>> 
>> typedef struct BDRVRawReopenState {
>>     int fd;
>> +    int lock_fd;
>>     int open_flags;
>> } BDRVRawReopenState;
>> 
>> @@ -795,6 +796,7 @@ static int raw_reopen_prepare(BDRVReopenState *state,
>>     raw_parse_flags(state->flags, &rs->open_flags);
>> 
>>     rs->fd = -1;
>> +    rs->lock_fd = -1;
>> 
>>     int fcntl_flags = O_APPEND | O_NONBLOCK;
>> #ifdef O_NOATIME
>> @@ -820,6 +822,17 @@ static int raw_reopen_prepare(BDRVReopenState *state,
>>                 rs->fd = -1;
>>             }
>>         }
>> +
>> +        if (s->use_lock) {
>> +            rs->lock_fd = qemu_dup(s->lock_fd);
>> +            if (rs->lock_fd >= 0) {
>> +                ret = fcntl_setfl(rs->lock_fd, rs->open_flags);
>> +                if (ret) {
>> +                    qemu_close(rs->lock_fd);
>> +                    rs->lock_fd = -1;
>> +                }
>> +            }
>> +        }
>>     }
>> 
>>     /* If we cannot use fcntl, or fcntl failed, fall back to qemu_open() */
>> @@ -835,6 +848,14 @@ static int raw_reopen_prepare(BDRVReopenState *state,
>>                 error_setg_errno(errp, errno, "Could not reopen file");
>>                 ret = -1;
>>             }
>> +
>> +            if (s->use_lock) {
>> +                rs->lock_fd = qemu_open(normalized_filename, rs->open_flags);
>> +                if (rs->lock_fd == -1) {
>> +                    error_setg_errno(errp, errno, "Could not reopen file for locking");
>> +                    ret = -1;
>> +                }
>> +            }
>>         }
>>     }
>> 
>> @@ -861,7 +882,11 @@ static void raw_reopen_commit(BDRVReopenState *state)
>>     s->open_flags = rs->open_flags;
>> 
>>     qemu_close(s->fd);
>> +    if (s->use_lock) {
>> +        qemu_close(s->lock_fd);
>> +    }
>>     s->fd = rs->fd;
>> +    s->lock_fd = rs->lock_fd;
>> 
>>     g_free(state->opaque);
>>     state->opaque = NULL;
>> -- 
>> 2.14.2
>> 

Re: [Qemu-devel] [PATCH 1/1] block/file-posix.c: fix not reopened lock file descriptor

[Fam Zheng]

On Thu, 03/22 18:39, Kevin Wolf wrote:
> [ Cc: qemu-block ]
> 
> Am 22.03.2018 um 18:20 hat Dion Bosschieter geschrieben:
> > In commit 244a5668106297378391b768e7288eb157616f64 another
> > file descriptor to BDRVRawState is added. When we try to issue the
> > reopen command only s->fd is reopened; lock_fd could still hold an old
> > file descriptor "possibly" pointing to another file.
> > 
> > - change raw_reopen_prepare so it checks use_lock from BDRVRawState and
> > tries to reopen lock_fd accordingly
> > - change raw_reopen_commit so it closes the old lock_fd on use_lock
> > 
> > Signed-off-by: Dion Bosschieter <dionbosschieter@gmail.com>
> 
> bdrv_reopen() is not meant for opening a different file, it is meant to
> change the flags and options of the same file. Do you have a use case
> where you would actually need to switch to a different file?
> 
> As far as I know, lock_fd was specifically introduced _because_ it stays
> the same across reopen, so we don't need a racy release/reacquire pair.
> Fam (CCed) should know more.

I think (I remember we have discussed a bit before) techinically we can do
reopen just well without a separate s->lock_fd.  After all all locks we acquire
are shared lock, so it is possible to handle the lock between old fd and the new
one back and forth freely.

Fam

Re: [Qemu-devel] [PATCH 1/1] block/file-posix.c: fix not reopened lock file descriptor

[Fam Zheng]

On Thu, 03/22 19:08, Dion Bosschieter wrote:
> Yeah I have a use case, before a last sync on a storage migration we suspend a
> VM -> send the last diffs -> mount the new storage server and after that we
> change a symlink -> call reopen -> check if all file descriptors are changed
> before resuming the VM.

What is the point of changing the symlink and checking if FDs are changed?

Fam