qemuDomainAttachDeviceMknodHelper: Remove symlink before creating it

[libvirt] [PATCH] qemuDomainAttachDeviceMknodHelper: Remove symlink before creating it

Posted by Michal Privoznik 6 years, 3 months ago

https://bugzilla.redhat.com/show_bug.cgi?id=1528502

So imagine you have /dev/blah symlink which points to /dev/sda.
You attach /dev/blah as disk to your domain. Libvirt correctly
creates the /dev/blah -> /dev/sda symlink in the qemu namespace.
However, then you detach the disk, change the symlink so that it
points to /dev/sdb and tries to attach the disk again. This time,
however, the attach fails (well, qemu attaches wrong disk)
because the code assumes that symlinks don't change. Well they
do.

This is inspired by test fix written by Eduardo Habkost.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_domain.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 70fb40650..5f29d1ad5 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -9737,13 +9737,23 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED,
 
     if (isLink) {
         VIR_DEBUG("Creating symlink %s -> %s", data->file, data->target);
+
+        /* First, unlink the symlink target. Symlinks change and
+         * therefore we have no guarantees that pre-existing
+         * symlink is still valid. */
+        if (unlink(data->file) < 0 &&
+            errno != ENOENT) {
+            virReportSystemError(errno,
+                                 _("Unable to remove symlink %s"),
+                                 data->file);
+            goto cleanup;
+        }
+
         if (symlink(data->target, data->file) < 0) {
-            if (errno != EEXIST) {
-                virReportSystemError(errno,
-                                     _("Unable to create symlink %s"),
-                                     data->target);
-                goto cleanup;
-            }
+            virReportSystemError(errno,
+                                 _("Unable to create symlink %s"),
+                                 data->target);
+            goto cleanup;
         } else {
             delDevice = true;
         }
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemuDomainAttachDeviceMknodHelper: Remove symlink before creating it

Posted by Andrea Bolognani 6 years, 3 months ago

On Thu, 2018-01-04 at 11:17 +0100, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1528502
> 
> So imagine you have /dev/blah symlink which points to /dev/sda.
> You attach /dev/blah as disk to your domain. Libvirt correctly
> creates the /dev/blah -> /dev/sda symlink in the qemu namespace.
> However, then you detach the disk, change the symlink so that it
> points to /dev/sdb and tries to attach the disk again. This time,
> however, the attach fails (well, qemu attaches wrong disk)
> because the code assumes that symlinks don't change. Well they
> do.
> 
> This is inspired by test fix written by Eduardo Habkost.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_domain.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 70fb40650..5f29d1ad5 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -9737,13 +9737,23 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid ATTRIBUTE_UNUSED,
>  
>      if (isLink) {
>          VIR_DEBUG("Creating symlink %s -> %s", data->file, data->target);
> +
> +        /* First, unlink the symlink target. Symlinks change and
> +         * therefore we have no guarantees that pre-existing
> +         * symlink is still valid. */
> +        if (unlink(data->file) < 0 &&

Here...

> +            errno != ENOENT) {
> +            virReportSystemError(errno,
> +                                 _("Unable to remove symlink %s"),
> +                                 data->file);

... and here, shouldn't you be using data->target instead
of data->file?

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemuDomainAttachDeviceMknodHelper: Remove symlink before creating it

Posted by Andrea Bolognani 6 years, 3 months ago

On Fri, 2018-01-05 at 12:14 +0100, Andrea Bolognani wrote:
> >      if (isLink) {
> >          VIR_DEBUG("Creating symlink %s -> %s", data->file, data->target);
> > +
> > +        /* First, unlink the symlink target. Symlinks change and
> > +         * therefore we have no guarantees that pre-existing
> > +         * symlink is still valid. */
> > +        if (unlink(data->file) < 0 &&
> 
> Here...
> 
> > +            errno != ENOENT) {
> > +            virReportSystemError(errno,
> > +                                 _("Unable to remove symlink %s"),
> > +                                 data->file);
> 
> ... and here, shouldn't you be using data->target instead
> of data->file?

Never mind, you got it right and I clearly need more coffee :)

However, later on:

> > +            virReportSystemError(errno,
> > +                                 _("Unable to create symlink %s"),
> > +                                 data->target);
> > +            goto cleanup;

You should use data->file here to be consistent.

Either way:

  Reviewed-by: Andrea Bolognani <abologna@redhat.com>

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemuDomainAttachDeviceMknodHelper: Remove symlink before creating it

Posted by Michal Privoznik 6 years, 3 months ago

On 01/05/2018 12:29 PM, Andrea Bolognani wrote:
> On Fri, 2018-01-05 at 12:14 +0100, Andrea Bolognani wrote:
>>>      if (isLink) {
>>>          VIR_DEBUG("Creating symlink %s -> %s", data->file, data->target);
>>> +
>>> +        /* First, unlink the symlink target. Symlinks change and
>>> +         * therefore we have no guarantees that pre-existing
>>> +         * symlink is still valid. */
>>> +        if (unlink(data->file) < 0 &&
>>
>> Here...
>>
>>> +            errno != ENOENT) {
>>> +            virReportSystemError(errno,
>>> +                                 _("Unable to remove symlink %s"),
>>> +                                 data->file);
>>
>> ... and here, shouldn't you be using data->target instead
>> of data->file?
> 
> Never mind, you got it right and I clearly need more coffee :)
> 
> However, later on:
> 
>>> +            virReportSystemError(errno,
>>> +                                 _("Unable to create symlink %s"),
>>> +                                 data->target);
>>> +            goto cleanup;
> 
> You should use data->file here to be consistent.

How about:

_("Unable to create symlink %s -> %s"), data->target, data->file

Here and above.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemuDomainAttachDeviceMknodHelper: Remove symlink before creating it

Posted by Andrea Bolognani 6 years, 3 months ago

On Fri, 2018-01-05 at 14:38 +0100, Michal Privoznik wrote:
> > However, later on:
> > 
> > > > +            virReportSystemError(errno,
> > > > +                                 _("Unable to create symlink %s"),
> > > > +                                 data->target);
> > > > +            goto cleanup;
> > 
> > You should use data->file here to be consistent.
> 
> How about:
> 
> _("Unable to create symlink %s -> %s"), data->target, data->file
> 
> Here and above.

Yeah, I thought about that too after sending the mail :)

I would go for a more explicit wording, though:

  _("Unable to create symlink %s (pointing to %s)"),
  data->file, data->target

or something along the lines.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list