Patchwork 9pfs segfaults on chmod(special)

login
register
mail settings
Submitter Aneesh Kumar K.V
Date Feb. 28, 2013, 9:12 a.m.
Message ID <87mwuo4x9o.fsf@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/223837/
State New
Headers show

Comments

Aneesh Kumar K.V - Feb. 28, 2013, 9:12 a.m.
Michael Tokarev <mjt@tls.msk.ru> writes:

> When guest tries to chmod a block or char device file over 9pfs,
> the qemu process segfaults.
>
> On host:
>  qemu-system-x86_64 -virtfs local,path=/dev,security_model=mapped-file,mount_tag=tag
>
> On guest:
>  mount -t 9p -o trans=virtio tag /mnt
>  chmod 0777 /mnt/tty

any specific reason why you are trying 9p .u ?

>
> Result (for 1.4.0):
>
> Program received signal SIGSEGV, Segmentation fault.
> 0x566095af in v9mode_to_mode (mode=8389101, extension=0xc7584ef8)
>     at /build/kvm/git/hw/9pfs/virtio-9p.c:662
> 662	        if (extension && extension->data[0] == 'c') {
> (gdb) p *extension
> $1 = {size = 0, data = 0x0}
> (gdb) bt
> #0  0x566095af in v9mode_to_mode (mode=8389101, extension=0xc7584ef8)
>     at hw/9pfs/virtio-9p.c:662
> #1  0x5660f38b in v9fs_wstat (opaque=0xd250945c)
>     at hw/9pfs/virtio-9p.c:2635
> (gdb) frame 1
> #1  0x5660f38b in v9fs_wstat (opaque=0xd250945c)
>     at hw/9pfs/virtio-9p.c:2635
> 2635	        err = v9fs_co_chmod(pdu, &fidp->path,
>                             v9mode_to_mode(v9stat.mode,
>                                            &v9stat.extension));
> (gdb) p v9stat
> $2 = {size = 61, type = -1, dev = -1, qid = {type = -1 '\377', version = -1,
>     path = -1}, mode = 8389101, atime = -1, mtime = -1, length = -1, name = {
>     size = 0, data = 0x0}, uid = {size = 0, data = 0x0}, gid = {size = 0,
>     data = 0x0}, muid = {size = 0, data = 0x0}, extension = {size = 0,
>     data = 0x0}, n_uid = -1, n_gid = -1, n_muid = -1}
>
>
> Corresponding code in v9mode_to_mode():
>
>      if (mode & P9_STAT_MODE_DEVICE) {
>          if (extension && extension->data[0] == 'c') {
>              ret |= S_IFCHR;
>          } else {
>              ret |= S_IFBLK;
>      }
>
> This (static) function (v9mode_to_mode) is called from only one place,
> namely from v9fs_wstat(), and it always calls it with non-NULL
> `extension' argument: &v9stat.extension.
>
> Maybe the buffer (extension->data) should be passed to it instead of
> the whole structure (extension)?  Or the check be extended (or,
> since this function isn't called from any other place, _replaced_) to
> test for non-NULL ->data too?
>

Thanks for the detailed analysis. Something like below ?
Michael Tokarev - Feb. 28, 2013, 10:47 a.m.
28.02.2013 13:12, Aneesh Kumar K.V wrote:
> Michael Tokarev <mjt@tls.msk.ru> writes:
> 
>> When guest tries to chmod a block or char device file over 9pfs,
>> the qemu process segfaults.
>>
>> On host:
>>  qemu-system-x86_64 -virtfs local,path=/dev,security_model=mapped-file,mount_tag=tag
>>
>> On guest:
>>  mount -t 9p -o trans=virtio tag /mnt
>>  chmod 0777 /mnt/tty
> 
> any specific reason why you are trying 9p .u ?

Sorry?  What _is_ "9p .u" ? :)


[]
>> Maybe the buffer (extension->data) should be passed to it instead of
>> the whole structure (extension)?  Or the check be extended (or,
>> since this function isn't called from any other place, _replaced_) to
>> test for non-NULL ->data too?
>>
> 
> Thanks for the detailed analysis. Something like below ?
> 
> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> index f526467..073067f 100644
> --- a/hw/9pfs/virtio-9p.c
> +++ b/hw/9pfs/virtio-9p.c
> @@ -659,7 +659,7 @@ static mode_t v9mode_to_mode(uint32_t mode, V9fsString *extension)
>          ret |= S_IFIFO;
>      }
>      if (mode & P9_STAT_MODE_DEVICE) {
> -        if (extension && extension->data[0] == 'c') {
> +        if (extension->size && extension->data[0] == 'c') {
>              ret |= S_IFCHR;
>          } else {
>              ret |= S_IFBLK;
> 

Yeah, that looks much saner.  And it obviously works, just
like testing ext->data to be non-null.

It has been this way since about beginning (as in, for example,
1.1 version of qemu is also affected), and probably is a good
candidate for -stable too.

Reviewed-By: Michael Tokarev <mjt@tls.msk.ru>

Thank you!

/mjt
Mohan Kumar M - Feb. 28, 2013, 12:24 p.m.
Michael Tokarev <mjt@tls.msk.ru> writes:

> 28.02.2013 13:12, Aneesh Kumar K.V wrote:
>> Michael Tokarev <mjt@tls.msk.ru> writes:
>> 
>>> When guest tries to chmod a block or char device file over 9pfs,
>>> the qemu process segfaults.
>>>
>>> On host:
>>>  qemu-system-x86_64 -virtfs local,path=/dev,security_model=mapped-file,mount_tag=tag
>>>
>>> On guest:
>>>  mount -t 9p -o trans=virtio tag /mnt
>>>  chmod 0777 /mnt/tty
>> 
>> any specific reason why you are trying 9p .u ?
>
> Sorry?  What _is_ "9p .u" ? :)
>
Hi,

9p.u is the extension of 9p protocol developed during Linux porting of
9p. Original 9p was designed for Plan 9 operating system. 9p.u has
support for numerical uids, gids, additional mode and permissio bits.

But still 9p.u lacked support for full Linux VFS operations. Such as
xattrs, locking etc. In order to overcome these issues and address
functionalities provides by Linux VFS a new protocol 9p2000.L was
developed (http://code.google.com/p/diod/wiki/protocol)

By default 9p.u is used, you can override by that
 mount -t 9p -otrans=virtio,version=9p2000.L tag /mnt


>
> []
>>> Maybe the buffer (extension->data) should be passed to it instead of
>>> the whole structure (extension)?  Or the check be extended (or,
>>> since this function isn't called from any other place, _replaced_) to
>>> test for non-NULL ->data too?
>>>
>> 
>> Thanks for the detailed analysis. Something like below ?
>> 
>> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
>> index f526467..073067f 100644
>> --- a/hw/9pfs/virtio-9p.c
>> +++ b/hw/9pfs/virtio-9p.c
>> @@ -659,7 +659,7 @@ static mode_t v9mode_to_mode(uint32_t mode, V9fsString *extension)
>>          ret |= S_IFIFO;
>>      }
>>      if (mode & P9_STAT_MODE_DEVICE) {
>> -        if (extension && extension->data[0] == 'c') {
>> +        if (extension->size && extension->data[0] == 'c') {
>>              ret |= S_IFCHR;
>>          } else {
>>              ret |= S_IFBLK;
>> 
>
> Yeah, that looks much saner.  And it obviously works, just
> like testing ext->data to be non-null.
>
> It has been this way since about beginning (as in, for example,
> 1.1 version of qemu is also affected), and probably is a good
> candidate for -stable too.
>
> Reviewed-By: Michael Tokarev <mjt@tls.msk.ru>
>
> Thank you!
>
> /mjt
Michael Tokarev - Feb. 28, 2013, 2:09 p.m.
28.02.2013 16:24, M. Mohan Kumar wrote:
> Michael Tokarev <mjt@tls.msk.ru> writes:
>> 28.02.2013 13:12, Aneesh Kumar K.V wrote:

>>> any specific reason why you are trying 9p .u ?
>>
>> Sorry?  What _is_ "9p .u" ? :)
> 
> 9p.u is the extension of 9p protocol developed during Linux porting of
> 9p. Original 9p was designed for Plan 9 operating system. 9p.u has
> support for numerical uids, gids, additional mode and permissio bits.
> 
> But still 9p.u lacked support for full Linux VFS operations. Such as
> xattrs, locking etc. In order to overcome these issues and address
> functionalities provides by Linux VFS a new protocol 9p2000.L was
> developed (http://code.google.com/p/diod/wiki/protocol)
> 
> By default 9p.u is used, you can override by that
>  mount -t 9p -otrans=virtio,version=9p2000.L tag /mnt

Aha.  Well. Most likely the reason is
 a) the default is "wrong" and
 b) there's not much knowledge about even existance of other option.

I've got this report from someone else (a debian user) and am just
forwarding it here (after verifying and adding a backtrace etc), --
this is the first time I _ever_ used 9pfs ;)

The same probably applies to the other bug (random errors for
unwritable dirs -- note the subject was wrong, it is about
unWRITEable dirs, not unreadable).

So it looks like the default mode isn't being tested much,
and when the usage is "real", everyone is using the .L mode.
Still, I think, it'd be nice to fix things in the default
mode too ;)

Thank you!

/mjt
H. Peter Anvin - March 1, 2013, 7:38 p.m.
On 02/28/2013 04:24 AM, M. Mohan Kumar wrote:
> 
> By default 9p.u is used, you can override by that
>  mount -t 9p -otrans=virtio,version=9p2000.L tag /mnt
> 

Shouldn't we change that default?

	-hpa
Eric Van Hensbergen - March 1, 2013, 9 p.m.
Yeah, that's probably overdue -- it should gracefully downgrade to 9p2000.u
and/or 9p2000 anyways.

    -eric


On Fri, Mar 1, 2013 at 1:38 PM, H. Peter Anvin <hpa@zytor.com> wrote:

> On 02/28/2013 04:24 AM, M. Mohan Kumar wrote:
> >
> > By default 9p.u is used, you can override by that
> >  mount -t 9p -otrans=virtio,version=9p2000.L tag /mnt
> >
>
> Shouldn't we change that default?
>
>         -hpa
>
>
>
>
Michael Tokarev - May 19, 2013, 4:10 p.m.
Guys, are we playing with our sand-box toys or what?

Can we apply this maybe to 1.5??  It's just insane that
such a simple bugfixes, with lots of preceeding work to
identify it, and with users suffering, are being simply
ignored for months...

Thanks,

/mjt

28.02.2013 13:12, Aneesh Kumar K.V wrote:
> Michael Tokarev <mjt@tls.msk.ru> writes:
> 
>> When guest tries to chmod a block or char device file over 9pfs,
>> the qemu process segfaults.
>>
>> On host:
>>  qemu-system-x86_64 -virtfs local,path=/dev,security_model=mapped-file,mount_tag=tag
>>
>> On guest:
>>  mount -t 9p -o trans=virtio tag /mnt
>>  chmod 0777 /mnt/tty
> 
> any specific reason why you are trying 9p .u ?
> 
>>
>> Result (for 1.4.0):
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> 0x566095af in v9mode_to_mode (mode=8389101, extension=0xc7584ef8)
>>     at /build/kvm/git/hw/9pfs/virtio-9p.c:662
>> 662	        if (extension && extension->data[0] == 'c') {
>> (gdb) p *extension
>> $1 = {size = 0, data = 0x0}
>> (gdb) bt
>> #0  0x566095af in v9mode_to_mode (mode=8389101, extension=0xc7584ef8)
>>     at hw/9pfs/virtio-9p.c:662
>> #1  0x5660f38b in v9fs_wstat (opaque=0xd250945c)
>>     at hw/9pfs/virtio-9p.c:2635
>> (gdb) frame 1
>> #1  0x5660f38b in v9fs_wstat (opaque=0xd250945c)
>>     at hw/9pfs/virtio-9p.c:2635
>> 2635	        err = v9fs_co_chmod(pdu, &fidp->path,
>>                             v9mode_to_mode(v9stat.mode,
>>                                            &v9stat.extension));
>> (gdb) p v9stat
>> $2 = {size = 61, type = -1, dev = -1, qid = {type = -1 '\377', version = -1,
>>     path = -1}, mode = 8389101, atime = -1, mtime = -1, length = -1, name = {
>>     size = 0, data = 0x0}, uid = {size = 0, data = 0x0}, gid = {size = 0,
>>     data = 0x0}, muid = {size = 0, data = 0x0}, extension = {size = 0,
>>     data = 0x0}, n_uid = -1, n_gid = -1, n_muid = -1}
>>
>>
>> Corresponding code in v9mode_to_mode():
>>
>>      if (mode & P9_STAT_MODE_DEVICE) {
>>          if (extension && extension->data[0] == 'c') {
>>              ret |= S_IFCHR;
>>          } else {
>>              ret |= S_IFBLK;
>>      }
>>
>> This (static) function (v9mode_to_mode) is called from only one place,
>> namely from v9fs_wstat(), and it always calls it with non-NULL
>> `extension' argument: &v9stat.extension.
>>
>> Maybe the buffer (extension->data) should be passed to it instead of
>> the whole structure (extension)?  Or the check be extended (or,
>> since this function isn't called from any other place, _replaced_) to
>> test for non-NULL ->data too?
>>
> 
> Thanks for the detailed analysis. Something like below ?
> 
> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> index f526467..073067f 100644
> --- a/hw/9pfs/virtio-9p.c
> +++ b/hw/9pfs/virtio-9p.c
> @@ -659,7 +659,7 @@ static mode_t v9mode_to_mode(uint32_t mode, V9fsString *extension)
>          ret |= S_IFIFO;
>      }
>      if (mode & P9_STAT_MODE_DEVICE) {
> -        if (extension && extension->data[0] == 'c') {
> +        if (extension->size && extension->data[0] == 'c') {
>              ret |= S_IFCHR;
>          } else {
>              ret |= S_IFBLK;
> 
>
Aneesh Kumar K.V - May 20, 2013, 6:06 a.m.
Michael Tokarev <mjt@tls.msk.ru> writes:

> Guys, are we playing with our sand-box toys or what?
>
> Can we apply this maybe to 1.5??  It's just insane that
> such a simple bugfixes, with lots of preceeding work to
> identify it, and with users suffering, are being simply
> ignored for months...
>

Sorry for not looking at this before. I wanted to make sure that we are
not missing the extension information as part of protocol. Verified
that chmod should not carry the extension details. 

I have sent the patch and also added qemu-stable. 

-aneesh

Patch

diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index f526467..073067f 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -659,7 +659,7 @@  static mode_t v9mode_to_mode(uint32_t mode, V9fsString *extension)
         ret |= S_IFIFO;
     }
     if (mode & P9_STAT_MODE_DEVICE) {
-        if (extension && extension->data[0] == 'c') {
+        if (extension->size && extension->data[0] == 'c') {
             ret |= S_IFCHR;
         } else {
             ret |= S_IFBLK;