ext2_fill_super breakage

PATCH In review Andrew Morton 10 participants 22 messages Mar 28, 2002 – Mar 29, 2002

Subsystems (3)

Memory management ext2 file system Kernel automounter

lore

Share & tools

lore

Andrew Morton

author

2002-03-28 07:32 UTC #0 lore raw

PATCH 1/1 ext2_fill_super breakage

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.

fs/autofs/inode.c renamed from fs/autofs/inode.c~fill-super +2 -1

@@ -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;

fs/devpts/inode.c renamed from fs/devpts/inode.c~fill-super +2 -1

@@ -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;

fs/ext2/super.c renamed from fs/ext2/super.c~fill-super +2 -2

@@ -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

Brian Gerst

2002-03-28 13:36 UTC #1 lore raw

Quoted 21 lines · 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));

Quoted 3 lines

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.

Signature · 1 line

						Brian Gerst

Jos Hulzink

2002-03-28 13:50 UTC #2 lore raw

Quoted 12 lines · Brian Gerst

Quoted 10 lines · Andrew Morton
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));

Quoted 1 line

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

Bill Davidsen

2002-03-28 17:29 UTC #3 lore raw

In reply to Brian Gerst

Quoted 11 lines · Brian Gerst

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

Quoted 6 lines

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...

Signature · 3 lines

bill davidsen <[email protected]>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Rob Landley

2002-03-28 13:46 UTC #4 lore raw

In reply to Andrew Morton

Quoted 37 lines · Brian Gerst

Quoted 21 lines · 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));
Quoted 3 lines
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

Quoted 6 lines

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

Alexander Viro

2002-03-28 14:21 UTC #5 lore raw

In reply to Andrew Morton

Quoted 13 lines · 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));

Quoted 7 lines

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.

Nikita Danilov

2002-03-28 14:37 UTC #6 lore raw

Quoted 21 lines · Alexander Viro

Quoted 17 lines · 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;

Quoted 8 lines

Quoted 3 lines
	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.

Quoted 2 lines

Nikita.

Alexander Viro

2002-03-28 14:48 UTC #7 lore raw

Quoted 4 lines · Nikita Danilov

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.

Nikita Danilov

2002-03-28 14:52 UTC #8 lore raw

Quoted 4 lines · Alexander Viro

On Thu, 28 Mar 2002, Nikita Danilov wrote:

Quoted 8 lines

Quoted 4 lines
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.

Quoted 2 lines

Nikita.

Alexander Viro

2002-03-28 15:20 UTC #9 lore raw

Quoted 6 lines · Nikita Danilov

Quoted 3 lines
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...

Arjan van de Ven

2002-03-28 14:50 UTC #10 lore raw

In reply to Andrew Morton

Quoted 5 lines

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 ?

Nikita Danilov

2002-03-28 15:02 UTC #11 lore raw

Quoted 9 lines · Arjan van de Ven

Quoted 5 lines
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.

Quoted 1 line

Nikita.

Andrew Morton

author

2002-03-28 17:30 UTC #12 lore raw

In reply to Andrew Morton

Quoted 23 lines · Brian Gerst

Quoted 21 lines · 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));

Quoted 8 lines

Quoted 3 lines
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.

Quoted 6 lines

yields:

00000b70 <foo1>:

      b70:       b8 dc 00 00 00          mov    $0xdc,%eax
      b75:       c3                      ret

Quoted 6 lines

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.

Andrew Morton

author

2002-03-28 17:47 UTC #13 lore raw

In reply to Andrew Morton

Quoted 18 lines · Alexander Viro

Quoted 15 lines · 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

Quoted 7 lines

Quoted 5 lines
      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 :)

Quoted 1 line

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.

Quoted 3 lines

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:

mm/slab.c +3 -3

@@ -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
 
 /*

Alexander Viro

2002-03-28 23:52 UTC #14 lore raw

Quoted 4 lines · Andrew Morton

Quoted 1 line
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.

Quoted 3 lines

(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...

Andrew Morton

author

2002-03-29 00:28 UTC #15 lore raw

In reply to Andrew Morton

Quoted 10 lines · Alexander Viro

Quoted 4 lines · Andrew Morton
Quoted 1 line
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?

Quoted 7 lines

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)

Quoted 5 lines

...
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.

Bill Davidsen

2002-03-29 15:48 UTC #16 lore raw

Quoted 6 lines · Andrew Morton

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.

Signature · 3 lines

bill davidsen <[email protected]>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Bill Davidsen

2002-03-29 00:45 UTC #17 lore raw

In reply to Andrew Morton

Quoted 7 lines · Andrew Morton

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.

Signature · 3 lines

bill davidsen <[email protected]>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Andreas Dilger

2002-03-29 05:16 UTC #18 lore raw

In reply to Andrew Morton

Quoted 6 lines · Andrew Morton

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

Guest section DW

2002-03-29 08:07 UTC #19 lore raw

In reply to Andrew Morton

Quoted 1 line · Andrew Morton

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

Brian Gerst

2002-03-28 18:14 UTC #20 lore raw

In reply to Andrew Morton

Quoted 31 lines · Andrew Morton

Quoted 29 lines · Brian Gerst
Quoted 21 lines · 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));
Quoted 3 lines
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):

Quoted 8 lines

Quoted 2 lines
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

2002-03-28 22:47 UTC #21 lore raw

In reply to Andrew Morton

Quoted 4 lines · 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.

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

Signature · 1 line

						Brian Gerst

fs/bfs/inode.c +2 -2

diff -urN linux-2.5.7-dj2/fs/bfs/inode.c linux/fs/bfs/inode.c
@@ -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);