[PATCH, RFC] directory offset overflows in 2.6.28

[PATCH, RFC] directory offset overflows in 2.6.28

[Christoph Hellwig]

John Stanley reported EOVERFLOW errors in readdir from his self-build
glibc.  I traced this down to glibc enabling d_off overflow checks
in one of the about five million different getdents implementations.

In 2.6.28 Dave Woodhouse moved our readdir double buffering required
for NFS4 readdirplus into nfsd and at that point we lost the capping
of the directory offsets to 32 bit signed values.  Johns glibc used
getdents64 to even implement readdir for normal 32 bit offset dirents,
and failed with EOVERFLOW only if this happens on the first dirent in
a getdents call.  I managed to come up with a testcase that uses
raw getdents and does the EOVERFLOW check manually.  We always hit
it with our last entry due to the special end of directory marker.

The patch below is a dumb version of just putting back the masking,
to make sure we have the same behavior as in 2.6.27 and earlier.
I think we should at least hide it in a macro that is well-commented,
but I suspect we also need to make sure that we never ever get bigger
offsets in directories in some way.


Index: xfs/fs/xfs/xfs_dir2_block.c
===================================================================
--- xfs.orig/fs/xfs/xfs_dir2_block.c	2008-12-29 21:25:29.680613664 +0100
+++ xfs/fs/xfs/xfs_dir2_block.c	2008-12-29 21:29:57.341627581 +0100
@@ -517,9 +517,9 @@ xfs_dir2_block_getdents(
 		/*
 		 * If it didn't fit, set the final offset to here & return.
 		 */
-		if (filldir(dirent, dep->name, dep->namelen, cook,
+		if (filldir(dirent, dep->name, dep->namelen, cook & 0x7fffffff,
 			    ino, DT_UNKNOWN)) {
-			*offset = cook;
+			*offset = cook & 0x7fffffff;
 			xfs_da_brelse(NULL, bp);
 			return 0;
 		}
@@ -529,7 +529,8 @@ xfs_dir2_block_getdents(
 	 * Reached the end of the block.
 	 * Set the offset to a non-existent block 1 and return.
 	 */
-	*offset = xfs_dir2_db_off_to_dataptr(mp, mp->m_dirdatablk + 1, 0);
+	*offset = xfs_dir2_db_off_to_dataptr(mp, mp->m_dirdatablk + 1, 0) &
+			0x7fffffff;
 	xfs_da_brelse(NULL, bp);
 	return 0;
 }
Index: xfs/fs/xfs/xfs_dir2_leaf.c
===================================================================
--- xfs.orig/fs/xfs/xfs_dir2_leaf.c	2008-12-29 21:25:13.899613482 +0100
+++ xfs/fs/xfs/xfs_dir2_leaf.c	2008-12-29 21:29:36.125616996 +0100
@@ -1092,7 +1092,7 @@ xfs_dir2_leaf_getdents(
 		 * Won't fit.  Return to caller.
 		 */
 		if (filldir(dirent, dep->name, dep->namelen,
-			    xfs_dir2_byte_to_dataptr(mp, curoff),
+			    xfs_dir2_byte_to_dataptr(mp, curoff) & 0x7fffffff,
 			    ino, DT_UNKNOWN))
 			break;
 
@@ -1108,9 +1108,9 @@ xfs_dir2_leaf_getdents(
 	 * All done.  Set output offset value to current offset.
 	 */
 	if (curoff > xfs_dir2_dataptr_to_byte(mp, XFS_DIR2_MAX_DATAPTR))
-		*offset = XFS_DIR2_MAX_DATAPTR;
+		*offset = XFS_DIR2_MAX_DATAPTR & 0x7fffffff;
 	else
-		*offset = xfs_dir2_byte_to_dataptr(mp, curoff);
+		*offset = xfs_dir2_byte_to_dataptr(mp, curoff) & 0x7fffffff;
 	kmem_free(map);
 	if (bp)
 		xfs_da_brelse(NULL, bp);
Index: xfs/fs/xfs/xfs_dir2_sf.c
===================================================================
--- xfs.orig/fs/xfs/xfs_dir2_sf.c	2008-12-29 21:23:55.174613820 +0100
+++ xfs/fs/xfs/xfs_dir2_sf.c	2008-12-29 21:29:00.721617384 +0100
@@ -752,8 +752,8 @@ xfs_dir2_sf_getdents(
 #if XFS_BIG_INUMS
 		ino += mp->m_inoadd;
 #endif
-		if (filldir(dirent, ".", 1, dot_offset, ino, DT_DIR)) {
-			*offset = dot_offset;
+		if (filldir(dirent, ".", 1, dot_offset & 0x7fffffff, ino, DT_DIR)) {
+			*offset = dot_offset & 0x7fffffff;
 			return 0;
 		}
 	}
@@ -766,8 +766,8 @@ xfs_dir2_sf_getdents(
 #if XFS_BIG_INUMS
 		ino += mp->m_inoadd;
 #endif
-		if (filldir(dirent, "..", 2, dotdot_offset, ino, DT_DIR)) {
-			*offset = dotdot_offset;
+		if (filldir(dirent, "..", 2, dotdot_offset & 0x7fffffff, ino, DT_DIR)) {
+			*offset = dotdot_offset & 0x7fffffff;
 			return 0;
 		}
 	}
@@ -791,14 +791,15 @@ xfs_dir2_sf_getdents(
 #endif
 
 		if (filldir(dirent, sfep->name, sfep->namelen,
-					    off, ino, DT_UNKNOWN)) {
-			*offset = off;
+			    off & 0x7fffffff, ino, DT_UNKNOWN)) {
+			*offset = off & 0x7fffffff;
 			return 0;
 		}
 		sfep = xfs_dir2_sf_nextentry(sfp, sfep);
 	}
 
-	*offset = xfs_dir2_db_off_to_dataptr(mp, mp->m_dirdatablk + 1, 0);
+	*offset = xfs_dir2_db_off_to_dataptr(mp, mp->m_dirdatablk + 1, 0) &
+			0x7fffffff;
 	return 0;
 }
 

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

Re: [PATCH, RFC] directory offset overflows in 2.6.28

[Dave Chinner]

On Mon, Dec 29, 2008 at 05:07:45PM -0500, Christoph Hellwig wrote:
> The patch below is a dumb version of just putting back the masking,
> to make sure we have the same behavior as in 2.6.27 and earlier.
> I think we should at least hide it in a macro that is well-commented,
> but I suspect we also need to make sure that we never ever get bigger
> offsets in directories in some way.

I think we need that macro sooner rather than later ;)

> Index: xfs/fs/xfs/xfs_dir2_block.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_dir2_block.c	2008-12-29 21:25:29.680613664 +0100
> +++ xfs/fs/xfs/xfs_dir2_block.c	2008-12-29 21:29:57.341627581 +0100
> @@ -517,9 +517,9 @@ xfs_dir2_block_getdents(
>  		/*
>  		 * If it didn't fit, set the final offset to here & return.
>  		 */
> -		if (filldir(dirent, dep->name, dep->namelen, cook,
> +		if (filldir(dirent, dep->name, dep->namelen, cook & 0x7fffffff,
>  			    ino, DT_UNKNOWN)) {
> -			*offset = cook;
> +			*offset = cook & 0x7fffffff;
>  			xfs_da_brelse(NULL, bp);
>  			return 0;
>  		}

In this case, you can do the masking at the time cook is
assigned. I haven't checked, but I suspect the rest will be the
same. That will make the patch less invasive and with a macro
somewhat cleaner...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH, RFC] directory offset overflows in 2.6.28

[Russell Cattelan]

Dave Chinner wrote:
> On Mon, Dec 29, 2008 at 05:07:45PM -0500, Christoph Hellwig wrote:
>   
>> The patch below is a dumb version of just putting back the masking,
>> to make sure we have the same behavior as in 2.6.27 and earlier.
>> I think we should at least hide it in a macro that is well-commented,
>> but I suspect we also need to make sure that we never ever get bigger
>> offsets in directories in some way.
>>     
>
> I think we need that macro sooner rather than later ;)
>
>   
>> Index: xfs/fs/xfs/xfs_dir2_block.c
>> ===================================================================
>> --- xfs.orig/fs/xfs/xfs_dir2_block.c	2008-12-29 21:25:29.680613664 +0100
>> +++ xfs/fs/xfs/xfs_dir2_block.c	2008-12-29 21:29:57.341627581 +0100
>> @@ -517,9 +517,9 @@ xfs_dir2_block_getdents(
>>  		/*
>>  		 * If it didn't fit, set the final offset to here & return.
>>  		 */
>> -		if (filldir(dirent, dep->name, dep->namelen, cook,
>> +		if (filldir(dirent, dep->name, dep->namelen, cook & 0x7fffffff,
>>  			    ino, DT_UNKNOWN)) {
>> -			*offset = cook;
>> +			*offset = cook & 0x7fffffff;
>>  			xfs_da_brelse(NULL, bp);
>>  			return 0;
>>  		}
>>     
>
> In this case, you can do the masking at the time cook is
> assigned. I haven't checked, but I suspect the rest will be the
> same. That will make the patch less invasive and with a macro
> somewhat cleaner...
>
> Cheers,
>
> Dave.
>   
I'm still sorting out the readdir changes in the FreeBSD port so I'm not 
exactly sure how this would
affect things there but I'm thinking FreeBSD does not have 32 bit 
readdir offset limits, so this change
might be another undo patch I would have to maintain.
So I agree, lets make this less invasive.

I've managed to change things at bit by passing an opaque ptr  
containing the  uuio struct into a freebsd "filldir" function  so my xfs 
changes so far
are minimal.


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

Re: [PATCH, RFC] directory offset overflows in 2.6.28

[Christoph Hellwig]

On Tue, Dec 30, 2008 at 11:11:17AM +1100, Dave Chinner wrote:
> On Mon, Dec 29, 2008 at 05:07:45PM -0500, Christoph Hellwig wrote:
> > The patch below is a dumb version of just putting back the masking,
> > to make sure we have the same behavior as in 2.6.27 and earlier.
> > I think we should at least hide it in a macro that is well-commented,
> > but I suspect we also need to make sure that we never ever get bigger
> > offsets in directories in some way.
> 
> I think we need that macro sooner rather than later ;)

> In this case, you can do the masking at the time cook is
> assigned. I haven't checked, but I suspect the rest will be the
> same. That will make the patch less invasive and with a macro
> somewhat cleaner...

That way we could replace two assignment by one one time each
in xfs_dir2_leaf.c and xfs_dir2_block.c.

I started working on the macro, but it seems even more hacky.

When looking at the big picture we have two problems:

 - the end of directory marker which always seems to be always too
   large for 32bit values for 32 bit indices
 - directories that actually are too large to be represented using
   32 bit signed offsets.  I guess we just can't support those
   for apps using the 32bit readdir interface and the EOVERFLOW
   is valid for those.  The current approach as in < 2.6.28 and
   with this patch breaks that second case in subtile ways.

So, I'd just push the first hacky opencoded patch into 2.6.29 and
-stable now to revert to the old behaviour with all it's faults, and
in the meantime I'll look into a proper way finding a better end of
directory indicator.  That should also help Russell's BSD concerns.

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

Re: [PATCH, RFC] directory offset overflows in 2.6.28

[Dave Chinner]

On Tue, Dec 30, 2008 at 10:57:26AM -0500, Christoph Hellwig wrote:
> On Tue, Dec 30, 2008 at 11:11:17AM +1100, Dave Chinner wrote:
> > On Mon, Dec 29, 2008 at 05:07:45PM -0500, Christoph Hellwig wrote:
> > > The patch below is a dumb version of just putting back the masking,
> > > to make sure we have the same behavior as in 2.6.27 and earlier.
> > > I think we should at least hide it in a macro that is well-commented,
> > > but I suspect we also need to make sure that we never ever get bigger
> > > offsets in directories in some way.
> > 
> > I think we need that macro sooner rather than later ;)
> 
> > In this case, you can do the masking at the time cook is
> > assigned. I haven't checked, but I suspect the rest will be the
> > same. That will make the patch less invasive and with a macro
> > somewhat cleaner...
> 
> That way we could replace two assignment by one one time each
> in xfs_dir2_leaf.c and xfs_dir2_block.c.
> 
> I started working on the macro, but it seems even more hacky.
> 
> When looking at the big picture we have two problems:
> 
>  - the end of directory marker which always seems to be always too
>    large for 32bit values for 32 bit indices
>  - directories that actually are too large to be represented using
>    32 bit signed offsets.  I guess we just can't support those
>    for apps using the 32bit readdir interface and the EOVERFLOW
>    is valid for those.  The current approach as in < 2.6.28 and
>    with this patch breaks that second case in subtile ways.
> 
> So, I'd just push the first hacky opencoded patch into 2.6.29 and
> -stable now to revert to the old behaviour with all it's faults, and
> in the meantime I'll look into a proper way finding a better end of
> directory indicator.  That should also help Russell's BSD concerns.

Ok, sounds like a plan.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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