[PATCH] xfs: add more checks to superblock validation

[PATCH] xfs: add more checks to superblock validation

[Felix Blyakher]

From: Olaf Weber <olaf@sgi.com>

There had been reports where xfs filesystem was randomly
corrupted with fsfuzzer, and xfs failed to handle it
gracefully. This patch fixes couple of reported problem
by providing additional checks in the superblock
validation routine.

Signed-off-by: Felix Blyakher <felixb@sgi.com>
---
 fs/xfs/xfs_mount.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index b101990..65a9972 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -291,14 +291,17 @@ xfs_mount_validate_sb(
 	    sbp->sb_sectsize > XFS_MAX_SECTORSIZE			||
 	    sbp->sb_sectlog < XFS_MIN_SECTORSIZE_LOG			||
 	    sbp->sb_sectlog > XFS_MAX_SECTORSIZE_LOG			||
+	    sbp->sb_sectsize != (1 << sbp->sb_sectlog)			||
 	    sbp->sb_blocksize < XFS_MIN_BLOCKSIZE			||
 	    sbp->sb_blocksize > XFS_MAX_BLOCKSIZE			||
 	    sbp->sb_blocklog < XFS_MIN_BLOCKSIZE_LOG			||
 	    sbp->sb_blocklog > XFS_MAX_BLOCKSIZE_LOG			||
+	    sbp->sb_blocksize != (1 << sbp->sb_blocklog)		||
 	    sbp->sb_inodesize < XFS_DINODE_MIN_SIZE			||
 	    sbp->sb_inodesize > XFS_DINODE_MAX_SIZE			||
 	    sbp->sb_inodelog < XFS_DINODE_MIN_LOG			||
 	    sbp->sb_inodelog > XFS_DINODE_MAX_LOG			||
+	    sbp->sb_inodesize != (1 << sbp->sb_inodelog)		||
 	    (sbp->sb_blocklog - sbp->sb_inodelog != sbp->sb_inopblog)	||
 	    (sbp->sb_rextsize * sbp->sb_blocksize > XFS_MAX_RTEXTSIZE)	||
 	    (sbp->sb_rextsize * sbp->sb_blocksize < XFS_MIN_RTEXTSIZE)	||
-- 
1.5.4.rc3

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: add more checks to superblock validation

[Josef 'Jeff' Sipek]

On Fri, Apr 17, 2009 at 04:12:45PM -0500, Felix Blyakher wrote:
> From: Olaf Weber <olaf@sgi.com>
> 
> There had been reports where xfs filesystem was randomly
> corrupted with fsfuzzer, and xfs failed to handle it
> gracefully. This patch fixes couple of reported problem
> by providing additional checks in the superblock
> validation routine.
> 
> Signed-off-by: Felix Blyakher <felixb@sgi.com>

Since this patch is from Olaf, shouldn't he have a s-o-b line as well?

I'm not really familiar with this part of the code...but it looks fine to
me.

Josef 'Jeff' Sipek.

-- 
Fact: 29.6% of all statistics are generated randomly.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: add more checks to superblock validation

[Felix Blyakher]

On Apr 18, 2009, at 12:05 AM, Josef 'Jeff' Sipek wrote:

> On Fri, Apr 17, 2009 at 04:12:45PM -0500, Felix Blyakher wrote:
>> From: Olaf Weber <olaf@sgi.com>
>>
>> There had been reports where xfs filesystem was randomly
>> corrupted with fsfuzzer, and xfs failed to handle it
>> gracefully. This patch fixes couple of reported problem
>> by providing additional checks in the superblock
>> validation routine.
>>
>> Signed-off-by: Felix Blyakher <felixb@sgi.com>
>
> Since this patch is from Olaf, shouldn't he have a s-o-b line as well?

I was following the guidelines from the SubmittingPatches:

The "from" line must be the very first line in the message body,
and has the form:

         From: Original Author <author@example.com>

The "from" line specifies who will be credited as the author of the
patch in the permanent changelog.  If the "from" line is missing,
then the "From:" line from the email header will be used to determine
the patch author in the changelog.


So, is "From:" enough here, or "Signed-off-by" is needed as well?

Felix

> I'm not really familiar with this part of the code...but it looks  
> fine to
> me.
>
> Josef 'Jeff' Sipek.
>
> -- 
> Fact: 29.6% of all statistics are generated randomly.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: add more checks to superblock validation

[Josef 'Jeff' Sipek]

On Sun, Apr 19, 2009 at 11:39:20AM -0500, Felix Blyakher wrote:
>
> On Apr 18, 2009, at 12:05 AM, Josef 'Jeff' Sipek wrote:
>
>> On Fri, Apr 17, 2009 at 04:12:45PM -0500, Felix Blyakher wrote:
>>> From: Olaf Weber <olaf@sgi.com>
>>>
>>> There had been reports where xfs filesystem was randomly
>>> corrupted with fsfuzzer, and xfs failed to handle it
>>> gracefully. This patch fixes couple of reported problem
>>> by providing additional checks in the superblock
>>> validation routine.
>>>
>>> Signed-off-by: Felix Blyakher <felixb@sgi.com>
>>
>> Since this patch is from Olaf, shouldn't he have a s-o-b line as well?
>
> I was following the guidelines from the SubmittingPatches:
>
> The "from" line must be the very first line in the message body,
> and has the form:
>
>         From: Original Author <author@example.com>
>
> The "from" line specifies who will be credited as the author of the
> patch in the permanent changelog.  If the "from" line is missing,
> then the "From:" line from the email header will be used to determine
> the patch author in the changelog.
>
>
> So, is "From:" enough here, or "Signed-off-by" is needed as well?

The From line determines author-ship. If this is Olaf's patch, then the From
is right. My understanding is that s-o-b is intended as a "I didn't do
anything stupid (e.g., incorporate licensed code, etc.) while working on
this patch/handling this patch." This makes me believe that the author
should include a s-o-b line as well.

So, for example, whenever _I_ send a patch that I authored, I have both a
>From and a s-o-b. If someone picks it up (e.g., akpm), he'd add his s-o-b,
so when he resends it, it'd have my from, my s-o-b, and his s-o-b. As far as
I know, other kernel folks do the same.

Jeff.

-- 
My public GPG key can be found at
http://www.josefsipek.net/gpg/public-0xC7958FFE.txt

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: add more checks to superblock validation

[Felix Blyakher]

On Apr 19, 2009, at 1:14 PM, Josef 'Jeff' Sipek wrote:

> On Sun, Apr 19, 2009 at 11:39:20AM -0500, Felix Blyakher wrote:
>>
>> On Apr 18, 2009, at 12:05 AM, Josef 'Jeff' Sipek wrote:
>>
>>> On Fri, Apr 17, 2009 at 04:12:45PM -0500, Felix Blyakher wrote:
>>>> From: Olaf Weber <olaf@sgi.com>
>>>>
>>>> There had been reports where xfs filesystem was randomly
>>>> corrupted with fsfuzzer, and xfs failed to handle it
>>>> gracefully. This patch fixes couple of reported problem
>>>> by providing additional checks in the superblock
>>>> validation routine.
>>>>
>>>> Signed-off-by: Felix Blyakher <felixb@sgi.com>
>>>
>>> Since this patch is from Olaf, shouldn't he have a s-o-b line as  
>>> well?
>>
>> I was following the guidelines from the SubmittingPatches:
>>
>> The "from" line must be the very first line in the message body,
>> and has the form:
>>
>>        From: Original Author <author@example.com>
>>
>> The "from" line specifies who will be credited as the author of the
>> patch in the permanent changelog.  If the "from" line is missing,
>> then the "From:" line from the email header will be used to determine
>> the patch author in the changelog.
>>
>>
>> So, is "From:" enough here, or "Signed-off-by" is needed as well?
>
> The From line determines author-ship. If this is Olaf's patch, then  
> the From
> is right. My understanding is that s-o-b is intended as a "I didn't do
> anything stupid (e.g., incorporate licensed code, etc.) while  
> working on
> this patch/handling this patch."

That what I did before creating (from the proposed changes)
and submitting the patch (and making sure the author get the
credit).

> This makes me believe that the author
> should include a s-o-b line as well.
>
> So, for example, whenever _I_ send a patch that I authored, I have  
> both a
>> From and a s-o-b.

That seems redundant based on the following excerpt from the
SubmittingPatches:

If the "from" line is missing,
then the "From:" line from the email header will be used to determine
the patch author in the changelog.

>>  If someone picks it up (e.g., akpm), he'd add his s-o-b,
> so when he resends it, it'd have my from, my s-o-b, and his s-o-b.  
> As far as
> I know, other kernel folks do the same.

That's definitely not usual case for submitting the patch on
somebody else behalf, but I found the following entry in the log:

commit 55643171de7ba429fbf2cb72fb1f2c6f2df0dcf3
Author: Ashwin Ganti <ashwin.ganti@gmail.com>
Date:   Tue Feb 24 19:48:44 2009 -0800

     Staging: add p9auth driver

     This is a driver that adds Plan 9 style capability device
     implementation.

     From: Ashwin Ganti <ashwin.ganti@gmail.com>
     Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>


But at the end, I don't mind to follow any established
guidelines here. Just need clarifications.

Felix

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: add more checks to superblock validation

[Josef 'Jeff' Sipek]

On Tue, Apr 21, 2009 at 10:47:30AM -0500, Felix Blyakher wrote:
...
>> This makes me believe that the author
>> should include a s-o-b line as well.
>>
>> So, for example, whenever _I_ send a patch that I authored, I have  
>> both a From and a s-o-b.
>
> That seems redundant based on the following excerpt from the
> SubmittingPatches:
>
> If the "from" line is missing,
> then the "From:" line from the email header will be used to determine
> the patch author in the changelog.

If you look at the "sign your work" section in that doc, the example it
provides shows the original patch author having a s-o-b as well. ;)

Anyway, enough of this...time to hack on xfs some more :)

Jeff.

-- 
*NOTE: This message is ROT-13 encrypted twice for extra protection*

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: add more checks to superblock validation

[Christoph Hellwig]

On Fri, Apr 17, 2009 at 04:12:45PM -0500, Felix Blyakher wrote:
> From: Olaf Weber <olaf@sgi.com>
> 
> There had been reports where xfs filesystem was randomly
> corrupted with fsfuzzer, and xfs failed to handle it
> gracefully. This patch fixes couple of reported problem
> by providing additional checks in the superblock
> validation routine.

Looks good.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs