Patchwork [1/2] ext4: Remove arbitrary block value in __es_remove_extent()

login
register
mail settings
Submitter Lukas Czerner
Date April 15, 2014, 6:08 p.m.
Message ID <1397585302-27175-1-git-send-email-lczerner@redhat.com>
Download mbox | patch
Permalink /patch/339343/
State Rejected
Headers show

Comments

Lukas Czerner - April 15, 2014, 6:08 p.m.
In __es_remove_extent() we're storing seemingly arbitrary value
0x7FDEADBEEF into block variable. I assume that the reason is just to
initialize the variable before the use because the actual value does not
matter at this point.

Just remove the arbitrary value and initialized block variable to zero
which is much less suspicious.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 fs/ext4/extents_status.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
Theodore Ts'o - April 17, 2014, 4:19 p.m.
On Tue, Apr 15, 2014 at 08:08:21PM +0200, Lukas Czerner wrote:
> In __es_remove_extent() we're storing seemingly arbitrary value
> 0x7FDEADBEEF into block variable. I assume that the reason is just to
> initialize the variable before the use because the actual value does not
> matter at this point.
> 
> Just remove the arbitrary value and initialized block variable to zero
> which is much less suspicious.

The whole point was for the value to be suspicious.  That way, if
there is a bug, and we try to use that value, it is a large enough
value that for most storage devices, we will get an I/O error (because
the disk isn't that big :-) referencing that block value, and we'll
have a bit of a hint where that suspicious value might have come from.

       	      	     	   		   - Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lukas Czerner - April 18, 2014, 9:22 a.m.
On Thu, 17 Apr 2014, Theodore Ts'o wrote:

> Date: Thu, 17 Apr 2014 12:19:44 -0400
> From: Theodore Ts'o <tytso@mit.edu>
> To: Lukas Czerner <lczerner@redhat.com>
> Cc: linux-ext4@vger.kernel.org
> Subject: Re: [PATCH 1/2] ext4: Remove arbitrary block value in
>     __es_remove_extent()
> 
> On Tue, Apr 15, 2014 at 08:08:21PM +0200, Lukas Czerner wrote:
> > In __es_remove_extent() we're storing seemingly arbitrary value
> > 0x7FDEADBEEF into block variable. I assume that the reason is just to
> > initialize the variable before the use because the actual value does not
> > matter at this point.
> > 
> > Just remove the arbitrary value and initialized block variable to zero
> > which is much less suspicious.
> 
> The whole point was for the value to be suspicious.  That way, if
> there is a bug, and we try to use that value, it is a large enough
> value that for most storage devices, we will get an I/O error (because
> the disk isn't that big :-) referencing that block value, and we'll
> have a bit of a hint where that suspicious value might have come from.

Aside from the fact that this is totally undocumented and there is
not even comment on what is that all about in couple of years we
might actually get file systems big enough that this would not be an
I/O error anymore (that might be a bit of a stretch).

But mainly this value is only going to be used if it is delayed
extent or a hole which implies that it has not been mapped and
pblock does not contain anything valid. And if we really screwed it
up and tried to use pblock of extent which is a hole or delayed
extent, then it would not help us anyway since the only place that
we actually set this is when splitting extent on removal.

Now I can see that in ext4_da_map_blocks() we're actually using ~0
value for the pblock which is a bit better I think as long as we're
using this reliably. So I'll resend the patch which will make sure
that we're using ~0 reliably when storin delayed, or hole extents in
the extent status tree. Does that make sense ?

-Lukas

> 
>        	      	     	   		   - Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o - April 18, 2014, 11:18 a.m.
On Fri, Apr 18, 2014 at 11:22:12AM +0200, Lukáš Czerner wrote:
> Aside from the fact that this is totally undocumented and there is
> not even comment on what is that all about in couple of years we
> might actually get file systems big enough that this would not be an
> I/O error anymore (that might be a bit of a stretch).

Well, do you have some suggestions about what might be a good place to
document something like this?  My assumption is that it's something
that would be used by developers after a bug had been reported, so
presumably it would be someplace in the source code.

And I used "0x7FDEADBEEF" deliberately so that it would be a extremely
unlikely we would have file systems that big (we're approximately 512
PB, and honestly, if we had fixed all of the scaling limits such that
it was sane to think someone would want to be using ext4 with a file
system that big --- well, that would be a very nice problem to have
:-)

> But mainly this value is only going to be used if it is delayed
> extent or a hole which implies that it has not been mapped and
> pblock does not contain anything valid. And if we really screwed it
> up and tried to use pblock of extent which is a hole or delayed
> extent, then it would not help us anyway since the only place that
> we actually set this is when splitting extent on removal.
> 
> Now I can see that in ext4_da_map_blocks() we're actually using ~0
> value for the pblock which is a bit better I think as long as we're
> using this reliably. So I'll resend the patch which will make sure
> that we're using ~0 reliably when storin delayed, or hole extents in
> the extent status tree. Does that make sense ?

So the technique that we're using in mballoc.c is that we use
different illegal flag values depending on where the bad value was
introduced:

% grep "debug value" fs/ext4/mballoc.c
	ex.fe_logical = 0xDEADFA11; /* debug value */
		ex.fe_logical = 0xDEADC0DE; /* debug value */
				ex.fe_logical = 0xDEADF00D; /* debug value */

I think it might be useful to do the same for the physical blocks in
the extent_status tree.

	  				- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lukas Czerner - April 18, 2014, 11:24 a.m.
On Fri, 18 Apr 2014, Theodore Ts'o wrote:

> Date: Fri, 18 Apr 2014 07:18:26 -0400
> From: Theodore Ts'o <tytso@mit.edu>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: linux-ext4@vger.kernel.org
> Subject: Re: [PATCH 1/2] ext4: Remove arbitrary block value in
>     __es_remove_extent()
> 
> On Fri, Apr 18, 2014 at 11:22:12AM +0200, Lukáš Czerner wrote:
> > Aside from the fact that this is totally undocumented and there is
> > not even comment on what is that all about in couple of years we
> > might actually get file systems big enough that this would not be an
> > I/O error anymore (that might be a bit of a stretch).
> 
> Well, do you have some suggestions about what might be a good place to
> document something like this?  My assumption is that it's something
> that would be used by developers after a bug had been reported, so
> presumably it would be someplace in the source code.
> 
> And I used "0x7FDEADBEEF" deliberately so that it would be a extremely
> unlikely we would have file systems that big (we're approximately 512
> PB, and honestly, if we had fixed all of the scaling limits such that
> it was sane to think someone would want to be using ext4 with a file
> system that big --- well, that would be a very nice problem to have
> :-)

Indeed :)

> 
> > But mainly this value is only going to be used if it is delayed
> > extent or a hole which implies that it has not been mapped and
> > pblock does not contain anything valid. And if we really screwed it
> > up and tried to use pblock of extent which is a hole or delayed
> > extent, then it would not help us anyway since the only place that
> > we actually set this is when splitting extent on removal.
> > 
> > Now I can see that in ext4_da_map_blocks() we're actually using ~0
> > value for the pblock which is a bit better I think as long as we're
> > using this reliably. So I'll resend the patch which will make sure
> > that we're using ~0 reliably when storin delayed, or hole extents in
> > the extent status tree. Does that make sense ?
> 
> So the technique that we're using in mballoc.c is that we use
> different illegal flag values depending on where the bad value was
> introduced:
> 
> % grep "debug value" fs/ext4/mballoc.c
> 	ex.fe_logical = 0xDEADFA11; /* debug value */
> 		ex.fe_logical = 0xDEADC0DE; /* debug value */
> 				ex.fe_logical = 0xDEADF00D; /* debug value */
> 
> I think it might be useful to do the same for the physical blocks in
> the extent_status tree.
> 
> 	  				- Ted

I agree. I'll send a patch.

Thanks!
-Lukas

Patch

diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index 33682aa..3c47b4e 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -782,7 +782,7 @@  static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
 	struct extent_status *es;
 	struct extent_status orig_es;
 	ext4_lblk_t len1, len2;
-	ext4_fsblk_t block;
+	ext4_fsblk_t block = 0;
 	int err;
 
 retry:
@@ -810,7 +810,6 @@  retry:
 
 			newes.es_lblk = end + 1;
 			newes.es_len = len2;
-			block = 0x7FDEADBEEF;
 			if (ext4_es_is_written(&orig_es) ||
 			    ext4_es_is_unwritten(&orig_es))
 				block = ext4_es_pblock(&orig_es) +