All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] libext2fs: Only link an inode into a directory once
@ 2012-01-07  4:47 Darrick J. Wong
  2012-02-15 19:25 ` Ted Ts'o
  0 siblings, 1 reply; 5+ messages in thread
From: Darrick J. Wong @ 2012-01-07  4:47 UTC (permalink / raw)
  To: Theodore Tso, Darrick J. Wong; +Cc: linux-ext4

The ext2fs_link helper function link_proc does not check the value of ls->done,
which means that if the function finds multiple empty spaces that will fit the
new directory entry, it will create a directory entry in each of the spaces.
Instead of doing that, check the done value and don't do anything more if we've
already added the directory entry.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---
 lib/ext2fs/link.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)


diff --git a/lib/ext2fs/link.c b/lib/ext2fs/link.c
index 2d03b57..2f4f54a 100644
--- a/lib/ext2fs/link.c
+++ b/lib/ext2fs/link.c
@@ -42,6 +42,9 @@ static int link_proc(struct ext2_dir_entry *dirent,
 	unsigned int rec_len, min_rec_len, curr_rec_len;
 	int ret = 0;
 
+	if (ls->done)
+		return 0;
+
 	rec_len = EXT2_DIR_REC_LEN(ls->namelen);
 
 	ls->err = ext2fs_get_rec_len(ls->fs, dirent, &curr_rec_len);


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH] libext2fs: Only link an inode into a directory once
  2012-01-07  4:47 [PATCH] libext2fs: Only link an inode into a directory once Darrick J. Wong
@ 2012-02-15 19:25 ` Ted Ts'o
  2012-02-15 20:40   ` Darrick J. Wong
  0 siblings, 1 reply; 5+ messages in thread
From: Ted Ts'o @ 2012-02-15 19:25 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-ext4

On Fri, Jan 06, 2012 at 08:47:43PM -0800, Darrick J. Wong wrote:
> The ext2fs_link helper function link_proc does not check the value of ls->done,
> which means that if the function finds multiple empty spaces that will fit the
> new directory entry, it will create a directory entry in each of the spaces.
> Instead of doing that, check the done value and don't do anything more if we've
> already added the directory entry.
> 
> Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>

This should't necessary, since when we insert the directory entry, we
return with the DIRENT_ABORT bit set:

        dirent->inode = ls->inode;
        dirent->name_len = ls->namelen;
        strncpy(dirent->name, ls->name, ls->namelen);
        if (ls->sb->s_feature_incompat & EXT2_FEATURE_INCOMPAT_FILETYPE)
                dirent->name_len |= (ls->flags & 0x7) << 8;

        ls->done++;
        return DIRENT_ABORT|DIRENT_CHANGED;

Did you actually observe this happening?

Thanks,

					- Ted

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] libext2fs: Only link an inode into a directory once
  2012-02-15 19:25 ` Ted Ts'o
@ 2012-02-15 20:40   ` Darrick J. Wong
  2012-02-15 22:23     ` Ted Ts'o
  0 siblings, 1 reply; 5+ messages in thread
From: Darrick J. Wong @ 2012-02-15 20:40 UTC (permalink / raw)
  To: Ted Ts'o; +Cc: linux-ext4

On Wed, Feb 15, 2012 at 02:25:37PM -0500, Ted Ts'o wrote:
> On Fri, Jan 06, 2012 at 08:47:43PM -0800, Darrick J. Wong wrote:
> > The ext2fs_link helper function link_proc does not check the value of ls->done,
> > which means that if the function finds multiple empty spaces that will fit the
> > new directory entry, it will create a directory entry in each of the spaces.
> > Instead of doing that, check the done value and don't do anything more if we've
> > already added the directory entry.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
> 
> This should't necessary, since when we insert the directory entry, we
> return with the DIRENT_ABORT bit set:
> 
>         dirent->inode = ls->inode;
>         dirent->name_len = ls->namelen;
>         strncpy(dirent->name, ls->name, ls->namelen);
>         if (ls->sb->s_feature_incompat & EXT2_FEATURE_INCOMPAT_FILETYPE)
>                 dirent->name_len |= (ls->flags & 0x7) << 8;
> 
>         ls->done++;
>         return DIRENT_ABORT|DIRENT_CHANGED;
> 
> Did you actually observe this happening?

Yes.  I took a look at the block iteration code again, and I'm wondering, do we
need one of these:

if (ret & BLOCK_ABORT)
	break;

...around lib/ext2fs/block.c, line 450?  This is the code blob:

if (!(extent.e_flags & EXT2_EXTENT_FLAGS_LEAF)) {
        if (ctx.flags & BLOCK_FLAG_DATA_ONLY)
                continue;
        if ((!(extent.e_flags &
               EXT2_EXTENT_FLAGS_SECOND_VISIT) &&
             !(ctx.flags & BLOCK_FLAG_DEPTH_TRAVERSE)) ||
            ((extent.e_flags &
              EXT2_EXTENT_FLAGS_SECOND_VISIT) &&
             (ctx.flags & BLOCK_FLAG_DEPTH_TRAVERSE))) {
                ret |= (*ctx.func)(fs, &blk,
                                   -1, 0, 0, priv_data);
                if (ret & BLOCK_CHANGED) {
                        extent.e_pblk = blk;
                        ctx.errcode =
        ext2fs_extent_replace(handle, 0, &extent);
                        if (ctx.errcode)
                                break;
                }
		/* INSERT HERE? */
        }
        continue;
}

It looks to me that when ctx.func returns BLOCK_ABORT, the continue causes
the code to loop around and get the next extent, which isn't what we want.

--D


^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] libext2fs: Only link an inode into a directory once
  2012-02-15 20:40   ` Darrick J. Wong
@ 2012-02-15 22:23     ` Ted Ts'o
  2012-02-15 22:27       ` [PATCH] libext2fs: fix BLOCK_ABORT handling in the block iterator for extents Theodore Ts'o
  0 siblings, 1 reply; 5+ messages in thread
From: Ted Ts'o @ 2012-02-15 22:23 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-ext4

On Wed, Feb 15, 2012 at 12:40:15PM -0800, Darrick J. Wong wrote:
> 
> Yes.  I took a look at the block iteration code again, and I'm
> wondering, do we need one of these:
> 
> if (ret & BLOCK_ABORT)
> 	break;
> 
> ...around lib/ext2fs/block.c, line 450?

Good spotting; it turns out there was another problem later on, where the

	if (ret & BLOCK_ABORT)
		break;

does the wrong thing, since we need to break out of both the for loop
*and* the while loop.

I'll send a patch which I believe will fix BLOCK_ABORT in the block
iterator for the extent case.

						- Ted

^ permalink raw reply	[flat|nested] 5+ messages in thread
* [PATCH] libext2fs: fix BLOCK_ABORT handling in the block iterator for extents
  2012-02-15 22:23     ` Ted Ts'o
@ 2012-02-15 22:27       ` Theodore Ts'o
  0 siblings, 0 replies; 5+ messages in thread
From: Theodore Ts'o @ 2012-02-15 22:27 UTC (permalink / raw)
  To: Ext4 Developers List; +Cc: djwong, Theodore Ts'o

When processing files that contain extents, the block iterator
functions were not properly handling the BLOCK_ABORT bit.  This could
cause problems such as ext2fs_link() adding a directory entry multiple
times.

Thanks to Darrick Wong <djwong@us.ibm.com> for reporting this.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 lib/ext2fs/block.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/lib/ext2fs/block.c b/lib/ext2fs/block.c
index 6a5c10d..85a1803 100644
--- a/lib/ext2fs/block.c
+++ b/lib/ext2fs/block.c
@@ -414,7 +414,7 @@ errcode_t ext2fs_block_iterate3(ext2_filsys fs,
 						0, 0, priv_data);
 				ret |= r;
 				check_for_ro_violation_goto(&ctx, ret,
-							    extent_errout);
+							    extent_done);
 				if (r & BLOCK_CHANGED) {
 					ctx.errcode =
 						ext2fs_extent_set_bmap(handle,
@@ -448,6 +448,8 @@ errcode_t ext2fs_block_iterate3(ext2_filsys fs,
 						if (ctx.errcode)
 							break;
 					}
+					if (ret & BLOCK_ABORT)
+						break;
 				}
 				continue;
 			}
@@ -473,23 +475,23 @@ errcode_t ext2fs_block_iterate3(ext2_filsys fs,
 						0, 0, priv_data);
 				ret |= r;
 				check_for_ro_violation_goto(&ctx, ret,
-							    extent_errout);
+							    extent_done);
 				if (r & BLOCK_CHANGED) {
 					ctx.errcode =
 						ext2fs_extent_set_bmap(handle,
 						       (blk64_t) blockcnt,
 						       new_blk, uninit);
 					if (ctx.errcode)
-						goto extent_errout;
+						goto extent_done;
 				}
 				if (ret & BLOCK_ABORT)
-					break;
+					goto extent_done;
 			}
 		}
 
-	extent_errout:
+	extent_done:
 		ext2fs_extent_free(handle);
-		ret |= BLOCK_ERROR | BLOCK_ABORT;
+		ret |= BLOCK_ERROR; /* ctx.errcode is always valid here */
 		goto errout;
 	}
 
-- 
1.7.9.107.g97f9a


^ permalink raw reply related	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-02-15 22:27 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-07  4:47 [PATCH] libext2fs: Only link an inode into a directory once Darrick J. Wong
2012-02-15 19:25 ` Ted Ts'o
2012-02-15 20:40   ` Darrick J. Wong
2012-02-15 22:23     ` Ted Ts'o
2012-02-15 22:27       ` [PATCH] libext2fs: fix BLOCK_ABORT handling in the block iterator for extents Theodore Ts'o

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.