2.6.0-test6 crash while reading files in /proc/fs/reiserfs/sda1

DISCUSSION Zan Lynx 6 participants 15 messages Sep 30, 2003 – Oct 2, 2003 VFS

lore

Share & tools

lore

Zan Lynx

author

2003-09-30 15:44 UTC #0 lore raw

I was interested in the contents of the files in /proc/fs/reiserfs/sda1, so I did these commands:

cd /proc/fs/reiserfs/sda1
grep . *

(I like using the grep . * because it labels the contents of each file with the filename.)

I did this as a regular user and also as root. Both times the system crashed and immediately rebooted. I tried it again as root and the system froze instead.

The system is basically RedHat 9. The kernel was compiled with GCC 3.2.2. I attached a compressed lsmod and kernel configuration to this message.

The CPU is an Athlon XP 2000+, the SCSI adapter is a LSI Logic 53c1010 Ultra3 64 bit adapter running on a 32 bit bus. (lspci output is also attached.) The SCSI drive is a Seagate X15.3.

Thanks for looking at this.

Signature · 1 line

Zan Lynx <[email protected]>

Jonathan Briggs

2003-09-30 15:51 UTC #1 lore raw

Quoted 17 lines · Zan Lynx

I was interested in the contents of the files in /proc/fs/reiserfs/sda1, so I did these commands:
cd /proc/fs/reiserfs/sda1
grep . *
(I like using the grep . * because it labels the contents of each file with the filename.)
I did this as a regular user and also as root. Both times the system crashed and immediately rebooted. I tried it again as root and the system froze instead.
The system is basically RedHat 9. The kernel was compiled with GCC 3.2.2. I attached a compressed lsmod and kernel configuration to this message.

Quoted 5 lines

The CPU is an Athlon XP 2000+, the SCSI adapter is a LSI Logic 53c1010 Ultra3 64 bit adapter running on a 32 bit bus. (lspci output is also attached.) The SCSI drive is a Seagate X15.3.
Thanks for looking at this.

Sorry, I forgot the attachment. Here it is again.

Signature · 2 lines

Jonathan Briggs
[email protected]

Vitaly Fertman

2003-09-30 16:06 UTC #2 lore raw

In reply to Zan Lynx

Quoted 6 lines · Zan Lynx

I was interested in the contents of the files in /proc/fs/reiserfs/sda1, so I did these commands:
cd /proc/fs/reiserfs/sda1
grep . *

Quoted 6 lines

(I like using the grep . * because it labels the contents of each file with the filename.)
I did this as a regular user and also as root. Both times the system crashed and immediately rebooted. I tried it again as root and the system froze instead.

which kernel do you use? some patches? could you look into syslog and send us all relevant information.

would you also run cat on all files there separately to detect the fault one.

Quoted 3 lines

The system is basically RedHat 9. The kernel was compiled with GCC 3.2.2. I attached a compressed lsmod and kernel configuration to this message.

no you do not.

Quoted 5 lines

The CPU is an Athlon XP 2000+, the SCSI adapter is a LSI Logic 53c1010 Ultra3 64 bit adapter running on a 32 bit bus. (lspci output is also attached.) The SCSI drive is a Seagate X15.3.
Thanks for looking at this.

Signature · 2 lines

Thanks,
Vitaly Fertman

Hans Reiser

2003-09-30 19:18 UTC #3 lore raw

Quoted 27 lines · Vitaly Fertman

Hi
Quoted 6 lines · Zan Lynx
I was interested in the contents of the files in /proc/fs/reiserfs/sda1, so I did these commands:
cd /proc/fs/reiserfs/sda1
grep . *
Quoted 8 lines
(I like using the grep . * because it labels the contents of each file with the filename.)
I did this as a regular user and also as root. Both times the system crashed and immediately rebooted. I tried it again as root and the system froze instead.
which kernel do you use? some patches? could you look into syslog and send us all relevant information.
would you also run cat on all files there separately to detect the fault one.

Quoted 20 lines

Quoted 5 lines
The system is basically RedHat 9. The kernel was compiled with GCC 3.2.2. I attached a compressed lsmod and kernel configuration to this message.
no you do not.
Quoted 7 lines
The CPU is an Athlon XP 2000+, the SCSI adapter is a LSI Logic 53c1010 Ultra3 64 bit adapter running on a 32 bit bus. (lspci output is also attached.) The SCSI drive is a Seagate X15.3.
Thanks for looking at this.

nikita, if vitaly doesn't solve it before you get in, it is yours to solve.

Vitaly, what configuration did you fail to replicate it using (I hope you attempted it on your machine before asking the user all this)?

Signature · 1 line

Hans

Vitaly Fertman

2003-10-01 10:11 UTC #4 lore raw

Quoted 4 lines

nikita, if vitaly doesn't solve it before you get in, it is yours to solve.
Vitaly, what configuration did you fail to replicate it using (I hope you attempted it on your machine before asking the user all this)?

yes I did, 2.4.22 works fine for me.

Signature · 2 lines

Thanks,
Vitaly Fertman

Zan Lynx

author

2003-10-01 14:44 UTC #5 lore raw

In reply to Vitaly Fertman

Quoted 12 lines · Vitaly Fertman

Hi
Quoted 9 lines · Zan Lynx
I was interested in the contents of the files in /proc/fs/reiserfs/sda1, so I did these commands:
cd /proc/fs/reiserfs/sda1
grep . *
(I like using the grep . * because it labels the contents of each file with the filename.)

Quoted 9 lines

Quoted 3 lines
I did this as a regular user and also as root. Both times the system crashed and immediately rebooted. I tried it again as root and the system froze instead.
which kernel do you use? some patches? could you look into syslog and send us all relevant information.
would you also run cat on all files there separately to detect the fault one.

The kernel is 2.6.0-test6 from kernel.org. No other patches.

Okay, I did cat file > /dev/null on each one. It looks like the problem is with oidmap. The other files do not crash.

Signature · 1 line

Zan Lynx <[email protected]>

Vitaly Fertman

2003-10-01 17:15 UTC #6 lore raw

Quoted 4 lines

The kernel is 2.6.0-test6 from kernel.org. No other patches.
Okay, I did cat file > /dev/null on each one. It looks like the problem is with oidmap. The other files do not crash.

we found a bug in the kernel 2.6.0-test6 and working on the fix.

Signature · 2 lines

Thanks,
Vitaly Fertman

Nikita Danilov

2003-10-01 17:54 UTC #7 lore raw

In reply to Zan Lynx

Quoted 20 lines · Zan Lynx

Quoted 19 lines · Vitaly Fertman
Hi
Quoted 6 lines · Zan Lynx
I was interested in the contents of the files in /proc/fs/reiserfs/sda1, so I did these commands:
cd /proc/fs/reiserfs/sda1
grep . *
Quoted 6 lines
(I like using the grep . * because it labels the contents of each file with the filename.)
I did this as a regular user and also as root. Both times the system crashed and immediately rebooted. I tried it again as root and the system froze instead.
which kernel do you use? some patches? could you look into syslog and send us all relevant information.

Quoted 7 lines

Quoted 2 lines
would you also run cat on all files there separately to detect the fault one.
The kernel is 2.6.0-test6 from kernel.org. No other patches.
Okay, I did cat file > /dev/null on each one. It looks like the problem is with oidmap. The other files do not crash.

Below is a patch, please test.

Seems that while seq_file-ing fs/reiserfs/procfs.c, Alexander got lost in a maze of little pointers and iterators all alike.

Cannot help but describe a little detail: r_stop() erroneously thought that de->data contains a pointer to the super block, while in reality address of some fs/reiserfs/procfs.c:show_* function was stored there. As a result, deactivate_super() danced fine fandango on core, in particular, in the case of show_oidmap() it modified first assignment within loop to reset loop counter back to zero.

Nikita.
----------------------------------------------------------------------

procfs.c renamed from fs/reiserfs/procfs.c +14 ~~-12~~

@@ -453,24 +453,25 @@ static int set_sb(struct super_block *sb
 	return -ENOENT;
 }
 
+extern struct file_system_type reiserfs_fs_type;
+
 static void *r_start(struct seq_file *m, loff_t *pos)
 {
 	struct proc_dir_entry *de = m->private;
 	struct super_block *s = de->parent->data;
-	loff_t l = *pos;
+	void *ret;
 
-	if (l)
-		return NULL;
-
-	if (IS_ERR(sget(&reiserfs_fs_type, test_sb, set_sb, s)))
-		return NULL;
+	ret = sget(&reiserfs_fs_type, test_sb, set_sb, s);
+	if (IS_ERR(ret))
+		return ret;
 
 	up_write(&s->s_umount);
 
-	if (de->deleted) {
-		deactivate_super(s);
+	if (*pos)
+		return NULL;
+
+	if (de->deleted)
 		return NULL;
-	}
 
 	return s;
 }

@@ -484,15 +485,16 @@ static void *r_next(struct seq_file *m, 
 static void r_stop(struct seq_file *m, void *v)
 {
 	struct proc_dir_entry *de = m->private;
-	struct super_block *s = de->data;
-	deactivate_super(s);
+	struct super_block *s = de->parent->data;
+	if (v == NULL || !IS_ERR(v))
+		deactivate_super(s);
 }
 
 static int r_show(struct seq_file *m, void *v)
 {
 	struct proc_dir_entry *de = m->private;
 	int (*show)(struct seq_file *, struct super_block *) = de->data;
-	return show(m, v);
+	return show(m, de->parent->data);
 }
 
 static struct seq_operations r_ops = {

----------------------------------------------------------------------

Quoted 2 lines

Signature · 1 line
Zan Lynx <[email protected]>

[email protected]

2003-10-01 18:43 UTC #8 lore raw

Quoted 6 lines · Nikita Danilov

Cannot help but describe a little detail: r_stop() erroneously thought that de->data contains a pointer to the super block, while in reality address of some fs/reiserfs/procfs.c:show_* function was stored there. As a result, deactivate_super() danced fine fandango on core, in particular, in the case of show_oidmap() it modified first assignment within loop to reset loop counter back to zero.

*Ouch*

Thanks for spotting. IMO there's an easier fix, though - I see what you do with ERR_PTR() and it's a clever way to pass information, but it would be much more straightforward to have the following:

r_start() - as now

static void *r_next(struct seq_file *m, void *v, loff_t *pos)

{
	++*pos;
	if (v)
		deactivate_super(v);
	return NULL;
}

static void r_stop(struct seq_file *m, void *v)

{
	if (v)
		deactivate_super(v);
}

r_show() - as now.

We don't need to crawl in de->... guts past that point in ->start() - after all, past that point we'll have a pointer to our superblock passed as argument.

Nikita Danilov

2003-10-02 10:08 UTC #9 lore raw

Quoted 10 lines · [email protected] writes:

Quoted 6 lines · Nikita Danilov
Cannot help but describe a little detail: r_stop() erroneously thought that de->data contains a pointer to the super block, while in reality address of some fs/reiserfs/procfs.c:show_* function was stored there. As a result, deactivate_super() danced fine fandango on core, in particular, in the case of show_oidmap() it modified first assignment within loop to reset loop counter back to zero.
*Ouch*

There are few other things that bother me:

1. r_start() call sget() and this seems somewhat expensive (sget() starts by allocating new super block just for case) to be done at the beginning of each read operation, especially given that we already have pointer to the super block. Probably slook() function that only looks for the super block without creating it would be useful.

2. More importantly, sget() doesn't actually seem to provide protection against races with umount (which I think is its purpose): test_sb callback will succeed for any super block with the same *address*, which may be (and will be, given slab allocator) some completely unrelated super block that just happened to be allocated at the same address as (already freed) de->parent->data. Roughly speaking, super block in de->parent->data is untrusted pointer and, as such, is almost completely useless.

This is why original fs/reiserfs/procfs.c code stored dev_t in proc entry and played unholy games with it: if we look super block up by dev_t and then check that it is actually reiserfs super block, races with umount are not important.

This seems to display deeper problem: VFS has no idea that files in /proc/fs/reiserfs/<devname> (or /sys/fs/reiser4/<devname>) are related to the file system mounted at /<mountpoint> and, hence, provide no synchronization.

What about creating fake struct vfsmount for /proc/fs/reiserfs/<devname> and attaching it to the super block of /<mountpoint>? After all /proc/fs/reiserfs/<devname> is just a view into /<mountpoint>. This will automatically guarantee that /<mountpoint> cannot be unmounted while files in /proc/fs/reiserfs/<devname> are opened. Will this screw up dcache?

Quoted 21 lines

Thanks for spotting. IMO there's an easier fix, though - I see what you do with ERR_PTR() and it's a clever way to pass information, but it would be much more straightforward to have the following:
r_start() - as now
static void *r_next(struct seq_file *m, void *v, loff_t *pos)
{
	++*pos;
	if (v)
		deactivate_super(v);
	return NULL;
}
static void r_stop(struct seq_file *m, void *v)
{
	if (v)
		deactivate_super(v);
}

Quoted 4 lines

r_show() - as now.
We don't need to crawl in de->... guts past that point in ->start() - after all, past that point we'll have a pointer to our superblock passed as argument.

Yes, possible. I had similar problem in reiser4 also: for some seq_file, x_start() allocated some data structure (struct x_struct) that is used by x_next(), x_show(), and is deallocated in x_stop(). As x_struct is per read invocation rather than per file it cannot be stored in seq_file itself. I had to resort to returning x_struct from x_start() and passing it without change through x_next()'s. Thing actually changed by x_next() was embedded in x_struct. Last x_next() had to deallocate x_struct and to return NULL (much like your example above).

What about changing seq_file interface to allow passing additional data like:

struct seq_operations {
	void * (*start) (struct seq_file *m, void **ctx, loff_t *pos);
	void (*stop) (struct seq_file *m, void *ctx, void *v);
	void * (*next) (struct seq_file *m, void *ctx, void *v, loff_t *pos);
	int (*show) (struct seq_file *m, void *ctx, void *v);
};

Or may be

struct seq_context {
void *v; /* this is changed by ->next() */
loff_t pos; /* current position */
void *ctx; /* for private use */
};

struct seq_operations {
	int  (*start) (struct seq_file *m, struct seq_context *ctx);
	void (*stop) (struct seq_file *m, struct seq_context *ctx);
	int  (*next) (struct seq_file *m, struct seq_context *ctx);
	int  (*show) (struct seq_file *m, struct seq_context *ctx);
};

In the latter case all methods return error codes or 0 for success. ->next() returns 0 for stop or >0 for continue.

Nikita.

[email protected]

2003-10-02 10:26 UTC #10 lore raw

Quoted 6 lines · Nikita Danilov

1. r_start() call sget() and this seems somewhat expensive (sget() starts by allocating new super block just for case) to be done at the beginning of each read operation, especially given that we already have pointer to the super block. Probably slook() function that only looks for the super block without creating it would be useful.

Quoted 8 lines

2. More importantly, sget() doesn't actually seem to provide protection against races with umount (which I think is its purpose): test_sb callback will succeed for any super block with the same *address*, which may be (and will be, given slab allocator) some completely unrelated super block that just happened to be allocated at the same address as (already freed) de->parent->data. Roughly speaking, super block in de->parent->data is untrusted pointer and, as such, is almost completely useless.

That's OK. Yes, we could get reused superblock. Which is perfectly fine, since
a) whatever superblock we get, we know that it won't go away
b) if it had been reused, we will know that immediately - in that
case ->put_super() had already run its course and thus de->deleted had been set before sget() had returned.

IOW, sget() alone would *not* be enough, but sget() + check for ->deleted + desactivate_super() if ->deleted was set *is*. See how it works?

The first point is true and applies not only to that case - indeed, sget() is pessimistic. It should be dealt with in sget() itself, though, and it's easy to deal with. The same way as iget() does it.

fs/super.c +11 -5

diff -urN B6-rest/fs/super.c B6-current/fs/super.c
@@ -226,13 +226,10 @@
 			int (*set)(struct super_block *,void *),
 			void *data)
 {
-	struct super_block *s = alloc_super();
+	struct super_block *s = NULL;
 	struct list_head *p;
 	int err;
 
-	if (!s)
-		return ERR_PTR(-ENOMEM);
-
 retry:
 	spin_lock(&sb_lock);
 	if (test) list_for_each(p, &type->fs_supers) {

@@ -242,9 +239,18 @@
 			continue;
 		if (!grab_super(old))
 			goto retry;
-		destroy_super(s);
+		if (s)
+			destroy_super(s);
 		return old;
 	}
+	if (!s) {
+		spin_unlock(&sb_lock);
+		s = alloc_super();
+		if (!s)
+			return ERR_PTR(-ENOMEM);
+		goto retry;
+	}
+		
 	err = set(s, data);
 	if (err) {
 		spin_unlock(&sb_lock);

[email protected]

2003-10-02 10:35 UTC #11 lore raw

In reply to Nikita Danilov

Quoted 6 lines · Nikita Danilov

What about creating fake struct vfsmount for /proc/fs/reiserfs/<devname> and attaching it to the super block of /<mountpoint>? After all /proc/fs/reiserfs/<devname> is just a view into /<mountpoint>. This will automatically guarantee that /<mountpoint> cannot be unmounted while files in /proc/fs/reiserfs/<devname> are opened. Will this screw up dcache?

I don't see what it would buy you - you get to revalidate the pointer to vfsmount instead of revalidating pointer to superblock, which is not easier. sget()-based revalidation actually works OK - see previous reply for details. With optimistic sget() (which we should've done a long time ago) it's actually cheaper than your original "search by dev_t" - you are looking only through reiserfs superblocks instead of all superblocks in system.

Quoted 8 lines

Yes, possible. I had similar problem in reiser4 also: for some seq_file, x_start() allocated some data structure (struct x_struct) that is used by x_next(), x_show(), and is deallocated in x_stop(). As x_struct is per read invocation rather than per file it cannot be stored in seq_file itself. I had to resort to returning x_struct from x_start() and passing it without change through x_next()'s. Thing actually changed by x_next() was embedded in x_struct. Last x_next() had to deallocate x_struct and to return NULL (much like your example above).

Umm... And what's the problem with that? If your context is created by ->start() and is needed by ->next() and ->stop(), the above is obvious solution - you are already passing opaque pointer through that sequence, so that's about as straightforward as it gets...

Nikita Danilov

2003-10-02 10:51 UTC #12 lore raw

Quoted 10 lines · [email protected] writes:

Quoted 6 lines · Nikita Danilov
What about creating fake struct vfsmount for /proc/fs/reiserfs/<devname> and attaching it to the super block of /<mountpoint>? After all /proc/fs/reiserfs/<devname> is just a view into /<mountpoint>. This will automatically guarantee that /<mountpoint> cannot be unmounted while files in /proc/fs/reiserfs/<devname> are opened. Will this screw up dcache?
I don't see what it would buy you - you get to revalidate the pointer to vfsmount instead of revalidating pointer to superblock, which is not easier.

I thought that opening procfs file would do mntget() that will pin super block of host file system. Wouldn't it?

Quoted 4 lines

sget()-based revalidation actually works OK - see previous reply for details. With optimistic sget() (which we should've done a long time ago) it's actually cheaper than your original "search by dev_t" - you are looking only through reiserfs superblocks instead of all superblocks in system.

Yes, I agree.

Quoted 10 lines

Quoted 9 lines
Yes, possible. I had similar problem in reiser4 also: for some seq_file, x_start() allocated some data structure (struct x_struct) that is used by x_next(), x_show(), and is deallocated in x_stop(). As x_struct is per read invocation rather than per file it cannot be stored in seq_file itself. I had to resort to returning x_struct from x_start() and passing it without change through x_next()'s. Thing actually changed by x_next() was embedded in x_struct. Last x_next() had to deallocate x_struct and to return NULL (much like your example above).

Quoted 4 lines

Umm... And what's the problem with that? If your context is created by ->start() and is needed by ->next() and ->stop(), the above is obvious solution - you are already passing opaque pointer through that sequence, so that's about as straightforward as it gets...

It looks unnatural to return constant from ->next(), hiding mutable part inside.

Also code duplication in ->next() and ->stop(). No objective reasons, but new interface will provide more clean (and intuitive to me) separation of error reporting, iteration context, and iteration cookie.

Nikita.

[email protected]

2003-10-02 12:01 UTC #13 lore raw

[Linus, please wait with applying the patch below until ACK from Nikita, OK?]

Quoted 9 lines · Nikita Danilov

Quoted 8 lines · [email protected] writes:
Quoted 6 lines · Nikita Danilov
What about creating fake struct vfsmount for /proc/fs/reiserfs/<devname> and attaching it to the super block of /<mountpoint>? After all /proc/fs/reiserfs/<devname> is just a view into /<mountpoint>. This will automatically guarantee that /<mountpoint> cannot be unmounted while files in /proc/fs/reiserfs/<devname> are opened. Will this screw up dcache?

Quoted 5 lines

Quoted 2 lines
I don't see what it would buy you - you get to revalidate the pointer to vfsmount instead of revalidating pointer to superblock, which is not easier.
I thought that opening procfs file would do mntget() that will pin super block of host file system. Wouldn't it?

That wouldn't help. Look, the real problem here is that at some point you need to decide that filesystem is getting shut down. Doesn't really matter how you do it - until some moment superblock is up and running, after it we start shutting the things down.

Now, you want two places that would get access to superblock (directly or not, again, it doesn't matter): mountpoint and file in procfs. Mountpoint is given up by explicit action - umount. You want that action to trigger removal of file in procfs. I.e. you don't want simple presense of that file to pin the thing down. OTOH, you want IO on that file to hold the filesystem until we are done. Whether we do that in open() or in read(), we still have a transition point somewhere.

And that transition is where the trouble is. The object you want to access is superblock. So no matter what you do, you will need to get hold of it. Which brings us back to revalidation of pointers to superblocks...

There *is* an alternative, and it might be worth considering, but it's much more intrusive. We might give these files a separate superblock and have them mounted explicitly. Then we are fine - this superblock would have a pointer to vfsmount or reiserfs superblock directly and would pin it down as long as it's mounted. It would look like that:

mount -t reisermeta <pathname of reiserfs mountpoint> <some directory>

and we get these files under <some directory>. That would avoid all problems nicely and it's not hard to implement, but it's definitely more intrusive than sget()-based revalidation and it creates a user-visible interface change.

It might be worth doing as an alternative interface (Jeff Garzik had played with similar stuff for ext2 metadata/defragmenting/etc., so there's even some existing code), but I would rather go for minimally intrusive correct fix right now.

Speaking of seq_file... It's not impossible to do if this context is kept on stack of ->start()/->next()/->stop()/->show() callers, but I'm not sure it buys us enough to be worth doing.

If you are OK with the patch below - please ACK it. AFAICS it's the minimal fix and combined with optimistic sget() patch it should address all objections.

fs/reiserfs/procfs.c +4 -3

diff -urN B6-rest/fs/reiserfs/procfs.c B6-current/fs/reiserfs/procfs.c
@@ -478,14 +478,15 @@
 static void *r_next(struct seq_file *m, void *v, loff_t *pos)
 {
 	++*pos;
+	if (v)
+		deactivate_super(v);
 	return NULL;
 }
 
 static void r_stop(struct seq_file *m, void *v)
 {
-	struct proc_dir_entry *de = m->private;
-	struct super_block *s = de->data;
-	deactivate_super(s);
+	if (v)
+		deactivate_super(v);
 }
 
 static int r_show(struct seq_file *m, void *v)

Nikita Danilov

2003-10-02 15:40 UTC #14 lore raw

Quoted 2 lines · [email protected] writes:

[Linus, please wait with applying the patch below until ACK from Nikita, OK?]

[...]

Quoted 4 lines

If you are OK with the patch below - please ACK it. AFAICS it's the minimal fix and combined with optimistic sget() patch it should address all objections.

Yes, it works (cum optimistic sget()).

Quoted 13 lines

diff -urN B6-rest/fs/reiserfs/procfs.c B6-current/fs/reiserfs/procfs.c

fs/reiserfs/procfs.c +2 -0

@@ -478,14 +478,15 @@
 static void *r_next(struct seq_file *m, void *v, loff_t *pos)
 {
 	++*pos;
+	if (v)
+		deactivate_super(v);
 	return NULL;
 }

fs/reiserfs/procfs.c line 492

Quoted 10 lines

fs/reiserfs/procfs.c +2 -3

 static void r_stop(struct seq_file *m, void *v)
 {
-	struct proc_dir_entry *de = m->private;
-	struct super_block *s = de->data;
-	deactivate_super(s);
+	if (v)
+		deactivate_super(v);
 }
 
 static int r_show(struct seq_file *m, void *v)

Nikita.