Reiserfs.c bug in 3.2-rc5

Reiserfs.c bug in 3.2-rc5

[Jorge Bastos]

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

Howdy,

Have a VM in hyper-v, and with 3.2x reiserfs.c crashes.
Attached there's the images, since I'm unable to copy text only.

What can be done here?
Thanks,
Jorge,

[-- Attachment #2: bug 3.2-rc5_1.jpg --]
[-- Type: image/jpeg, Size: 120662 bytes --]

[-- Attachment #3: bug 3.2-rc5_2.png --]
[-- Type: image/png, Size: 40668 bytes --]

[-- Attachment #4: bug 3.2-rc5_3.jpg --]
[-- Type: image/jpeg, Size: 120721 bytes --]

Re: Reiserfs.c bug in 3.2-rc5

[Jan Kara]

  Hello,

On Sat 10-12-11 23:48:40, Jorge Bastos wrote:
> Have a VM in hyper-v, and with 3.2x reiserfs.c crashes.
> Attached there's the images, since I'm unable to copy text only.
> 
> What can be done here?
  The assertion failing is:
        BUG_ON(nblocks > journal->j_trans_max);
in do_journal_begin_r(). That means that reiserfs_create() tried to start a
transaction with more credits than allowed by the journal.

  When did you started to see this problem? Also do you use SELinux (or
generally security labels)?

								Honza

-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

RE: Reiserfs.c bug in 3.2-rc5

[Jorge Bastos]

Howdy,

> >
> > What can be done here?
>   The assertion failing is:
>         BUG_ON(nblocks > journal->j_trans_max);
> in do_journal_begin_r(). That means that reiserfs_create() tried to
> start a
> transaction with more credits than allowed by the journal.
> 
>   When did you started to see this problem? Also do you use SELinux (or
> generally security labels)?
> 

No SELinux, the only change is from 3.1.1 to 3.2-rc5.
Tried the new 3.2-rc7 and the problem persists.

Is there a way that the ReiserFS people can commit the fix?

Jorge,

Re: Reiserfs.c bug in 3.2-rc5

[Jan Kara]

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

On Sat 24-12-11 11:55:43, Jorge Bastos wrote:
> > >
> > > What can be done here?
> >   The assertion failing is:
> >         BUG_ON(nblocks > journal->j_trans_max);
> > in do_journal_begin_r(). That means that reiserfs_create() tried to
> > start a
> > transaction with more credits than allowed by the journal.
> > 
> >   When did you started to see this problem? Also do you use SELinux (or
> > generally security labels)?
> > 
> 
> No SELinux, the only change is from 3.1.1 to 3.2-rc5.
> Tried the new 3.2-rc7 and the problem persists.
  OK, so do I understand right that 3.1.1 is OK but 3.2-rc5/rc7 have
the problem?

> Is there a way that the ReiserFS people can commit the fix?
  Sure, we just have to find where the problem is. Can you please run with
the attached debugging patch applied and send me kernel output when the
system crashes? Thanks.

								Honza

-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[-- Attachment #2: 0001-reiserfs-Debugging-patch.patch --]
[-- Type: text/x-patch, Size: 1937 bytes --]

>From 4dd005ac485a6e86e2f81995894e9f8f6a352557 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Mon, 2 Jan 2012 12:49:06 +0100
Subject: [PATCH] reiserfs: Debugging patch

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/reiserfs/journal.c |    8 +++++++-
 fs/reiserfs/namei.c   |    7 +++++++
 2 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/fs/reiserfs/journal.c b/fs/reiserfs/journal.c
index eb71106..c7ff04a 100644
--- a/fs/reiserfs/journal.c
+++ b/fs/reiserfs/journal.c
@@ -3003,7 +3003,13 @@ static int do_journal_begin_r(struct reiserfs_transaction_handle *th,
 	int retval;
 
 	reiserfs_check_lock_depth(sb, "journal_begin");
-	BUG_ON(nblocks > journal->j_trans_max);
+	if (nblocks > journal->j_trans_max) {
+		printk(KERN_ERR "Too many blocks for reiserfs a transaction"
+			" (%lu > %lu)\n", nblocks,
+			(unsigned long)journal->j_trans_max);
+		dump_stack();
+		return -EINVAL;
+	}
 
 	PROC_INFO_INC(sb, journal.journal_being);
 	/* set here for journal_join */
diff --git a/fs/reiserfs/namei.c b/fs/reiserfs/namei.c
index 80058e8..1a05abe 100644
--- a/fs/reiserfs/namei.c
+++ b/fs/reiserfs/namei.c
@@ -584,6 +584,7 @@ static int reiserfs_create(struct inode *dir, struct dentry *dentry, int mode,
 		 REISERFS_QUOTA_TRANS_BLOCKS(dir->i_sb));
 	struct reiserfs_transaction_handle th;
 	struct reiserfs_security_handle security;
+	int security_ret;
 
 	dquot_initialize(dir);
 
@@ -598,11 +599,17 @@ static int reiserfs_create(struct inode *dir, struct dentry *dentry, int mode,
 		drop_new_inode(inode);
 		return retval;
 	}
+	security_ret = retval;
 	jbegin_count += retval;
 	reiserfs_write_lock(dir->i_sb);
 
 	retval = journal_begin(&th, dir->i_sb, jbegin_count);
 	if (retval) {
+		if (retval == -EINVAL) {
+			printk(KERN_ERR "reiserfs_security_init() returned %d"
+				" (dir=%lu)\n", security_ret, dir->i_ino);
+			BUG_ON(1);
+		}
 		drop_new_inode(inode);
 		goto out_failed;
 	}
-- 
1.7.1

Re: Reiserfs.c bug in 3.2-rc5

[Jan Kara]

  Hello,

On Mon 02-01-12 21:47:29, Jorge Bastos wrote:
> > > No SELinux, the only change is from 3.1.1 to 3.2-rc5.
> > > Tried the new 3.2-rc7 and the problem persists.
> >   OK, so do I understand right that 3.1.1 is OK but 3.2-rc5/rc7 have
> > the problem?
> > 
> > > Is there a way that the ReiserFS people can commit the fix?
> >   Sure, we just have to find where the problem is. Can you please run
> > with the attached debugging patch applied and send me kernel output
> > when the system crashes? Thanks.
> 
> Yes, till 3.1.6 everything works, with 3.2-rc7 (or rc5) it doesn't.
> 
> Here are the images with the output, I can't copy the text since this is a
> VM.
> 
> http://neotrix.decimal.pt/bugs/reiserfs_1.jpg
> http://neotrix.decimal.pt/bugs/reiserfs_2.jpg
> http://neotrix.decimal.pt/bugs/reiserfs_3.jpg
> http://neotrix.decimal.pt/bugs/reiserfs_4.jpg 
> 
> Does this help? If not let me know further instructions.
  Yes, thanks! So we see that reiserfs_security_init() returned bogus
number 790797. This is caused by security_old_inode_init_security() either
returning some bogus number or setting sec->length to something bogus. In
any case it's some security module problem so I'm adding Mimi Zohar who did
the changes in this code recently to CC. I'm also adding Rafael since this
is a regression.

  Also I think he might use your kernel config so can you please send it?
Thanks.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: Reiserfs.c bug in 3.2-rc5

[Jan Kara]

  Hell,

On Tue 03-01-12 09:31:22, Jorge Bastos wrote:
> >   Yes, thanks! So we see that reiserfs_security_init() returned bogus
> >   number 790797. This is caused by security_old_inode_init_security()
> >   either returning some bogus number or setting sec->length to
> >   something bogus.  In any case it's some security module problem so
> >   I'm adding Mimi Zohar who did the changes in this code recently to
> >   CC. I'm also adding Rafael since this is a regression.
> > 
> >   Also I think he might use your kernel config so can you please send
> > it?
> > Thanks.
> 
> Sure,
> Please grab it here:
> 
> http://neotrix.decimal.pt/bugs/config-3.2-rc7.txt 
> 
> Let me know when the fix gets commited git master.
  Thanks! So I've managed to reproduce the problem and I now understand
what is the problem. Commit 1e39f384bb01b0395b69cb70c2cacae65012f203 makes
security_old_inode_init_security() return 0 when CONFIG_SECURITY is not
set. But that makes caller such as reiserfs_security_init() assume that
security_old_inode_init_security() has set name, value, and len arguments
properly (which is IMO correct assumption). But they were left
uninitialized which makes things break in an interesting ways... The fix is
below.

Mimi, James, Linus, this patch fixes a regression from 3.1 and should make
it to 3.2 if possible.

								Honza
----

>From 5bcd17065fa27d5f27756e24a98331f796ff2481 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Tue, 3 Jan 2012 13:14:29 +0100
Subject: [PATCH] security: Fix security_old_inode_init_security() when CONFIG_SECURITY is not set

Commit 1e39f384bb01b0395b69cb70c2cacae65012f203 makes
security_old_inode_init_security() return 0 when CONFIG_SECURITY is not set.
But that makes callers such as reiserfs_security_init() assume that
security_old_inode_init_security() has set name, value, and len arguments
properly. But security_old_inode_init_security() left them uninitialized
which then results in interesting failures.

Revert security_old_inode_init_security() to the old behavior of returning
EOPNOTSUPP since both callers (reiserfs and ocfs2) handle this just fine.

Reported-by: Jorge Bastos <mysql.jorge@decimal.pt>
CC: James Morris <jmorris@namei.org>
CC: Mimi Zohar <zohar@us.ibm.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 include/linux/security.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index 19d8e04..e8c619d 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -2056,7 +2056,7 @@ static inline int security_old_inode_init_security(struct inode *inode,
 						   char **name, void **value,
 						   size_t *len)
 {
-	return 0;
+	return -EOPNOTSUPP;
 }
 
 static inline int security_inode_create(struct inode *dir,
-- 
1.7.1

Re: Reiserfs.c bug in 3.2-rc5

[Mimi Zohar]

On Tue, 2012-01-03 at 13:38 +0100, Jan Kara wrote:
> Hell,
> 
> On Tue 03-01-12 09:31:22, Jorge Bastos wrote:
> > >   Yes, thanks! So we see that reiserfs_security_init() returned bogus
> > >   number 790797. This is caused by security_old_inode_init_security()
> > >   either returning some bogus number or setting sec->length to
> > >   something bogus.  In any case it's some security module problem so
> > >   I'm adding Mimi Zohar who did the changes in this code recently to
> > >   CC. I'm also adding Rafael since this is a regression.
> > > 
> > >   Also I think he might use your kernel config so can you please send
> > > it?
> > > Thanks.
> > 
> > Sure,
> > Please grab it here:
> > 
> > http://neotrix.decimal.pt/bugs/config-3.2-rc7.txt 
> > 
> > Let me know when the fix gets commited git master.
>   Thanks! So I've managed to reproduce the problem and I now understand
> what is the problem. Commit 1e39f384bb01b0395b69cb70c2cacae65012f203 makes
> security_old_inode_init_security() return 0 when CONFIG_SECURITY is not
> set. But that makes caller such as reiserfs_security_init() assume that
> security_old_inode_init_security() has set name, value, and len arguments
> properly (which is IMO correct assumption). But they were left
> uninitialized which makes things break in an interesting ways... The fix is
> below.
> 
> Mimi, James, Linus, this patch fixes a regression from 3.1 and should make
> it to 3.2 if possible.
> 
> 								Honza

Commit fb88c2b changed the security_old_inode_init_security() return
code for S_PRIVATE inodes. As long as -EOPNOTSUPP is expected, probably
should revert that as well.

(I'm cc'ing the ocfs2 maintainers.)

thanks,

Mimi

> ----
> 
> From 5bcd17065fa27d5f27756e24a98331f796ff2481 Mon Sep 17 00:00:00 2001
> From: Jan Kara <jack@suse.cz>
> Date: Tue, 3 Jan 2012 13:14:29 +0100
> Subject: [PATCH] security: Fix security_old_inode_init_security() when CONFIG_SECURITY is not set
> 
> Commit 1e39f384bb01b0395b69cb70c2cacae65012f203 makes
> security_old_inode_init_security() return 0 when CONFIG_SECURITY is not set.
> But that makes callers such as reiserfs_security_init() assume that
> security_old_inode_init_security() has set name, value, and len arguments
> properly. But security_old_inode_init_security() left them uninitialized
> which then results in interesting failures.
> 
> Revert security_old_inode_init_security() to the old behavior of returning
> EOPNOTSUPP since both callers (reiserfs and ocfs2) handle this just fine.
> 
> Reported-by: Jorge Bastos <mysql.jorge@decimal.pt>
> CC: James Morris <jmorris@namei.org>
> CC: Mimi Zohar <zohar@us.ibm.com>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  include/linux/security.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 19d8e04..e8c619d 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -2056,7 +2056,7 @@ static inline int security_old_inode_init_security(struct inode *inode,
>  						   char **name, void **value,
>  						   size_t *len)
>  {
> -	return 0;
> +	return -EOPNOTSUPP;
>  }
> 
>  static inline int security_inode_create(struct inode *dir,

Re: Reiserfs.c bug in 3.2-rc5

[Linus Torvalds]

On Tue, Jan 3, 2012 at 7:25 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>
> Commit fb88c2b changed the security_old_inode_init_security() return
> code for S_PRIVATE inodes. As long as -EOPNOTSUPP is expected, probably
> should revert that as well.
>
> (I'm cc'ing the ocfs2 maintainers.)

Jan, Joel? Can you confirm? I was planning on doing 3.2 today, but
maybe I can't, or maybe we need to punt this to be a stable patch.

But maybe we can have quick testing and agreement? Does everybody
agree on both Honza's patch *and* revert of fb88c2b? Does the
EOPNOTSUPP return case match what we used to do?

                          Linus

Re: Reiserfs.c bug in 3.2-rc5

[Mimi Zohar]

On Tue, 2012-01-03 at 08:48 -0800, Linus Torvalds wrote:
> On Tue, Jan 3, 2012 at 7:25 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> >
> > Commit fb88c2b changed the security_old_inode_init_security() return
> > code for S_PRIVATE inodes. As long as -EOPNOTSUPP is expected, probably
> > should revert that as well.
> >
> > (I'm cc'ing the ocfs2 maintainers.)
> 
> Jan, Joel? Can you confirm? I was planning on doing 3.2 today, but
> maybe I can't, or maybe we need to punt this to be a stable patch.
> 
> But maybe we can have quick testing and agreement? Does everybody
> agree on both Honza's patch *and* revert of fb88c2b? Does the
> EOPNOTSUPP return case match what we used to do?
> 
>                           Linus
> 

Just clarifying not all of commit fb88c2b, but only the
security_old_inode_init_security() hunk.

Mimi

Re: Reiserfs.c bug in 3.2-rc5

[Linus Torvalds]

On Tue, Jan 3, 2012 at 10:45 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>
> Just clarifying not all of commit fb88c2b, but only the
> security_old_inode_init_security() hunk.

Is it really sane to have different semantics like that for the
security[_old]_inode_init_security functions?

Look at ocfs2, for example: it does nothing if
ocfs2_init_security_get() returns 0. That does not sound like the
correct thing to do, when the fallback is to do ocfs2_init_acl() under
the lock.

And ocfs2_init_security_get() just calls either
security_inode_init_security() or security_old_inode_init_security()
depending on whether ocfs2_security_xattr_info is NULL or not. So I
really think callers expect the same kind of semantics regardless of
whether it's the "old" or not version. Which would make sense anyway.

Also, the *documentation* in include/linux/security.h very much says
that it returns 0 only if @name and @value have been successfully set.
So my gut feel says that both security_inode_init_security and
security_old_inode_init_security should return -EOPNOTSUPP (although
the "new" version doesn't really have "name/value", so maybe returning
0 is ok)

Anyway, I'd love for (multiple) people who really know the code to
give me a clean agreement on exactly what the correct patch is.
Please?

                                 Linus

Re: Reiserfs.c bug in 3.2-rc5

[Mimi Zohar]

On Tue, 2012-01-03 at 11:17 -0800, Linus Torvalds wrote:
> On Tue, Jan 3, 2012 at 10:45 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> >
> > Just clarifying not all of commit fb88c2b, but only the
> > security_old_inode_init_security() hunk.
> 
> Is it really sane to have different semantics like that for the
> security[_old]_inode_init_security functions?

No, but unfortunately it seems necessary.

> Look at ocfs2, for example: it does nothing if
> ocfs2_init_security_get() returns 0. That does not sound like the
> correct thing to do, when the fallback is to do ocfs2_init_acl() under
> the lock.

The original code did the same for -EOPNOTSUPP.

7192        ret = ocfs2_init_security_get(inode, dir, qstr, &si);
7193        if (!ret) {
7194                ret = ocfs2_xattr_set(inode, OCFS2_XATTR_INDEX_SECURITY,
7195                                      si.name, si.value, si.value_len,
7196                                      XATTR_CREATE);
7197                if (ret) {
7198                        mlog_errno(ret);
7199                        goto leave;
7200                }
7201        } else if (ret != -EOPNOTSUPP) {
7202                mlog_errno(ret);
7203                goto leave;
7204        }
7205
7206        ret = ocfs2_inode_lock(dir, &dir_bh, 0);
7207        if (ret) {
7208                mlog_errno(ret);
7209                goto leave;
7210        }
7211
7212        ret = ocfs2_init_acl(NULL, inode, dir, NULL, dir_bh, NULL, NULL);
7213        if (ret)
7214                mlog_errno(ret);
7215

> And ocfs2_init_security_get() just calls either
> security_inode_init_security() or security_old_inode_init_security()
> depending on whether ocfs2_security_xattr_info is NULL or not. So I
> really think callers expect the same kind of semantics regardless of
> whether it's the "old" or not version. Which would make sense anyway.
> 
> Also, the *documentation* in include/linux/security.h very much says
> that it returns 0 only if @name and @value have been successfully set.
> So my gut feel says that both security_inode_init_security and
> security_old_inode_init_security should return -EOPNOTSUPP (although
> the "new" version doesn't really have "name/value", so maybe returning
> 0 is ok)

The original security_inode_init_security() version queried the LSM for
the security xattr, leaving writing the xattr up to the caller.  The
caller changed -EOPNPTSUPP to 0, before returning.  The new version
combines the querying and writing the xattr.  Like the previous version
it converts the -EOPNOTSUPP to 0, before returning.

reiserfs_security_init() is dependent on
security_old_inode_init_security() to return -EOPNOTSUPP to initialize
some variables and return, but before returning it changes -EOPNOTSUPP
to 0.

Unfortunately this leaves security_old/security_inode_init_security()
needing to return different things.

Mimi

> Anyway, I'd love for (multiple) people who really know the code to
> give me a clean agreement on exactly what the correct patch is.
> Please?
> 
>                                  Linus
> 

Re: Reiserfs.c bug in 3.2-rc5

[James Morris]

On Tue, 3 Jan 2012, Mimi Zohar wrote:

> > EOPNOTSUPP return case match what we used to do?
> > 
> >                           Linus
> > 
> 
> Just clarifying not all of commit fb88c2b, but only the
> security_old_inode_init_security() hunk.

Agreed.

And yep, it's ugly :-\


- James
-- 
James Morris
<jmorris@namei.org>

Re: Reiserfs.c bug in 3.2-rc5

[Linus Torvalds]

On Tue, Jan 3, 2012 at 3:47 PM, James Morris <jmorris@namei.org> wrote:
>>
>> Just clarifying not all of commit fb88c2b, but only the
>> security_old_inode_init_security() hunk.
>
> Agreed.
>
> And yep, it's ugly :-\

Ok, applied - along with another last-minute fix - and pushed out.

Can everybody please test and double-check that what I applied was
what everybody agrees is the RightThing(tm)? I really wanted to do 3.2
today, but both of those commits are of the "please double-check"
kind, so..

                Linus

Re: Reiserfs.c bug in 3.2-rc5

[James Morris]

On Tue, 3 Jan 2012, Linus Torvalds wrote:

> Can everybody please test and double-check that what I applied was
> what everybody agrees is the RightThing(tm)? I really wanted to do 3.2
> today, but both of those commits are of the "please double-check"
> kind, so..

Boots up ok for me, although I'm not using reiserfs or ocfs2.  The patch 
looks correct.


Thanks.



- James
-- 
James Morris
<jmorris@namei.org>

Re: Reiserfs.c bug in 3.2-rc5

[Jan Kara]

On Tue 03-01-12 16:18:01, Linus Torvalds wrote:
> On Tue, Jan 3, 2012 at 3:47 PM, James Morris <jmorris@namei.org> wrote:
> >>
> >> Just clarifying not all of commit fb88c2b, but only the
> >> security_old_inode_init_security() hunk.
> >
> > Agreed.
> >
> > And yep, it's ugly :-\
> 
> Ok, applied - along with another last-minute fix - and pushed out.
> 
> Can everybody please test and double-check that what I applied was
> what everybody agrees is the RightThing(tm)? I really wanted to do 3.2
> today, but both of those commits are of the "please double-check"
> kind, so..
  Thanks. The patch looks OK and current HEAD works for me.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR