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

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

[Zan Lynx]

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

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.
-- 
Zan Lynx <zlynx@acm.org>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[Jonathan Briggs]

[-- Attachment #1.1: Type: text/plain, Size: 963 bytes --]

On Tue, 2003-09-30 at 09:44, Zan Lynx wrote:
> 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.

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

-- 
Jonathan Briggs
jbriggs@esoft.com

[-- Attachment #1.2: bug-report.gz --]
[-- Type: application/x-gzip, Size: 8972 bytes --]

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[Vitaly Fertman]

Hi

On Tuesday 30 September 2003 19:44, Zan Lynx wrote:
> 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.

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

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

-- 
Thanks,
Vitaly Fertman

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

[Hans Reiser]

Vitaly Fertman wrote:

>Hi
>
>On Tuesday 30 September 2003 19:44, Zan Lynx wrote:
>  
>
>>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.
>>    
>>
>
>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 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.
>
>  
>
>>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)?

-- 
Hans

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

[Vitaly Fertman]

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

-- 
Thanks,
Vitaly Fertman

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

[Zan Lynx]

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

On Tue, 2003-09-30 at 10:06, Vitaly Fertman wrote:
> Hi
> 
> On Tuesday 30 September 2003 19:44, Zan Lynx wrote:
> > 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.
> 
> 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.
-- 
Zan Lynx <zlynx@acm.org>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[Vitaly Fertman]

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

-- 
Thanks,
Vitaly Fertman

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

[Nikita Danilov]

Zan Lynx writes:
 > On Tue, 2003-09-30 at 10:06, Vitaly Fertman wrote:
 > > Hi
 > > 
 > > On Tuesday 30 September 2003 19:44, Zan Lynx wrote:
 > > > 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.
 > > 
 > > 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.

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.
----------------------------------------------------------------------
--- bk-linux-2.5/fs/reiserfs/procfs.c	Wed Sep 24 03:00:43 2003
+++ procfs.c	Wed Oct  1 21:37:43 2003
@@ -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 = {
----------------------------------------------------------------------
 > -- 
 > Zan Lynx <zlynx@acm.org>

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

[viro]

On Wed, Oct 01, 2003 at 09:54:44PM +0400, Nikita Danilov wrote:
 
> 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.

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

[Nikita Danilov]

viro@parcelfarce.linux.theplanet.co.uk writes:
 > On Wed, Oct 01, 2003 at 09:54:44PM +0400, Nikita Danilov wrote:
 >  
 > > 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?

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

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.

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

[viro]

On Thu, Oct 02, 2003 at 02:08:26PM +0400, Nikita Danilov wrote:
 
> 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.

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.

diff -urN B6-rest/fs/super.c B6-current/fs/super.c
--- B6-rest/fs/super.c	Sat Sep 27 22:04:57 2003
+++ B6-current/fs/super.c	Thu Oct  2 06:23:22 2003
@@ -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);

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

[viro]

On Thu, Oct 02, 2003 at 02:08:26PM +0400, Nikita Danilov wrote:
> 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.
 
> 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...

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

[Nikita Danilov]

viro@parcelfarce.linux.theplanet.co.uk writes:
 > On Thu, Oct 02, 2003 at 02:08:26PM +0400, Nikita Danilov wrote:
 > > 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?

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

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

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.

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

[viro]

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

On Thu, Oct 02, 2003 at 02:51:10PM +0400, Nikita Danilov wrote:
> viro@parcelfarce.linux.theplanet.co.uk writes:
>  > On Thu, Oct 02, 2003 at 02:08:26PM +0400, Nikita Danilov wrote:
>  > > 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?

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.

diff -urN B6-rest/fs/reiserfs/procfs.c B6-current/fs/reiserfs/procfs.c
--- B6-rest/fs/reiserfs/procfs.c	Sat Sep 27 22:04:57 2003
+++ B6-current/fs/reiserfs/procfs.c	Thu Oct  2 07:57:31 2003
@@ -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)

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

[Nikita Danilov]

viro@parcelfarce.linux.theplanet.co.uk writes:
 > [Linus, please wait with applying the patch below until ACK from Nikita, OK?]
 > 

[...]

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

 > 
 > diff -urN B6-rest/fs/reiserfs/procfs.c B6-current/fs/reiserfs/procfs.c
 > --- B6-rest/fs/reiserfs/procfs.c	Sat Sep 27 22:04:57 2003
 > +++ B6-current/fs/reiserfs/procfs.c	Thu Oct  2 07:57:31 2003
 > @@ -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.