1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/8] Yet another round of qemu namespace fixes
  4. View patch

[libvirt] [PATCH 2/8] qemuDomainBuildNamespace: Handle special file mount points

Michal Privoznik posted 8 patches 8 years, 7 months ago
[libvirt] [PATCH 2/8] qemuDomainBuildNamespace: Handle special file mount points
Posted by Michal Privoznik 8 years, 7 months ago 
https://bugzilla.redhat.com/show_bug.cgi?id=1459592

In 290a00e41d I've tried to fix the process of building a
qemu namespace when dealing with file mount points. What I
haven't realized then is that we might be dealing not with just
regular files but also special files (like sockets). Indeed, try
the following:

1) socat unix-listen:/tmp/soket stdio
2) touch /dev/socket
3) mount --bind /tmp/socket /dev/socket
4) virsh start anyDomain

Problem with my previous approach is that I wasn't creating the
temporary location (where mount points under /dev are moved) for
anything but directories and regular files.

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

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 8e7404da6..212717c80 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8356,9 +8356,11 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
             goto cleanup;
         }
 
-        /* At this point, devMountsPath is either a regular file or a directory. */
+        /* At this point, devMountsPath is either:
+         * a file (regular or special), or
+         * a directory. */
         if ((S_ISDIR(sb.st_mode) && virFileMakePath(devMountsSavePath[i]) < 0) ||
-            (S_ISREG(sb.st_mode) && virFileTouch(devMountsSavePath[i], sb.st_mode) < 0)) {
+            (!S_ISDIR(sb.st_mode) && virFileTouch(devMountsSavePath[i], sb.st_mode) < 0)) {
             virReportSystemError(errno,
                                  _("Failed to create %s"),
                                  devMountsSavePath[i]);
-- 
2.13.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/8] qemuDomainBuildNamespace: Handle special file mount points
Posted by John Ferlan 8 years, 7 months ago 

On 06/22/2017 12:18 PM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1459592
> 
> In 290a00e41d I've tried to fix the process of building a
> qemu namespace when dealing with file mount points. What I
> haven't realized then is that we might be dealing not with just
> regular files but also special files (like sockets). Indeed, try
> the following:
> 
> 1) socat unix-listen:/tmp/soket stdio
> 2) touch /dev/socket
> 3) mount --bind /tmp/socket /dev/socket
> 4) virsh start anyDomain
> 
> Problem with my previous approach is that I wasn't creating the
> temporary location (where mount points under /dev are moved) for
> anything but directories and regular files.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_domain.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 8e7404da6..212717c80 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -8356,9 +8356,11 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
>              goto cleanup;
>          }
>  
> -        /* At this point, devMountsPath is either a regular file or a directory. */
> +        /* At this point, devMountsPath is either:
> +         * a file (regular or special), or
> +         * a directory. */
>          if ((S_ISDIR(sb.st_mode) && virFileMakePath(devMountsSavePath[i]) < 0) ||
> -            (S_ISREG(sb.st_mode) && virFileTouch(devMountsSavePath[i], sb.st_mode) < 0)) {
> +            (!S_ISDIR(sb.st_mode) && virFileTouch(devMountsSavePath[i], sb.st_mode) < 0)) {

It would seem to me that this would open Pandora's box to all different
types of things (BLK, CHR, FIFO, LNK, NAM, MPB, MPC, NWK) - some of
which it may not be so popular to perform a touch on.

I think you should keep it specific...  Perhaps use the list from
qemuDomainCreateDeviceRecursive:

   isReg = S_ISREG(sb.st_mode) || S_ISFIFO(sb.st_mode) ||
S_ISSOCK(sb.st_mode);

John

(FWIW: I used a cscope search on S_ISSOCK...)


>              virReportSystemError(errno,
>                                   _("Failed to create %s"),
>                                   devMountsSavePath[i]);
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/8] qemuDomainBuildNamespace: Handle special file mount points
Posted by John Ferlan 8 years, 7 months ago 

On 06/27/2017 05:37 PM, John Ferlan wrote:
> 
> 
> On 06/22/2017 12:18 PM, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1459592
>>
>> In 290a00e41d I've tried to fix the process of building a
>> qemu namespace when dealing with file mount points. What I
>> haven't realized then is that we might be dealing not with just
>> regular files but also special files (like sockets). Indeed, try
>> the following:
>>
>> 1) socat unix-listen:/tmp/soket stdio
>> 2) touch /dev/socket
>> 3) mount --bind /tmp/socket /dev/socket
>> 4) virsh start anyDomain
>>
>> Problem with my previous approach is that I wasn't creating the
>> temporary location (where mount points under /dev are moved) for
>> anything but directories and regular files.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/qemu/qemu_domain.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 8e7404da6..212717c80 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -8356,9 +8356,11 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
>>              goto cleanup;
>>          }
>>  
>> -        /* At this point, devMountsPath is either a regular file or a directory. */
>> +        /* At this point, devMountsPath is either:
>> +         * a file (regular or special), or
>> +         * a directory. */
>>          if ((S_ISDIR(sb.st_mode) && virFileMakePath(devMountsSavePath[i]) < 0) ||
>> -            (S_ISREG(sb.st_mode) && virFileTouch(devMountsSavePath[i], sb.st_mode) < 0)) {
>> +            (!S_ISDIR(sb.st_mode) && virFileTouch(devMountsSavePath[i], sb.st_mode) < 0)) {
> 
> It would seem to me that this would open Pandora's box to all different
> types of things (BLK, CHR, FIFO, LNK, NAM, MPB, MPC, NWK) - some of
> which it may not be so popular to perform a touch on.
> 
> I think you should keep it specific...  Perhaps use the list from
> qemuDomainCreateDeviceRecursive:
> 
>    isReg = S_ISREG(sb.st_mode) || S_ISFIFO(sb.st_mode) ||
> S_ISSOCK(sb.st_mode);
> 
> John
> 

I guess it's obvious now that I've actually paged in patches 4-8 that I
used the same sandbox I'm reviewing in order to make this comment!

I think the same concern applies though...

John

> (FWIW: I used a cscope search on S_ISSOCK...)
> 
> 
>>              virReportSystemError(errno,
>>                                   _("Failed to create %s"),
>>                                   devMountsSavePath[i]);
>>
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/8] qemuDomainBuildNamespace: Handle special file mount points
Posted by Michal Privoznik 8 years, 7 months ago 
On 06/27/2017 11:37 PM, John Ferlan wrote:
> 
> 
> On 06/22/2017 12:18 PM, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1459592
>>
>> In 290a00e41d I've tried to fix the process of building a
>> qemu namespace when dealing with file mount points. What I
>> haven't realized then is that we might be dealing not with just
>> regular files but also special files (like sockets). Indeed, try
>> the following:
>>
>> 1) socat unix-listen:/tmp/soket stdio
>> 2) touch /dev/socket
>> 3) mount --bind /tmp/socket /dev/socket
>> 4) virsh start anyDomain
>>
>> Problem with my previous approach is that I wasn't creating the
>> temporary location (where mount points under /dev are moved) for
>> anything but directories and regular files.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>  src/qemu/qemu_domain.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 8e7404da6..212717c80 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -8356,9 +8356,11 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
>>              goto cleanup;
>>          }
>>  
>> -        /* At this point, devMountsPath is either a regular file or a directory. */
>> +        /* At this point, devMountsPath is either:
>> +         * a file (regular or special), or
>> +         * a directory. */
>>          if ((S_ISDIR(sb.st_mode) && virFileMakePath(devMountsSavePath[i]) < 0) ||
>> -            (S_ISREG(sb.st_mode) && virFileTouch(devMountsSavePath[i], sb.st_mode) < 0)) {
>> +            (!S_ISDIR(sb.st_mode) && virFileTouch(devMountsSavePath[i], sb.st_mode) < 0)) {
> 
> It would seem to me that this would open Pandora's box to all different
> types of things (BLK, CHR, FIFO, LNK, NAM, MPB, MPC, NWK) - some of
> which it may not be so popular to perform a touch on.
> 
> I think you should keep it specific...  Perhaps use the list from
> qemuDomainCreateDeviceRecursive:
> 
>    isReg = S_ISREG(sb.st_mode) || S_ISFIFO(sb.st_mode) ||
> S_ISSOCK(sb.st_mode);

Not really. mount --bind makes the target to be the correct type. IOW:

# create a regular file
touch /tmp/blah

# here, assume /source/socket is a socket
mount --bind /source/socket /tmp/blah

# now, /tmp/blah will be type of socket too

The only problem here is that for file based 'devices' (or things in
general) we have to create the file whereas for directories we have to
create directories. Just like in the example.

BTW, what type of file are NAM, MPB, MPC, NWK?

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/8] qemuDomainBuildNamespace: Handle special file mount points
Posted by John Ferlan 8 years, 7 months ago 

On 07/11/2017 01:14 AM, Michal Privoznik wrote:
> On 06/27/2017 11:37 PM, John Ferlan wrote:
>>
>>
>> On 06/22/2017 12:18 PM, Michal Privoznik wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1459592
>>>
>>> In 290a00e41d I've tried to fix the process of building a
>>> qemu namespace when dealing with file mount points. What I
>>> haven't realized then is that we might be dealing not with just
>>> regular files but also special files (like sockets). Indeed, try
>>> the following:
>>>
>>> 1) socat unix-listen:/tmp/soket stdio
>>> 2) touch /dev/socket
>>> 3) mount --bind /tmp/socket /dev/socket
>>> 4) virsh start anyDomain
>>>
>>> Problem with my previous approach is that I wasn't creating the
>>> temporary location (where mount points under /dev are moved) for
>>> anything but directories and regular files.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>> ---
>>>  src/qemu/qemu_domain.c | 6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>>> index 8e7404da6..212717c80 100644
>>> --- a/src/qemu/qemu_domain.c
>>> +++ b/src/qemu/qemu_domain.c
>>> @@ -8356,9 +8356,11 @@ qemuDomainBuildNamespace(virQEMUDriverConfigPtr cfg,
>>>              goto cleanup;
>>>          }
>>>  
>>> -        /* At this point, devMountsPath is either a regular file or a directory. */
>>> +        /* At this point, devMountsPath is either:
>>> +         * a file (regular or special), or
>>> +         * a directory. */
>>>          if ((S_ISDIR(sb.st_mode) && virFileMakePath(devMountsSavePath[i]) < 0) ||
>>> -            (S_ISREG(sb.st_mode) && virFileTouch(devMountsSavePath[i], sb.st_mode) < 0)) {
>>> +            (!S_ISDIR(sb.st_mode) && virFileTouch(devMountsSavePath[i], sb.st_mode) < 0)) {
>>
>> It would seem to me that this would open Pandora's box to all different
>> types of things (BLK, CHR, FIFO, LNK, NAM, MPB, MPC, NWK) - some of
>> which it may not be so popular to perform a touch on.
>>
>> I think you should keep it specific...  Perhaps use the list from
>> qemuDomainCreateDeviceRecursive:
>>
>>    isReg = S_ISREG(sb.st_mode) || S_ISFIFO(sb.st_mode) ||
>> S_ISSOCK(sb.st_mode);
> 
> Not really. mount --bind makes the target to be the correct type. IOW:
> 

OK - I wasn't sure what all those other things were and going with
!IS_DIR just seemed to open the door to unsurety for me.  Since there
was other code that was more restrictive to decide "ifReg", I figured
using that same list would be fine, but I'm not sure if I ever check
where in the scheme of the path the other check is make.

If you're fine with how things are, then fine consider it...

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

> # create a regular file
> touch /tmp/blah
> 
> # here, assume /source/socket is a socket
> mount --bind /source/socket /tmp/blah
> 
> # now, /tmp/blah will be type of socket too
> 
> The only problem here is that for file based 'devices' (or things in
> general) we have to create the file whereas for directories we have to
> create directories. Just like in the example.
> 
> BTW, what type of file are NAM, MPB, MPC, NWK?
> 
> Michal
> 

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

Patchew 2.1-devel-8c64711

© 2016 - 2026 Red Hat, Inc.