[Qemu-devel] VirtIO 9p mount_tag (bogus?) limit of 32 bytes

[Qemu-devel] VirtIO 9p mount_tag (bogus?) limit of 32 bytes

[Daniel P. Berrange]

I've noticed that if you use a virtio 9p filesystem with a mount_tag
property value that is longer than 32 bytes, it gets silently truncated.

In virtio-9p-device.c

    len = strlen(conf->tag);
    if (len > MAX_TAG_LEN) {
        len = MAX_TAG_LEN;
    }


The header  virtio-9p.h contains


  /* from Linux's linux/virtio_9p.h */

  /* The ID for virtio console */
  #define VIRTIO_ID_9P    9
  #define MAX_REQ         128
  #define MAX_TAG_LEN     32


The Linux kernel's  virtio_9p.h, however, does not have any MAX_TAG_LEN
constant and AFAICT the code in Linux's net/9p/trans_virtio.c is not
placing any 32 byte length restriction on the mount tag.

So is this QEMU length limit legacy code that can be removed ?

If using the mount_tag to specify the desired guest mount location path,
then 32 bytes is really quite limiting - a good 255 bytes is much more
desirable.

Finally, regardless of what limit is imposed, it would be better to
return an error if the user attempts to specify an excessively long
mount tag, rather than truncate it breaking the guest app relying on
the full tag.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] VirtIO 9p mount_tag (bogus?) limit of 32 bytes

[Harsh Bora]

On 09/22/2011 11:12 PM, Daniel P. Berrange wrote:
> I've noticed that if you use a virtio 9p filesystem with a mount_tag
> property value that is longer than 32 bytes, it gets silently truncated.
>
> In virtio-9p-device.c
>
>      len = strlen(conf->tag);
>      if (len>  MAX_TAG_LEN) {
>          len = MAX_TAG_LEN;

I think its better to return here with a failure message saying 
mount_tag too long. IIUC, The 32 byte limit has been kept because of 
understanding that mount_tag is a device name in guest (and not a path 
location).

Aneesh, any inputs ?

- Harsh

>      }
>
>
> The header  virtio-9p.h contains
>
>
>    /* from Linux's linux/virtio_9p.h */
>
>    /* The ID for virtio console */
>    #define VIRTIO_ID_9P    9
>    #define MAX_REQ         128
>    #define MAX_TAG_LEN     32
>
>
> The Linux kernel's  virtio_9p.h, however, does not have any MAX_TAG_LEN
> constant and AFAICT the code in Linux's net/9p/trans_virtio.c is not
> placing any 32 byte length restriction on the mount tag.
>
> So is this QEMU length limit legacy code that can be removed ?
>
> If using the mount_tag to specify the desired guest mount location path,
> then 32 bytes is really quite limiting - a good 255 bytes is much more
> desirable.
>
> Finally, regardless of what limit is imposed, it would be better to
> return an error if the user attempts to specify an excessively long
> mount tag, rather than truncate it breaking the guest app relying on
> the full tag.
>
> Regards,
> Daniel

Re: [Qemu-devel] VirtIO 9p mount_tag (bogus?) limit of 32 bytes

[Daniel P. Berrange]

On Wed, Sep 28, 2011 at 05:22:06PM +0530, Harsh Bora wrote:
> On 09/22/2011 11:12 PM, Daniel P. Berrange wrote:
> >I've noticed that if you use a virtio 9p filesystem with a mount_tag
> >property value that is longer than 32 bytes, it gets silently truncated.
> >
> >In virtio-9p-device.c
> >
> >     len = strlen(conf->tag);
> >     if (len>  MAX_TAG_LEN) {
> >         len = MAX_TAG_LEN;
> 
> I think its better to return here with a failure message saying
> mount_tag too long. IIUC, The 32 byte limit has been kept because of
> understanding that mount_tag is a device name in guest (and not a
> path location).

While I appreciate the fact that 'mount_tag' is not required to be
a path name, so you can allow symbolic naming for exports, in some
use cases it is important / significantly simpler to be able to just
set a path name. I don't think we should mandate symbolic naming,
or path based naming - we should just allow users to choose which
best suits their needs.

For example, I am building appliances which have multiple 9p devices
exported to the guest. These 9p filesystems are all mounted by the
'init' process in the initrd. If I'm forced to use symbolic naming
for devices, it means I need to create a custom initrd for every
appliance configuration I have (many many many of them), with the
init process knowing how to map from symbolic names back to the
mount paths I actually want. If I can just use a path for the
mount_tag, then one single initrd can be used for all my appliances.

So I really would like 'mount_tag' to be significantly larger upto
at least 255 bytes, or more.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] VirtIO 9p mount_tag (bogus?) limit of 32 bytes

[Aneesh Kumar K.V]

On Wed, 28 Sep 2011 16:18:07 +0100, "Daniel P. Berrange" <berrange@redhat.com> wrote:
> On Wed, Sep 28, 2011 at 05:22:06PM +0530, Harsh Bora wrote:
> > On 09/22/2011 11:12 PM, Daniel P. Berrange wrote:
> > >I've noticed that if you use a virtio 9p filesystem with a mount_tag
> > >property value that is longer than 32 bytes, it gets silently truncated.
> > >
> > >In virtio-9p-device.c
> > >
> > >     len = strlen(conf->tag);
> > >     if (len>  MAX_TAG_LEN) {
> > >         len = MAX_TAG_LEN;
> > 
> > I think its better to return here with a failure message saying
> > mount_tag too long. IIUC, The 32 byte limit has been kept because of
> > understanding that mount_tag is a device name in guest (and not a
> > path location).
> 
> While I appreciate the fact that 'mount_tag' is not required to be
> a path name, so you can allow symbolic naming for exports, in some
> use cases it is important / significantly simpler to be able to just
> set a path name. I don't think we should mandate symbolic naming,
> or path based naming - we should just allow users to choose which
> best suits their needs.
> 
> For example, I am building appliances which have multiple 9p devices
> exported to the guest. These 9p filesystems are all mounted by the
> 'init' process in the initrd. If I'm forced to use symbolic naming
> for devices, it means I need to create a custom initrd for every
> appliance configuration I have (many many many of them), with the
> init process knowing how to map from symbolic names back to the
> mount paths I actually want. If I can just use a path for the
> mount_tag, then one single initrd can be used for all my appliances.
> 
> So I really would like 'mount_tag' to be significantly larger upto
> at least 255 bytes, or more.
> 

Will you not be able to have well defined mount tags, that map these
directories. I guess we don't want to claim 255 bytes out of config
space for mount tag. That is one of the reason it is limited to 32
bytes.

-aneesh

Re: [Qemu-devel] VirtIO 9p mount_tag (bogus?) limit of 32 bytes

[Daniel P. Berrange]

On Thu, Sep 29, 2011 at 08:52:14PM +0530, Aneesh Kumar K.V wrote:
> On Wed, 28 Sep 2011 16:18:07 +0100, "Daniel P. Berrange" <berrange@redhat.com> wrote:
> > On Wed, Sep 28, 2011 at 05:22:06PM +0530, Harsh Bora wrote:
> > > On 09/22/2011 11:12 PM, Daniel P. Berrange wrote:
> > > >I've noticed that if you use a virtio 9p filesystem with a mount_tag
> > > >property value that is longer than 32 bytes, it gets silently truncated.
> > > >
> > > >In virtio-9p-device.c
> > > >
> > > >     len = strlen(conf->tag);
> > > >     if (len>  MAX_TAG_LEN) {
> > > >         len = MAX_TAG_LEN;
> > > 
> > > I think its better to return here with a failure message saying
> > > mount_tag too long. IIUC, The 32 byte limit has been kept because of
> > > understanding that mount_tag is a device name in guest (and not a
> > > path location).
> > 
> > While I appreciate the fact that 'mount_tag' is not required to be
> > a path name, so you can allow symbolic naming for exports, in some
> > use cases it is important / significantly simpler to be able to just
> > set a path name. I don't think we should mandate symbolic naming,
> > or path based naming - we should just allow users to choose which
> > best suits their needs.
> > 
> > For example, I am building appliances which have multiple 9p devices
> > exported to the guest. These 9p filesystems are all mounted by the
> > 'init' process in the initrd. If I'm forced to use symbolic naming
> > for devices, it means I need to create a custom initrd for every
> > appliance configuration I have (many many many of them), with the
> > init process knowing how to map from symbolic names back to the
> > mount paths I actually want. If I can just use a path for the
> > mount_tag, then one single initrd can be used for all my appliances.
> > 
> > So I really would like 'mount_tag' to be significantly larger upto
> > at least 255 bytes, or more.
> > 
> 
> Will you not be able to have well defined mount tags, that map these
> directories. I guess we don't want to claim 255 bytes out of config
> space for mount tag. That is one of the reason it is limited to 32
> bytes.

The reason for using paths instead of symbolic names in the
mount tag is because the guest code does not know what paths it
might be asked to mount at runtime. Symbolic names in the mount
tags are only useful if the guest can be told ahead of time about
a finite set of tag -> path mappings, which is not a reasonable
assumption in general.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] VirtIO 9p mount_tag (bogus?) limit of 32 bytes

[C Anthony Risinger]

if this doesn't thread correctly ... RE:

http://lists.gnu.org/archive/html/qemu-devel/2011-09/msg03694.html

On Thu, 29 Sep 2011 16:45:37 +0100, Daniel P. Berrange wrote:
> On Thu, Sep 29, 2011 at 08:52:14PM +0530, Aneesh Kumar K.V wrote:
>> On Wed, 28 Sep 2011 16:18:07 +0100, Daniel P. Berrange wrote:
>>> On Wed, Sep 28, 2011 at 05:22:06PM +0530, Harsh Bora wrote:
>>>> On 09/22/2011 11:12 PM, Daniel P. Berrange wrote:
>>>>> I've noticed that if you use a virtio 9p filesystem with a mount_tag
>>>>> property value that is longer than 32 bytes, it gets silently truncated.
>>>>>
>>>>> In virtio-9p-device.c
>>>>>
>>>>>     len = strlen(conf->tag);
>>>>>     if (len>  MAX_TAG_LEN) {
>>>>>         len = MAX_TAG_LEN;
>>>>
>>>> I think its better to return here with a failure message saying
>>>> mount_tag too long. IIUC, The 32 byte limit has been kept because of
>>>> understanding that mount_tag is a device name in guest (and not a
>>>> path location).
>>>
>>> While I appreciate the fact that 'mount_tag' is not required to be
>>> a path name, so you can allow symbolic naming for exports, in some
>>> use cases it is important / significantly simpler to be able to just
>>> set a path name. I don't think we should mandate symbolic naming,
>>> or path based naming - we should just allow users to choose which
>>> best suits their needs.
>>>
>>> For example, I am building appliances which have multiple 9p devices
>>> exported to the guest. These 9p filesystems are all mounted by the
>>> 'init' process in the initrd. If I'm forced to use symbolic naming
>>> for devices, it means I need to create a custom initrd for every
>>> appliance configuration I have (many many many of them), with the
>>> init process knowing how to map from symbolic names back to the
>>> mount paths I actually want. If I can just use a path for the
>>> mount_tag, then one single initrd can be used for all my appliances.
>>>
>>> So I really would like 'mount_tag' to be significantly larger up to
>>> at least 255 bytes, or more.

32 bytes is very small! barely one UUID sans dashes.  i have exact
same use case ...

>> Will you not be able to have well defined mount tags, that map these
>> directories. I guess we don't want to claim 255 bytes out of config
>> space for mount tag. That is one of the reason it is limited to 32
>> bytes.
>
> The reason for using paths instead of symbolic names in the
> mount tag is because the guest code does not know what paths it
> might be asked to mount at runtime. Symbolic names in the mount
> tags are only useful if the guest can be told ahead of time about
> a finite set of tag -> path mappings, which is not a reasonable
> assumption in general.

i use UUIDs for everything ... ~2x the 32byte limit.  since i also
wanted a generic initramfs handler (Archlinux), i ended up working
around the issue by opening a virtio serial channel and marshaling the
information that way ... kind of hacky, but it does work.

i see an error message has been added, which is great (i killed a
couple hours of $%!@ until i noticed the truncated length was *exactly
32* bytes; silent truncation), but it would really be great if this
restriction could be lifted, or at least mitigated by expanding the
field some.

is config space that precious?  what constrains it (personal curiosity :-)?

PS: off-topic, but since i'm here ... (qemu-discuss is busted --
majordomo replies to everyone even though it's a mailman list --
unless this was fixed in last month)

a) mapped FS security policy (xattrs) causes `ldconfig` to abort()?
root or normal user ...

somehow `ldconfig` gets a duplicate inode while constructing the
cache, even though it already de-duped (confirmed via gdb and grep --
only a single abort() in the source)

b) unable to run `locale-gen` on *any* virtfs configuration? (strace)

[...]
mmap(NULL, 536870912, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
0x7fb3aac63000
mmap(0x7fb3aac63000, 103860, PROT_READ|PROT_WRITE,
MAP_SHARED|MAP_FIXED, 3, 0) = -1 EINVAL (Invalid argument)
cannot map archive header: Invalid argument

c) package files containing device nodes fail (maybe this is expected
...); specifically `/lib/udev/devices/loop0`

-- 

C Anthony

Re: [Qemu-devel] VirtIO 9p mount_tag (bogus?) limit of 32 bytes

[Paul Brook]

> i see an error message has been added, which is great (i killed a
> couple hours of $%!@ until i noticed the truncated length was *exactly
> 32* bytes; silent truncation), but it would really be great if this
> restriction could be lifted, or at least mitigated by expanding the
> field some.
> 
> is config space that precious?  

Yes.

> what constrains it (personal curiosity :-)?

Virtio PCI devices map the config space directly onto an ISA IO port range, 
along with the virtio control structure.  On most systems (in particular 
anything PC based) this is a 16-bit address space.  i.e. you have 
substantually less than 64k of virtio config space for the whole machine, and 
potentially only a couple of kbytes for a given PCI bus.

Paul

Re: [Qemu-devel] VirtIO 9p mount_tag (bogus?) limit of 32 bytes

[Aneesh Kumar K.V]

On Thu, 16 Feb 2012 06:20:21 -0600, C Anthony Risinger <anthony@xtfx.me> wrote:
> a) mapped FS security policy (xattrs) causes `ldconfig` to abort()?
> root or normal user ...
> 
> somehow `ldconfig` gets a duplicate inode while constructing the
> cache, even though it already de-duped (confirmed via gdb and grep --
> only a single abort() in the source)
> 
> b) unable to run `locale-gen` on *any* virtfs configuration? (strace)
> 
> [...]
> mmap(NULL, 536870912, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
> 0x7fb3aac63000
> mmap(0x7fb3aac63000, 103860, PROT_READ|PROT_WRITE,
> MAP_SHARED|MAP_FIXED, 3, 0) = -1 EINVAL (Invalid argument)
> cannot map archive header: Invalid argument
> 
> c) package files containing device nodes fail (maybe this is expected
> ...); specifically `/lib/udev/devices/loop0`
> 

Is this with 9p2000.L ?. What is the guest kernel version ?

-aneesh

Re: [Qemu-devel] VirtIO 9p mount_tag (bogus?) limit of 32 bytes

[C Anthony Risinger]

On Sat, Feb 18, 2012 at 11:38 AM, Aneesh Kumar K.V
<aneesh.kumar@linux.vnet.ibm.com> wrote:
> On Thu, 16 Feb 2012 06:20:21 -0600, C Anthony Risinger <anthony@xtfx.me> wrote:
>> a) mapped FS security policy (xattrs) causes `ldconfig` to abort()?
>> root or normal user ...
>>
>> somehow `ldconfig` gets a duplicate inode while constructing the
>> cache, even though it already de-duped (confirmed via gdb and grep --
>> only a single abort() in the source)
>>
>> b) unable to run `locale-gen` on *any* virtfs configuration? (strace)
>>
>> [...]
>> mmap(NULL, 536870912, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
>> 0x7fb3aac63000
>> mmap(0x7fb3aac63000, 103860, PROT_READ|PROT_WRITE,
>> MAP_SHARED|MAP_FIXED, 3, 0) = -1 EINVAL (Invalid argument)
>> cannot map archive header: Invalid argument
>>
>> c) package files containing device nodes fail (maybe this is expected
>> ...); specifically `/lib/udev/devices/loop0`
>>
>
> Is this with 9p2000.L ?. What is the guest kernel version ?

(not sure if list will accept this ... too much traffic! had to remove myself)

yes this is with 9p2000.L, both host and guests run kernel 3.2.5.  i'm
happy to provide/try additional information/tests if useful.

... is there really no chance of upping the max path?  seems like
config space will be a big constraint, forever :-(

and i'm very much willing to do additional testing for the other
issues as well (i had to revert to qemu-as-root to get passthru
working 100% on rootfs ... ldconfig is kind of critical :-).  are
these known issues?

-- 

C Anthony

Re: [Qemu-devel] VirtIO 9p mount_tag (bogus?) limit of 32 bytes

[Aneesh Kumar K.V]

On Tue, 21 Feb 2012 21:58:39 -0600, C Anthony Risinger <anthony@xtfx.me> wrote:
> On Sat, Feb 18, 2012 at 11:38 AM, Aneesh Kumar K.V
> <aneesh.kumar@linux.vnet.ibm.com> wrote:
> > On Thu, 16 Feb 2012 06:20:21 -0600, C Anthony Risinger <anthony@xtfx.me> wrote:
> >> a) mapped FS security policy (xattrs) causes `ldconfig` to abort()?
> >> root or normal user ...
> >>
> >> somehow `ldconfig` gets a duplicate inode while constructing the
> >> cache, even though it already de-duped (confirmed via gdb and grep --
> >> only a single abort() in the source)
> >>

I will try to reproduce this to get more info.


> >> b) unable to run `locale-gen` on *any* virtfs configuration? (strace)
> >>
> >> [...]
> >> mmap(NULL, 536870912, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
> >> 0x7fb3aac63000
> >> mmap(0x7fb3aac63000, 103860, PROT_READ|PROT_WRITE,
> >> MAP_SHARED|MAP_FIXED, 3, 0) = -1 EINVAL (Invalid argument)
> >> cannot map archive header: Invalid argument
> >>

For writable mmap to work you need to mount with -o cache=loose. Did
you try local-gen with that mount option ?


> >> c) package files containing device nodes fail (maybe this is expected
> >> ...); specifically `/lib/udev/devices/loop0`
> >>
> >

Does this mean mknod fails for you. Or is something else in package
manager causing the failure ? 


> > Is this with 9p2000.L ?. What is the guest kernel version ?
> 
> (not sure if list will accept this ... too much traffic! had to remove myself)
> 
> yes this is with 9p2000.L, both host and guests run kernel 3.2.5.  i'm
> happy to provide/try additional information/tests if useful.
> 

One quick thing you could do is to try latest linus kernel as the guest
kernel. 

> ... is there really no chance of upping the max path?  seems like
> config space will be a big constraint, forever :-(
> 
> and i'm very much willing to do additional testing for the other
> issues as well (i had to revert to qemu-as-root to get passthru
> working 100% on rootfs ... ldconfig is kind of critical :-).  are
> these known issues?
> 

I don't have much suggestion on what could be going wrong there. I will
try to reproduce the ldconfig issue here.

-aneesh

Re: [Qemu-devel] VirtIO 9p mount_tag (bogus?) limit of 32 bytes

[M. Mohan Kumar]

Hi Anthony,

When I tried with ldconfig version 2.14.90, ldconfig successfully completed

QEMU version: 1.0.50
Kernel version: 3.3.0-rc6+

Could you please try with recent ldconfig?

On 02/22/2012 09:28 AM, C Anthony Risinger wrote:
> On Sat, Feb 18, 2012 at 11:38 AM, Aneesh Kumar K.V
> <aneesh.kumar@linux.vnet.ibm.com>  wrote:
>> On Thu, 16 Feb 2012 06:20:21 -0600, C Anthony Risinger<anthony@xtfx.me>  wrote:
>>> a) mapped FS security policy (xattrs) causes `ldconfig` to abort()?
>>> root or normal user ...
>>>
>>> somehow `ldconfig` gets a duplicate inode while constructing the
>>> cache, even though it already de-duped (confirmed via gdb and grep --
>>> only a single abort() in the source)
>>>
>>> b) unable to run `locale-gen` on *any* virtfs configuration? (strace)
>>>
>>> [...]
>>> mmap(NULL, 536870912, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
>>> 0x7fb3aac63000
>>> mmap(0x7fb3aac63000, 103860, PROT_READ|PROT_WRITE,
>>> MAP_SHARED|MAP_FIXED, 3, 0) = -1 EINVAL (Invalid argument)
>>> cannot map archive header: Invalid argument
>>>
>>> c) package files containing device nodes fail (maybe this is expected
>>> ...); specifically `/lib/udev/devices/loop0`
>>>
>> Is this with 9p2000.L ?. What is the guest kernel version ?
> (not sure if list will accept this ... too much traffic! had to remove myself)
>
> yes this is with 9p2000.L, both host and guests run kernel 3.2.5.  i'm
> happy to provide/try additional information/tests if useful.
>
> ... is there really no chance of upping the max path?  seems like
> config space will be a big constraint, forever :-(
>
> and i'm very much willing to do additional testing for the other
> issues as well (i had to revert to qemu-as-root to get passthru
> working 100% on rootfs ... ldconfig is kind of critical :-).  are
> these known issues?
>