[PATCH 1/1] hostfs: fix UML crash

[PATCH 1/1] hostfs: fix UML crash

[Richard Weinberger]

365b1818 resized f_spare within struct statfs.
hostfs accesses f_spare directly and needs an update.

Signed-off-by: Richard Weinberger <richard@nod.at>
Reported-by: Toralf Förster <toralf.foerster@gmx.de>
Tested-by: Toralf Förster <toralf.foerster@gmx.de>
---
 fs/hostfs/hostfs_user.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/hostfs/hostfs_user.c b/fs/hostfs/hostfs_user.c
index 6777aa0..ce2f168 100644
--- a/fs/hostfs/hostfs_user.c
+++ b/fs/hostfs/hostfs_user.c
@@ -388,6 +388,6 @@ int do_statfs(char *root, long *bsize_out, long long *blocks_out,
 	spare_out[1] = buf.f_spare[1];
 	spare_out[2] = buf.f_spare[2];
 	spare_out[3] = buf.f_spare[3];
-	spare_out[4] = buf.f_spare[4];
+
 	return 0;
 }
-- 
1.6.6.1

Re: [PATCH 1/1] hostfs: fix UML crash

[Andrew Morton]

On Mon, 18 Oct 2010 18:36:54 +0200 Richard Weinberger <richard@nod.at> wrote:

> 365b1818 resized f_spare within struct statfs.
> hostfs accesses f_spare directly and needs an update.
> 
> Signed-off-by: Richard Weinberger <richard@nod.at>
> Reported-by: Toralf F__rster <toralf.foerster@gmx.de>
> Tested-by: Toralf F__rster <toralf.foerster@gmx.de>
> ---
>  fs/hostfs/hostfs_user.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/hostfs/hostfs_user.c b/fs/hostfs/hostfs_user.c
> index 6777aa0..ce2f168 100644
> --- a/fs/hostfs/hostfs_user.c
> +++ b/fs/hostfs/hostfs_user.c
> @@ -388,6 +388,6 @@ int do_statfs(char *root, long *bsize_out, long long *blocks_out,
>  	spare_out[1] = buf.f_spare[1];
>  	spare_out[2] = buf.f_spare[2];
>  	spare_out[3] = buf.f_spare[3];
> -	spare_out[4] = buf.f_spare[4];
> +
>  	return 0;
>  }

Thanks.

Is there any reason for hostfs to be playing with the f_spare field at all?

Re: [PATCH 1/1] hostfs: fix UML crash

[Richard Weinberger]

Am Montag 18 Oktober 2010, 20:40:39 schrieb Andrew Morton:
> On Mon, 18 Oct 2010 18:36:54 +0200 Richard Weinberger <richard@nod.at> 
wrote:
> > 365b1818 resized f_spare within struct statfs.
> > hostfs accesses f_spare directly and needs an update.
> > 
> > Signed-off-by: Richard Weinberger <richard@nod.at>
> > Reported-by: Toralf F__rster <toralf.foerster@gmx.de>
> > Tested-by: Toralf F__rster <toralf.foerster@gmx.de>
> > ---
> > 
> >  fs/hostfs/hostfs_user.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/fs/hostfs/hostfs_user.c b/fs/hostfs/hostfs_user.c
> > index 6777aa0..ce2f168 100644
> > --- a/fs/hostfs/hostfs_user.c
> > +++ b/fs/hostfs/hostfs_user.c
> > @@ -388,6 +388,6 @@ int do_statfs(char *root, long *bsize_out, long long
> > *blocks_out,
> > 
> >  	spare_out[1] = buf.f_spare[1];
> >  	spare_out[2] = buf.f_spare[2];
> >  	spare_out[3] = buf.f_spare[3];
> > 
> > -	spare_out[4] = buf.f_spare[4];
> > +
> > 
> >  	return 0;
> >  
> >  }
> 
> Thanks.
> 
> Is there any reason for hostfs to be playing with the f_spare field at all?

I'm not sure why hostfs is using this field.
Currently I'm reviewing UML. 
As soon I'm done with hostfs you'll receive a patch or an explanation. :-)

Cheers,
//richard

Re: [PATCH 1/1] hostfs: fix UML crash

[Andrew Morton]

On Mon, 18 Oct 2010 20:53:58 +0200
Richard Weinberger <richard@nod.at> wrote:

> Am Montag 18 Oktober 2010, 20:40:39 schrieb Andrew Morton:
> > On Mon, 18 Oct 2010 18:36:54 +0200 Richard Weinberger <richard@nod.at> 
> wrote:
> > > 365b1818 resized f_spare within struct statfs.
> > > hostfs accesses f_spare directly and needs an update.
> > > 
> > > Signed-off-by: Richard Weinberger <richard@nod.at>
> > > Reported-by: Toralf F__rster <toralf.foerster@gmx.de>
> > > Tested-by: Toralf F__rster <toralf.foerster@gmx.de>
> > > ---
> > > 
> > >  fs/hostfs/hostfs_user.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/fs/hostfs/hostfs_user.c b/fs/hostfs/hostfs_user.c
> > > index 6777aa0..ce2f168 100644
> > > --- a/fs/hostfs/hostfs_user.c
> > > +++ b/fs/hostfs/hostfs_user.c
> > > @@ -388,6 +388,6 @@ int do_statfs(char *root, long *bsize_out, long long
> > > *blocks_out,
> > > 
> > >  	spare_out[1] = buf.f_spare[1];
> > >  	spare_out[2] = buf.f_spare[2];
> > >  	spare_out[3] = buf.f_spare[3];
> > > 
> > > -	spare_out[4] = buf.f_spare[4];
> > > +
> > > 
> > >  	return 0;
> > >  
> > >  }
> > 
> > Thanks.
> > 
> > Is there any reason for hostfs to be playing with the f_spare field at all?
> 
> I'm not sure why hostfs is using this field.
> Currently I'm reviewing UML. 
> As soon I'm done with hostfs you'll receive a patch or an explanation. :-)

365b1818 added f_flags to `struct statfs' but didn't carry that over to
`struct compat_statfs' and hence didn't copy f_flags over into
compat_statfs.  Deliberate, or oversight?

Re: [PATCH 1/1] hostfs: fix UML crash

[Christoph Hellwig]

On Mon, Oct 18, 2010 at 01:02:08PM -0700, Andrew Morton wrote:
> > > 
> > > Is there any reason for hostfs to be playing with the f_spare field at all?

No, there is no reason at all.  do_statfs is only called by
hostfs_statfs, which is the hostfs implementation of ->statfs.  zeroing
of the spare fields is done by common code (vfs_statfs_native).

Can you submit a patch to just remove the spare_out argument to do_statfs?

> 365b1818 added f_flags to `struct statfs' but didn't carry that over to
> `struct compat_statfs' and hence didn't copy f_flags over into
> compat_statfs.  Deliberate, or oversight?

Oversight.

Re: [PATCH 1/1] hostfs: fix UML crash

[Richard Weinberger]

On Tue, 19 Oct 2010 03:11:25 +0200, Christoph Hellwig <hch@lst.de>
wrote:
> On Mon, Oct 18, 2010 at 01:02:08PM -0700, Andrew Morton wrote:
>> > >
>> > > Is there any reason for hostfs to be playing with the f_spare field at all?
> 
> No, there is no reason at all.  do_statfs is only called by
> hostfs_statfs, which is the hostfs implementation of ->statfs.  zeroing
> of the spare fields is done by common code (vfs_statfs_native).

Thanks!

> Can you submit a patch to just remove the spare_out argument to do_statfs?

Sure.

Re: [uml-devel] [PATCH 1/1] hostfs: fix UML crash

[Geert Uytterhoeven]

On Mon, Oct 18, 2010 at 20:40, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Mon, 18 Oct 2010 18:36:54 +0200 Richard Weinberger <richard@nod.at> wrote:
>> 365b1818 resized f_spare within struct statfs.
>> hostfs accesses f_spare directly and needs an update.
>>
>> Signed-off-by: Richard Weinberger <richard@nod.at>
>> Reported-by: Toralf F__rster <toralf.foerster@gmx.de>
>> Tested-by: Toralf F__rster <toralf.foerster@gmx.de>
>> ---
>>  fs/hostfs/hostfs_user.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/hostfs/hostfs_user.c b/fs/hostfs/hostfs_user.c
>> index 6777aa0..ce2f168 100644
>> --- a/fs/hostfs/hostfs_user.c
>> +++ b/fs/hostfs/hostfs_user.c
>> @@ -388,6 +388,6 @@ int do_statfs(char *root, long *bsize_out, long long *blocks_out,
>>       spare_out[1] = buf.f_spare[1];
>>       spare_out[2] = buf.f_spare[2];
>>       spare_out[3] = buf.f_spare[3];
>> -     spare_out[4] = buf.f_spare[4];
>> +
>>       return 0;
>>  }
>
> Thanks.
>
> Is there any reason for hostfs to be playing with the f_spare field at all?

It just copies it from struct statfs64 on the host to struct kstatfs
on the guest.
Probably a memcpy() is more future-safe, if it's combined with a
BUILD_BUG_ON(sizeof(statfs64.f_spare) != sizeof(kstatfs.f_spare)).

Still, currently it doesn't copy the recently added f_flags field.
To protect against future changes like that, an explicit
BUILD_BUG_ON(sizeof(kstatfs.f_spare) != 4*sizeof(long)) may be even
better...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [uml-devel] [PATCH 1/1] hostfs: fix UML crash

[Richard Weinberger]

Am Montag 18 Oktober 2010, 21:22:31 schrieb Geert Uytterhoeven:
> On Mon, Oct 18, 2010 at 20:40, Andrew Morton <akpm@linux-foundation.org> 
wrote:
> > On Mon, 18 Oct 2010 18:36:54 +0200 Richard Weinberger <richard@nod.at> 
wrote:
> >> 365b1818 resized f_spare within struct statfs.
> >> hostfs accesses f_spare directly and needs an update.
> >> 
> >> Signed-off-by: Richard Weinberger <richard@nod.at>
> >> Reported-by: Toralf F__rster <toralf.foerster@gmx.de>
> >> Tested-by: Toralf F__rster <toralf.foerster@gmx.de>
> >> ---
> >>  fs/hostfs/hostfs_user.c |    2 +-
> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >> 
> >> diff --git a/fs/hostfs/hostfs_user.c b/fs/hostfs/hostfs_user.c
> >> index 6777aa0..ce2f168 100644
> >> --- a/fs/hostfs/hostfs_user.c
> >> +++ b/fs/hostfs/hostfs_user.c
> >> @@ -388,6 +388,6 @@ int do_statfs(char *root, long *bsize_out, long long
> >> *blocks_out, spare_out[1] = buf.f_spare[1];
> >>       spare_out[2] = buf.f_spare[2];
> >>       spare_out[3] = buf.f_spare[3];
> >> -     spare_out[4] = buf.f_spare[4];
> >> +
> >>       return 0;
> >>  }
> > 
> > Thanks.
> > 
> > Is there any reason for hostfs to be playing with the f_spare field at
> > all?
> 
> It just copies it from struct statfs64 on the host to struct kstatfs
> on the guest.
> Probably a memcpy() is more future-safe, if it's combined with a
> BUILD_BUG_ON(sizeof(statfs64.f_spare) != sizeof(kstatfs.f_spare)).
> 
> Still, currently it doesn't copy the recently added f_flags field.
> To protect against future changes like that, an explicit
> BUILD_BUG_ON(sizeof(kstatfs.f_spare) != 4*sizeof(long)) may be even
> better...

Anyway, why do we need to copy f_spare from the host to the guest?
I'm quite sure it can be omitted.

Thanks,
//richard

Re: [uml-devel] [PATCH 1/1] hostfs: fix UML crash

[Geert Uytterhoeven]

On Mon, Oct 18, 2010 at 21:31, Richard Weinberger <richard@nod.at> wrote:
> Am Montag 18 Oktober 2010, 21:22:31 schrieb Geert Uytterhoeven:
>> On Mon, Oct 18, 2010 at 20:40, Andrew Morton <akpm@linux-foundation.org>
> wrote:
>> > On Mon, 18 Oct 2010 18:36:54 +0200 Richard Weinberger <richard@nod.at>
> wrote:
>> >> 365b1818 resized f_spare within struct statfs.
>> >> hostfs accesses f_spare directly and needs an update.
>> >>
>> >> Signed-off-by: Richard Weinberger <richard@nod.at>
>> >> Reported-by: Toralf F__rster <toralf.foerster@gmx.de>
>> >> Tested-by: Toralf F__rster <toralf.foerster@gmx.de>
>> >> ---
>> >>  fs/hostfs/hostfs_user.c |    2 +-
>> >>  1 files changed, 1 insertions(+), 1 deletions(-)
>> >>
>> >> diff --git a/fs/hostfs/hostfs_user.c b/fs/hostfs/hostfs_user.c
>> >> index 6777aa0..ce2f168 100644
>> >> --- a/fs/hostfs/hostfs_user.c
>> >> +++ b/fs/hostfs/hostfs_user.c
>> >> @@ -388,6 +388,6 @@ int do_statfs(char *root, long *bsize_out, long long
>> >> *blocks_out, spare_out[1] = buf.f_spare[1];
>> >>       spare_out[2] = buf.f_spare[2];
>> >>       spare_out[3] = buf.f_spare[3];
>> >> -     spare_out[4] = buf.f_spare[4];
>> >> +
>> >>       return 0;
>> >>  }
>> >
>> > Thanks.
>> >
>> > Is there any reason for hostfs to be playing with the f_spare field at
>> > all?
>>
>> It just copies it from struct statfs64 on the host to struct kstatfs
>> on the guest.
>> Probably a memcpy() is more future-safe, if it's combined with a
>> BUILD_BUG_ON(sizeof(statfs64.f_spare) != sizeof(kstatfs.f_spare)).
>>
>> Still, currently it doesn't copy the recently added f_flags field.
>> To protect against future changes like that, an explicit
>> BUILD_BUG_ON(sizeof(kstatfs.f_spare) != 4*sizeof(long)) may be even
>> better...
>
> Anyway, why do we need to copy f_spare from the host to the guest?
> I'm quite sure it can be omitted.

I guess it wants to preserve fields that are added in the future,
which may be useful
if the host is more recent than the guest.

Still, even if you don't want to preserve them, you probably want to clear them,
so memset() is better than a fixed loop through 4 elements.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [uml-devel] [PATCH 1/1] hostfs: fix UML crash

[Richard Weinberger]

Am Montag 18 Oktober 2010, 21:43:44 schrieb Geert Uytterhoeven:
> On Mon, Oct 18, 2010 at 21:31, Richard Weinberger <richard@nod.at> wrote:
> > Am Montag 18 Oktober 2010, 21:22:31 schrieb Geert Uytterhoeven:
> >> On Mon, Oct 18, 2010 at 20:40, Andrew Morton <akpm@linux-foundation.org>
> > 
> > wrote:
> >> > On Mon, 18 Oct 2010 18:36:54 +0200 Richard Weinberger <richard@nod.at>
> > 
> > wrote:
> >> >> 365b1818 resized f_spare within struct statfs.
> >> >> hostfs accesses f_spare directly and needs an update.
> >> >> 
> >> >> Signed-off-by: Richard Weinberger <richard@nod.at>
> >> >> Reported-by: Toralf F__rster <toralf.foerster@gmx.de>
> >> >> Tested-by: Toralf F__rster <toralf.foerster@gmx.de>
> >> >> ---
> >> >>  fs/hostfs/hostfs_user.c |    2 +-
> >> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >> >> 
> >> >> diff --git a/fs/hostfs/hostfs_user.c b/fs/hostfs/hostfs_user.c
> >> >> index 6777aa0..ce2f168 100644
> >> >> --- a/fs/hostfs/hostfs_user.c
> >> >> +++ b/fs/hostfs/hostfs_user.c
> >> >> @@ -388,6 +388,6 @@ int do_statfs(char *root, long *bsize_out, long
> >> >> long *blocks_out, spare_out[1] = buf.f_spare[1];
> >> >>       spare_out[2] = buf.f_spare[2];
> >> >>       spare_out[3] = buf.f_spare[3];
> >> >> -     spare_out[4] = buf.f_spare[4];
> >> >> +
> >> >>       return 0;
> >> >>  }
> >> > 
> >> > Thanks.
> >> > 
> >> > Is there any reason for hostfs to be playing with the f_spare field at
> >> > all?
> >> 
> >> It just copies it from struct statfs64 on the host to struct kstatfs
> >> on the guest.
> >> Probably a memcpy() is more future-safe, if it's combined with a
> >> BUILD_BUG_ON(sizeof(statfs64.f_spare) != sizeof(kstatfs.f_spare)).
> >> 
> >> Still, currently it doesn't copy the recently added f_flags field.
> >> To protect against future changes like that, an explicit
> >> BUILD_BUG_ON(sizeof(kstatfs.f_spare) != 4*sizeof(long)) may be even
> >> better...

IMHO using kstatfs within ubd_user.c is not very nice because is a internal 
kernel struct.

> > Anyway, why do we need to copy f_spare from the host to the guest?
> > I'm quite sure it can be omitted.
> 
> I guess it wants to preserve fields that are added in the future,
> which may be useful
> if the host is more recent than the guest.

Good point!

Thanks,
//richard