| Message ID | 87mwuo4x9o.fsf@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
>
>
>
>
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; > >
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
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;