[patch] ext2_fill_super breakage

[patch] ext2_fill_super breakage

[Andrew Morton]

In 2.5.7 there is a thinko in the allocation and initialisation
of the fs-private superblock for ext2.  It's passing the wrong type
to the sizeof operator (which of course gives the wrong size)
when allocating and clearing the memory. 

Lesson for the day: this is one of the reasons why this idiom:

	some_type *p;

	p = malloc(sizeof(*p));
	...
	memset(p, 0, sizeof(*p));

is preferable to

	some_type *p;

	p = malloc(sizeof(some_type));
	...
	memset(p, 0, sizeof(some_type));

I checked the other filesystems.  They're OK (but idiomatically
impure).  I've added a couple of defensive memsets where
they were missing.


--- 2.5.7/fs/autofs/inode.c~fill-super	Wed Mar 27 23:14:20 2002
+++ 2.5.7-akpm/fs/autofs/inode.c	Wed Mar 27 23:14:54 2002
@@ -119,9 +119,10 @@ int autofs_fill_super(struct super_block
 	struct autofs_sb_info *sbi;
 	int minproto, maxproto;
 
-	sbi = (struct autofs_sb_info *) kmalloc(sizeof(struct autofs_sb_info), GFP_KERNEL);
+	sbi = kmalloc(sizeof(*sbi), GFP_KERNEL);
 	if ( !sbi )
 		goto fail_unlock;
+	memset(sbi, 0, sizeof(*sbi));
 	DPRINTK(("autofs: starting up, sbi = %p\n",sbi));
 
 	s->u.generic_sbp = sbi;
--- 2.5.7/fs/devpts/inode.c~fill-super	Wed Mar 27 23:16:05 2002
+++ 2.5.7-akpm/fs/devpts/inode.c	Wed Mar 27 23:16:33 2002
@@ -123,9 +123,10 @@ static int devpts_fill_super(struct supe
 	struct inode * inode;
 	struct devpts_sb_info *sbi;
 
-	sbi = (struct devpts_sb_info *) kmalloc(sizeof(struct devpts_sb_info), GFP_KERNEL);
+	sbi = kmalloc(sizeof(*sbi), GFP_KERNEL);
 	if ( !sbi )
 		goto fail;
+	memset(sbi, 0, sizeof(*sbi));
 
 	sbi->magic  = DEVPTS_SBI_MAGIC;
 	sbi->max_ptys = unix98_max_ptys;
--- 2.5.7/fs/ext2/super.c~fill-super	Wed Mar 27 23:16:57 2002
+++ 2.5.7-akpm/fs/ext2/super.c	Wed Mar 27 23:17:25 2002
@@ -465,11 +465,11 @@ static int ext2_fill_super(struct super_
 	int db_count;
 	int i, j;
 
-	sbi = kmalloc(sizeof(struct ext2_super_block), GFP_KERNEL);
+	sbi = kmalloc(sizeof(*sbi), GFP_KERNEL);
 	if (!sbi)
 		return -ENOMEM;
 	sb->u.generic_sbp = sbi;
-	memset(sbi, 0, sizeof(struct ext2_super_block));
+	memset(sbi, 0, sizeof(*sbi));
 
 	/*
 	 * See what the current blocksize for the device is, and


-

Re: [patch] ext2_fill_super breakage

[Brian Gerst]

Andrew Morton wrote:
> In 2.5.7 there is a thinko in the allocation and initialisation
> of the fs-private superblock for ext2.  It's passing the wrong type
> to the sizeof operator (which of course gives the wrong size)
> when allocating and clearing the memory. 
> 
> Lesson for the day: this is one of the reasons why this idiom:
> 
> 	some_type *p;
> 
> 	p = malloc(sizeof(*p));
> 	...
> 	memset(p, 0, sizeof(*p));
> 
> is preferable to
> 
> 	some_type *p;
> 
> 	p = malloc(sizeof(some_type));
> 	...
> 	memset(p, 0, sizeof(some_type));
> 
> I checked the other filesystems.  They're OK (but idiomatically
> impure).  I've added a couple of defensive memsets where
> they were missing.

I'm not sure I follow you here.  Compiling this code (gcc 2.96):

int foo1(void) { return sizeof(struct ext2_sb_info); }
int foo2(struct ext2_sb_info *sbi) { return sizeof(*sbi); }

yields:

00000b70 <foo1>:
      b70:       b8 dc 00 00 00          mov    $0xdc,%eax
      b75:       c3                      ret

00000b80 <foo2>:
      b80:       b8 dc 00 00 00          mov    $0xdc,%eax
      b85:       c3                      ret

The sizes are the same, so unless it makes a difference with another 
version of gcc then this is just a cosmetic change.

-- 

						Brian Gerst

Re: [patch] ext2_fill_super breakage

[Rob Landley]

On Thursday 28 March 2002 08:34 am, Brian Gerst wrote:
> Andrew Morton wrote:
> > In 2.5.7 there is a thinko in the allocation and initialisation
> > of the fs-private superblock for ext2.  It's passing the wrong type
> > to the sizeof operator (which of course gives the wrong size)
> > when allocating and clearing the memory.
> >
> > Lesson for the day: this is one of the reasons why this idiom:
> >
> > 	some_type *p;
> >
> > 	p = malloc(sizeof(*p));
> > 	...
> > 	memset(p, 0, sizeof(*p));
> >
> > is preferable to
> >
> > 	some_type *p;
> >
> > 	p = malloc(sizeof(some_type));
> > 	...
> > 	memset(p, 0, sizeof(some_type));
> >
> > I checked the other filesystems.  They're OK (but idiomatically
> > impure).  I've added a couple of defensive memsets where
> > they were missing.
>
> I'm not sure I follow you here.  Compiling this code (gcc 2.96):
>
> int foo1(void) { return sizeof(struct ext2_sb_info); }
> int foo2(struct ext2_sb_info *sbi) { return sizeof(*sbi); }
>
> yields:
>
> 00000b70 <foo1>:
>       b70:       b8 dc 00 00 00          mov    $0xdc,%eax
>       b75:       c3                      ret
>
> 00000b80 <foo2>:
>       b80:       b8 dc 00 00 00          mov    $0xdc,%eax
>       b85:       c3                      ret
>
> The sizes are the same, so unless it makes a difference with another
> version of gcc then this is just a cosmetic change.

If you take the sizeof based on the type of the actual variable is using, 
you're guaranteed to get the right result.  (The type information is only in 
one place, so what the compiler's using and what you're reserving space for 
have to be the same thing.)

If you sizeof( ) the type and declare the type of the variable seperately, 
you have the possibility of version skew between them.  The sizeof( ) and the 
variable declaration itself may not match.  Hence bugs.

Rob

Re: [patch] ext2_fill_super breakage

[Jos Hulzink]

On Thu, 28 Mar 2002, Brian Gerst wrote:

> Andrew Morton wrote:
> > Lesson for the day: this is one of the reasons why this idiom:
> > 	some_type *p;
> > 	p = malloc(sizeof(*p));
> > 	...
> > 	memset(p, 0, sizeof(*p));
> > is preferable to
> > 	some_type *p;
> > 	p = malloc(sizeof(some_type));
> > 	...
> > 	memset(p, 0, sizeof(some_type));

> I'm not sure I follow you here.  Compiling this code (gcc 2.96):
....

It is not about what comes out of the compiler, it is about the fact that
in the second case, when some_type becomes another_type, you have to
replace some_type at 3 locations, easy to forget one. In the first case,
your compiler checks what the size of your variable is, the memset will
never fill beyond the end of your allocated memory.

It is about preventing patching again and again for someone forgot to use
:s/something/anotherthing/g. Besides, the code looks much better imho.

Jos

Re: [patch] ext2_fill_super breakage

[Bill Davidsen]

On Thu, 28 Mar 2002, Brian Gerst wrote:

> I'm not sure I follow you here.  Compiling this code (gcc 2.96):
> 
> int foo1(void) { return sizeof(struct ext2_sb_info); }
> int foo2(struct ext2_sb_info *sbi) { return sizeof(*sbi); }
> 
> yields:
> 
> 00000b70 <foo1>:
>       b70:       b8 dc 00 00 00          mov    $0xdc,%eax
>       b75:       c3                      ret
> 
> 00000b80 <foo2>:
>       b80:       b8 dc 00 00 00          mov    $0xdc,%eax
>       b85:       c3                      ret
> 
> The sizes are the same, so unless it makes a difference with another 
> version of gcc then this is just a cosmetic change.

  Not at all, it's good programming practice. If you use the sizeof(*p) 
notation then the code work right, first time, every time, *even if you
change the type of the pointer*. Without that you have to search all the
code following the pointer declaration for a use of the type where the
pointer should have been dereferenced. 

  Now scale that to the case where you make a similar change in a macro or
typedef. Now you have to search the whole kernel (or NIC drivers, or ...).
Doing it right assures a change in one place will not break things at
random places all of the source tree.

  That used to be part of Intro To Programming Computers...

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [patch] ext2_fill_super breakage

[Andrew Morton]

Brian Gerst wrote:
> 
> Andrew Morton wrote:
> > In 2.5.7 there is a thinko in the allocation and initialisation
> > of the fs-private superblock for ext2.  It's passing the wrong type
> > to the sizeof operator (which of course gives the wrong size)
> > when allocating and clearing the memory.
> >
> > Lesson for the day: this is one of the reasons why this idiom:
> >
> >       some_type *p;
> >
> >       p = malloc(sizeof(*p));
> >       ...
> >       memset(p, 0, sizeof(*p));
> >
> > is preferable to
> >
> >       some_type *p;
> >
> >       p = malloc(sizeof(some_type));
> >       ...
> >       memset(p, 0, sizeof(some_type));
> >
> > I checked the other filesystems.  They're OK (but idiomatically
> > impure).  I've added a couple of defensive memsets where
> > they were missing.
> 
> I'm not sure I follow you here.  Compiling this code (gcc 2.96):
> 
> int foo1(void) { return sizeof(struct ext2_sb_info); }
> int foo2(struct ext2_sb_info *sbi) { return sizeof(*sbi); }

The current code is using sizeof(ext2_super_block) for
something which is of type ext2_sb_info.

> yields:
> 
> 00000b70 <foo1>:
>       b70:       b8 dc 00 00 00          mov    $0xdc,%eax
>       b75:       c3                      ret
> 
> 00000b80 <foo2>:
>       b80:       b8 dc 00 00 00          mov    $0xdc,%eax
>       b85:       c3                      ret
> 
> The sizes are the same, so unless it makes a difference with another
> version of gcc then this is just a cosmetic change.

The stylistic change tends to provide insulation from the
above bug.

-

Re: [patch] ext2_fill_super breakage

[Brian Gerst]

Andrew Morton wrote:
> 
> Brian Gerst wrote:
> >
> > Andrew Morton wrote:
> > > In 2.5.7 there is a thinko in the allocation and initialisation
> > > of the fs-private superblock for ext2.  It's passing the wrong type
> > > to the sizeof operator (which of course gives the wrong size)
> > > when allocating and clearing the memory.
> > >
> > > Lesson for the day: this is one of the reasons why this idiom:
> > >
> > >       some_type *p;
> > >
> > >       p = malloc(sizeof(*p));
> > >       ...
> > >       memset(p, 0, sizeof(*p));
> > >
> > > is preferable to
> > >
> > >       some_type *p;
> > >
> > >       p = malloc(sizeof(some_type));
> > >       ...
> > >       memset(p, 0, sizeof(some_type));
> > >
> > > I checked the other filesystems.  They're OK (but idiomatically
> > > impure).  I've added a couple of defensive memsets where
> > > they were missing.
> >
> > I'm not sure I follow you here.  Compiling this code (gcc 2.96):
> >
> > int foo1(void) { return sizeof(struct ext2_sb_info); }
> > int foo2(struct ext2_sb_info *sbi) { return sizeof(*sbi); }
> 
> The current code is using sizeof(ext2_super_block) for
> something which is of type ext2_sb_info.
> 
> The stylistic change tends to provide insulation from the
> above bug.

Doh.  Point taken.

--

				Brian Gerst

Re: [patch] ext2_fill_super breakage

[Alexander Viro]

On Wed, 27 Mar 2002, Andrew Morton wrote:

> In 2.5.7 there is a thinko in the allocation and initialisation
> of the fs-private superblock for ext2.  It's passing the wrong type
> to the sizeof operator (which of course gives the wrong size)
> when allocating and clearing the memory. 
 
> Lesson for the day: this is one of the reasons why this idiom:
> 
> 	some_type *p;
> 
> 	p = malloc(sizeof(*p));
> 	...
> 	memset(p, 0, sizeof(*p));
> 
> is preferable to
> 
> 	some_type *p;
> 
> 	p = malloc(sizeof(some_type));
> 	...
> 	memset(p, 0, sizeof(some_type));

... however, there is a lot of reasons why the former is preferable.
For one thing, the latter is hell on any search.  Moreover, I would
argue that memset() on a structure is not a good idea - better do
the explicit initialization.

Re: [patch] ext2_fill_super breakage

[Nikita Danilov]

Alexander Viro writes:
 > 
 > 
 > On Wed, 27 Mar 2002, Andrew Morton wrote:
 > 
 > > In 2.5.7 there is a thinko in the allocation and initialisation
 > > of the fs-private superblock for ext2.  It's passing the wrong type
 > > to the sizeof operator (which of course gives the wrong size)
 > > when allocating and clearing the memory. 
 >  
 > > Lesson for the day: this is one of the reasons why this idiom:
 > > 
 > > 	some_type *p;
 > > 
 > > 	p = malloc(sizeof(*p));
 > > 	...
 > > 	memset(p, 0, sizeof(*p));
 > > 
 > > is preferable to
 > > 
 > > 	some_type *p;
 > > 
 > > 	p = malloc(sizeof(some_type));
 > > 	...
 > > 	memset(p, 0, sizeof(some_type));
 > 
 > ... however, there is a lot of reasons why the former is preferable.
 > For one thing, the latter is hell on any search.  Moreover, I would
 > argue that memset() on a structure is not a good idea - better do
 > the explicit initialization.

Explicit initialization always leaves room for some "pad" field inserted
by compiler for alignment to be left with garbage. This is more than
just annoyance when structure is something that will be written to the
disk. Reiserfs had such problems.

 > 
 > 

Nikita.

Re: [patch] ext2_fill_super breakage

[Alexander Viro]

On Thu, 28 Mar 2002, Nikita Danilov wrote:

> Explicit initialization always leaves room for some "pad" field inserted
> by compiler for alignment to be left with garbage. This is more than
> just annoyance when structure is something that will be written to the
> disk. Reiserfs had such problems.

If your structure will be written on disk you'd better have full control
over alignment - otherwise you are risking incompatibilities between
platforms and compiler versions.

Re: [patch] ext2_fill_super breakage

[Nikita Danilov]

Alexander Viro writes:
 > 
 > 
 > On Thu, 28 Mar 2002, Nikita Danilov wrote:
 > 
 > > Explicit initialization always leaves room for some "pad" field inserted
 > > by compiler for alignment to be left with garbage. This is more than
 > > just annoyance when structure is something that will be written to the
 > > disk. Reiserfs had such problems.
 > 
 > If your structure will be written on disk you'd better have full control
 > over alignment - otherwise you are risking incompatibilities between
 > platforms and compiler versions.

Yes, but such experience frequently is only gained after format is
already carved in stone^Wdisk.

 > 
 > 

Nikita.

Re: [patch] ext2_fill_super breakage

[Alexander Viro]

On Thu, 28 Mar 2002, Nikita Danilov wrote:

>  > If your structure will be written on disk you'd better have full control
>  > over alignment - otherwise you are risking incompatibilities between
>  > platforms and compiler versions.
> 
> Yes, but such experience frequently is only gained after format is
> already carved in stone^Wdisk.

Or after reading through a FAQ or two.  Not to mention any half-decent
textbook on C from mid-80s or later...

Re: [patch] ext2_fill_super breakage

[Arjan van de Ven]

> Explicit initialization always leaves room for some "pad" field inserted
> by compiler for alignment to be left with garbage. This is more than
> just annoyance when structure is something that will be written to the
> disk. Reiserfs had such problems.

such compiler-based padding is architecture specific.. I'd hope the
reiserfs
on disk format isn't architecture specific ?

Re: [patch] ext2_fill_super breakage

[Nikita Danilov]

Arjan van de Ven writes:
 > 
 > > Explicit initialization always leaves room for some "pad" field inserted
 > > by compiler for alignment to be left with garbage. This is more than
 > > just annoyance when structure is something that will be written to the
 > > disk. Reiserfs had such problems.
 > 
 > such compiler-based padding is architecture specific.. I'd hope the
 > reiserfs
 > on disk format isn't architecture specific ?

It is not.

 > 

Nikita.

Re: [patch] ext2_fill_super breakage

[Andrew Morton]

Alexander Viro wrote:
> 
> On Wed, 27 Mar 2002, Andrew Morton wrote:
> 
> > In 2.5.7 there is a thinko in the allocation and initialisation
> > of the fs-private superblock for ext2.  It's passing the wrong type
> > to the sizeof operator (which of course gives the wrong size)
> > when allocating and clearing the memory.
> 
> > Lesson for the day: this is one of the reasons why this idiom:
> >
> >       some_type *p;
> >
> >       p = malloc(sizeof(*p));
> >       ...
> >       memset(p, 0, sizeof(*p));
> >
> > is preferable to
> >
> >       some_type *p;
> >
> >       p = malloc(sizeof(some_type));
> >       ...
> >       memset(p, 0, sizeof(some_type));
> 
> ... however, there is a lot of reasons why the former is preferable.

Yeah, a lot of newbies think that :)

> For one thing, the latter is hell on any search.

If the usage of the type is hard to search for then
then wrong identifier was chosen.

>  Moreover, I would
> argue that memset() on a structure is not a good idea - better do
> the explicit initialization.

memset will run at up to twice the speed (according to
Arjan).  Dunno if this includes I-cache misses - probably
not.

I'm not particularly fussed about this one, but I do prefer
the sleep-at-night safety of a blanket memset.  Because
(and I think this is something on which you and I somewhat
differ) code should be written for the convenience of others,
not the original author.  A nice memset will leave no doubt
in the reader's mind that all members of the structure have
been initialised.

BTW, Linus: while we're on the topic, I think we should do
this again:

--- linux-2.5.7/mm/slab.c	Sat Mar  9 00:18:43 2002
+++ 25/mm/slab.c	Thu Mar 28 09:42:41 2002
@@ -95,9 +95,9 @@
 #define	STATS		1
 #define	FORCED_DEBUG	1
 #else
-#define	DEBUG		0
-#define	STATS		0
-#define	FORCED_DEBUG	0
+#define	DEBUG		1	/* It's a development kernel */
+#define	STATS		1
+#define	FORCED_DEBUG	1
 #endif
 
 /*

-

Re: [patch] ext2_fill_super breakage

[Alexander Viro]

On Thu, 28 Mar 2002, Andrew Morton wrote:

> > For one thing, the latter is hell on any search.
> 
> If the usage of the type is hard to search for then
> then wrong identifier was chosen.

Huh?

Search for ext2_sb_info will give you all places that refer to it.
Including a buttload of
	struct ext2_sb_info *p;

Now, search for ext2_sb_info[ 	]*[^ 	*] is much more interesting.
With explicit sizeof it is guaranteed to give you all places where
such beast is allocated or subjected to memset, etc.

In this case it's not too interesting.  But try it for struct super_block
or struct dentry or struct buffer_head, etc. - anything that is widely
used.
 
> (and I think this is something on which you and I somewhat
> differ) code should be written for the convenience of others,
> not the original author.

Ahem.  Right now I'm digging through the <expletives> known as lvm.c
trying to fix the use of kdev_t in ioctls.  Believe me, I'm _not_
the original author.  "Where the heck do they initialize <field>" is
the question I've to deal with all the time and I'd rather have it
(along with "where the heck do they allocate and free their monsters")
as greppable as possible.

BTW, I _really_ wonder who had audited lvm.c for inclusion - quite a
few places in there pull such lovely stunts as, say it, use of strcmp()
on a user-supplied array of characters.  Whaddya mean, "what if there's
no NUL"?  Sigh...

Re: [patch] ext2_fill_super breakage

[Andrew Morton]

Alexander Viro wrote:
> 
> On Thu, 28 Mar 2002, Andrew Morton wrote:
> 
> > > For one thing, the latter is hell on any search.
> >
> > If the usage of the type is hard to search for then
> > then wrong identifier was chosen.
> 
> Huh?
> 
> Search for ext2_sb_info will give you all places that refer to it.
> Including a buttload of
>         struct ext2_sb_info *p;
> 
> Now, search for ext2_sb_info[   ]*[^    *] is much more interesting.
> With explicit sizeof it is guaranteed to give you all places where
> such beast is allocated or subjected to memset, etc.

Where "etc" is "typecast".  I can't think of much else
which would be found.

So the search for ext2_sb_info found the function, and
then you're down to perfoming a forward search for "p".
Which, of course, doesn't work because "p" is an asinine
identifier.  Which was my point.  (Insert monthly whine
about ext2_new_block)

> ...
> BTW, I _really_ wonder who had audited lvm.c for inclusion - quite a
> few places in there pull such lovely stunts as, say it, use of strcmp()
> on a user-supplied array of characters.  Whaddya mean, "what if there's
> no NUL"?  Sigh...

We do not appear to have an "audit for inclusion" process.
I wish we did.  If a tree owner threw a patch at me and
asked for comments I'd gladly help out that way.  Jeff
did some absolutely brilliant work on the e100/e1000
drivers behind the scenes - it'd be nice to have more of that.

BTW, ext3 keeps a kdev_t on-disk for external journals.  The
external journal support is experimental, added to allow people
to evaluate the usefulness of external journalling.  If we
decide to retain the capability we'll be moving it to a UUID
or mount-based scheme.  So if the kdev_t is being a problem,
I think we can just break it.

-

Re: [patch] ext2_fill_super breakage

[Andreas Dilger]

On Mar 28, 2002  16:25 -0800, Andrew Morton wrote:
> BTW, ext3 keeps a kdev_t on-disk for external journals.  The
> external journal support is experimental, added to allow people
> to evaluate the usefulness of external journalling.  If we
> decide to retain the capability we'll be moving it to a UUID
> or mount-based scheme.  So if the kdev_t is being a problem,
> I think we can just break it.

Actually, I believe it would be keeping a "dev_t" on the disk, so
if we are using it from within the kernel we should be using
"to_kdevt()" or whatever the function is called.

That reminds me - I _do_ have the mount-based stuff for journal
devices set up.  At fs mount time, the ext3 code calls a helper
function which will locate the journal by its UUID.  The only
real reason for the s_journal_dev might be for e2fsck to locate
the journal device more easily, but it is also pretty easy to
find the journal by UUID in user space.

I'm just woefully behind on getting patches out of my tree.  At least
with my OLS paper I'm "forced" to finish off the ext3 online resizing
code within the next 3 months or so.

Cheers, Andreas
--
Andreas Dilger  \ "If a man ate a pound of pasta and a pound of antipasto,
                 \  would they cancel out, leaving him still hungry?"
http://www-mddsp.enel.ucalgary.ca/People/adilger/               -- Dogbert

Re: [patch] ext2_fill_super breakage

[Guest section DW]

On Thu, Mar 28, 2002 at 04:25:51PM -0800, Andrew Morton wrote:

> BTW, ext3 keeps a kdev_t on-disk for external journals.

A kdev_t is conceptually a struct block_device *.
Points to allocated kernel space.
Writing such an animal to disk is completely pointless.
You want to apply some conversion function first,
even when that conversion function may be the identity
in the setup where a kdev_t has integral type. And make
sure that the resulting integer can hold at least 64 bits.

Andries

Re: [patch] ext2_fill_super breakage

[Bill Davidsen]

On Thu, 28 Mar 2002, Andrew Morton wrote:

> BTW, ext3 keeps a kdev_t on-disk for external journals.  The
> external journal support is experimental, added to allow people
> to evaluate the usefulness of external journalling.  If we
> decide to retain the capability we'll be moving it to a UUID
> or mount-based scheme.  So if the kdev_t is being a problem,
> I think we can just break it.

  If experience on JFS is any predictor, the external journal will be
quite useful as a performance issue. It can be put on a faster device to
avoid bottlenecks. With JFS I finally wound up with the journal on a
battery-backed SCSI solid state disk, and got about 30% faster completion
of my daily audit run which deleted ~1000000 (yes one million) files as
fast as it could when done.

  I was also creating the same number of files over the course of a day,
which is still a respectable directory change rate!

  I predict that applications which create/delete a lot of files will run
better with tuning this feature.

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [patch] ext2_fill_super breakage

[Bill Davidsen]

On Thu, 28 Mar 2002, Andrew Morton wrote:

> I'm not particularly fussed about this one, but I do prefer
> the sleep-at-night safety of a blanket memset.  Because
> (and I think this is something on which you and I somewhat
> differ) code should be written for the convenience of others,
> not the original author.  A nice memset will leave no doubt
> in the reader's mind that all members of the structure have
> been initialised.

  Definitely. It's easy to forget to initialize something when you reuse a
struct.

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [patch] ext2_fill_super breakage

[Brian Gerst]

[-- Attachment #1: Type: text/plain, Size: 341 bytes --]

Andrew Morton wrote:
> In 2.5.7 there is a thinko in the allocation and initialisation
> of the fs-private superblock for ext2.  It's passing the wrong type
> to the sizeof operator (which of course gives the wrong size)
> when allocating and clearing the memory. 

Same bug with bfs patch (only in -dj tree so far).

-- 

						Brian Gerst

[-- Attachment #2: sb-bfs-2 --]
[-- Type: text/plain, Size: 533 bytes --]

diff -urN linux-2.5.7-dj2/fs/bfs/inode.c linux/fs/bfs/inode.c
--- linux-2.5.7-dj2/fs/bfs/inode.c	Thu Mar 28 16:34:37 2002
+++ linux/fs/bfs/inode.c	Thu Mar 28 16:35:43 2002
@@ -292,11 +292,11 @@
 	int i, imap_len;
 	struct bfs_sb_info * info;
 
-	info = kmalloc(sizeof(struct bfs_super_block), GFP_KERNEL);
+	info = kmalloc(sizeof(*info), GFP_KERNEL);
 	if (!info)
 		return -ENOMEM;
 	s->u.generic_sbp = info;
-	memset(info, 0, sizeof(struct bfs_super_block));
+	memset(info, 0, sizeof(*info));
 
 	sb_set_blocksize(s, BFS_BSIZE);