Message-ID: <Pine.GSO.4.53.0701311951100.4753@compserv1>
Date:	Thu, 1 Feb 2007 06:05:07 -0500 (EST)
From:	Harry Papaxenopoulos <harry@...sunysb.edu>
To:	Dave Kleikamp <shaggy@...ux.vnet.ibm.com>
cc:	linux-ext4@...r.kernel.org, ezk@...sunysb.edu, kolya@...sunysb.edu
Subject: Re: [Resubmit][PATCH 5/5] Secure Deletion and Trash-Bin Support for
 Ext4



On Wed, 31 Jan 2007, Dave Kleikamp wrote:

> Right now I've only really looked at the jfs patch, since that's what
> I'm the most familiar with.  I'll try to take a look at the rest of them
> later.
>
> I don't have a strong opinion for or against the function and your
> design.  The only potential problem I see in the approach is that
> the .trash directory may conflict with some other use of the same name.
> Since this is primarily vfs function, you'll probably get a wider
> audience on linux-fsdevel.
>
> Have you considered putting ALL of the function in the vfs layer?  It
> looks like this could be done without touching any code in the
> individual file systems.
>

Yes now that you mention it we probably could. The initial idea was to add
this functionality only for Ext4. Only after a suggestion did we move most
of the code to the VFS and have hooks to it through the underlying
file-systems. Nevertheless it probably could be completely moved to the VFS.

> On Wed, 2007-01-31 at 09:55 -0500, Harry Papaxenopoulos wrote:
> > Trash-Bin Functionality for the jfs filesystem:
> >
> > Signed-off-by: Harry Papaxenopoulos <harry@...sunysb.edu>
> > Signed-off-by: Nikolai Joukov <kolya@...sunysb.edu>
> > Signed-off-by: Erez Zadok <ezk@...sunysb.edu>
> >
> > Index: sdfs/src/linux-2.6.20-rc6-trashbin/fs/jfs/super.c
> > ===================================================================
> > --- sdfs.orig/src/linux-2.6.20-rc6-trashbin/fs/jfs/super.c
> > +++ sdfs/src/linux-2.6.20-rc6-trashbin/fs/jfs/super.c
>
> In the future, please restructure the patches so that the linux
> directory is the first component of the path.  It is standard that
> kernel patches can be applied from the top directory with patch -p1.
>

Ok noted. Sorry.

> > @@ -29,6 +29,7 @@
> >  #include <linux/buffer_head.h>
> >  #include <asm/uaccess.h>
> >  #include <linux/seq_file.h>
> > +#include <linux/trashbin.h>
> >
> >  #include "jfs_incore.h"
> >  #include "jfs_filsys.h"
> > @@ -503,6 +504,11 @@ static int jfs_fill_super(struct super_b
> >  	if (sbi->mntflag & JFS_OS2)
> >  		sb->s_root->d_op = &jfs_ci_dentry_operations;
> >
> > +#ifdef CONFIG_JFS_FS_TRASHBIN
> > +	if ((sb->s_flags & MNT_TRASHBIN) && vfs_create_trash_bin(sb))
> > +		goto out_no_root;
>
> This error path leaks sb->s_root.  I think you need to dput(sb->s_root)
> here.

Yes you're right. Will change.

>
> > +#endif
> > +
> >  	/* logical blocks are represented by 40 bits in pxd_t, etc. */
> >  	sb->s_maxbytes = ((u64) sb->s_blocksize) << 40;
> >  #if BITS_PER_LONG == 32
> > Index: sdfs/src/linux-2.6.20-rc6-trashbin/fs/jfs/namei.c
> > ===================================================================
> > --- sdfs.orig/src/linux-2.6.20-rc6-trashbin/fs/jfs/namei.c
> > +++ sdfs/src/linux-2.6.20-rc6-trashbin/fs/jfs/namei.c
> > @@ -20,6 +20,8 @@
> >  #include <linux/fs.h>
> >  #include <linux/ctype.h>
> >  #include <linux/quotaops.h>
> > +#include <linux/trashbin.h>
> > +#include <linux/mount.h>
> >  #include "jfs_incore.h"
> >  #include "jfs_superblock.h"
> >  #include "jfs_inode.h"
> > @@ -474,6 +476,11 @@ static int jfs_unlink(struct inode *dip,
> >  	struct tblock *tblk;
> >  	s64 new_size = 0;
> >  	int commit_flag;
> > +	int trashed = 0;
> > +#ifdef CONFIG_JFS_FS_TRASHBIN
> > +	unsigned int flags = 0;
> > +	struct dentry *user_dentry = NULL;
> > +#endif
> >
> >  	jfs_info("jfs_unlink: dip:0x%p name:%s", dip, dentry->d_name.name);
> >
> > @@ -483,6 +490,35 @@ static int jfs_unlink(struct inode *dip,
> >  	if ((rc = get_UCSname(&dname, dentry)))
> >  		goto out;
> >
> > +#ifdef CONFIG_JFS_FS_TRASHBIN
> > +	flags = JFS_IP(ip)->mode2 & JFS_FL_USER_VISIBLE;
> > +	if ((dentry->d_inode->i_sb->s_flags & MNT_TRASHBIN) && (
> > +				flags & (JFS_UNRM_FL | JFS_SECRM_FL))) {
> > +
> > +		/*
> > +		 * We put this code here to optimize the common case. Since
> > +		 * lookups are expensive, we try to reserve from making any,
> > +		 * unless one of the trash-bin flags are set. The cleanest
> > +		 * way though is to probably move this code outside the
> > +		 * above if statement.
> > +		 */
> > +		user_dentry = vfs_get_user_dentry(dip, 1);
> > +		if (IS_ERR(user_dentry)) {
> > +			rc = PTR_ERR(user_dentry);
> > +			user_dentry = NULL;
> > +			goto out;
> > +		}
> > +
> > +		if (ip->i_nlink == 1 && user_dentry->d_inode &&
> > +			   	user_dentry->d_inode->i_ino != dip->i_ino) {
> > +			rc = vfs_trash_entry(dip, dentry);
>
> can we dput(user_dentry) here rather than after out: ?
>
> > +			trashed = 1;
> > +			if (rc)
> > +				goto out;
>
> why not unconditionally goto out here?  if vfs_trash_entry() was
> successful, the file was successfully moved to the trashbin directory.
> There is nothing left to be done, right?  Then there's no need for the
> trashed flag.
>
> In fact, the ifdef'ed code should precede the call to get_UCSname(),
> since we don't need to allocate dname if we move the file to the
> trashbin, and we leak the allocation if we jump to out:.
>

Well the main reason I don't jump to out is to update the inode's
change time, otherwise I would have unconditionally jumped.

You're right, I used the incorrect label to jump. Should have been "out1"
instead of "out" so the allocation is freed. Sorry about that.

> > +		}
> > +	}
> > +#endif
> > +
> >  	IWRITE_LOCK(ip);
> >
> >  	tid = txBegin(dip->i_sb, 0);
> > @@ -497,7 +533,7 @@ static int jfs_unlink(struct inode *dip,
> >  	 * delete the entry of target file from parent directory
> >  	 */
> >  	ino = ip->i_ino;
> > -	if ((rc = dtDelete(tid, dip, &dname, &ino, JFS_REMOVE))) {
> > +	if (!trashed && (rc = dtDelete(tid, dip, &dname, &ino, JFS_REMOVE))) {
> >  		jfs_err("jfs_unlink: dtDelete returned %d", rc);
> >  		if (rc == -EIO)
> >  			txAbort(tid, 1);	/* Marks FS Dirty */
> > @@ -514,7 +550,8 @@ static int jfs_unlink(struct inode *dip,
> >  	mark_inode_dirty(dip);
> >
> >  	/* update target's inode */
> > -	inode_dec_link_count(ip);
> > +	if (!trashed)
> > +		inode_dec_link_count(ip);
> >
> >  	/*
> >  	 *      commit zero link count object
> > @@ -590,6 +627,10 @@ static int jfs_unlink(struct inode *dip,
> >  	free_UCSname(&dname);
> >        out:
> >  	jfs_info("jfs_unlink: rc:%d", rc);
> > +#ifdef CONFIG_JFS_FS_TRASHBIN
> > +	if (user_dentry)
> > +		dput(user_dentry);
> > +#endif
> >  	return rc;
> >  }
> >
> > Index: sdfs/src/linux-2.6.20-rc6-trashbin/fs/Kconfig
> > ===================================================================
> > --- sdfs.orig/src/linux-2.6.20-rc6-trashbin/fs/Kconfig
> > +++ sdfs/src/linux-2.6.20-rc6-trashbin/fs/Kconfig
> > @@ -443,6 +443,15 @@ config JFS_STATISTICS
> >  	  Enabling this option will cause statistics from the JFS file system
> >  	  to be made available to the user in the /proc/fs/jfs/ directory.
> >
> > +config JFS_FS_TRASHBIN
> > +	bool "JFS trashbin functionality"
> > +	depends on TRASHBIN
> > +	depends on JFS_FS
> > +	help
> > +	  Trashbin functionality for the JFS filesystem
> > +
> > +	  If unsure, say N.
> > +
> >  config FS_POSIX_ACL
> >  # Posix ACL utility routines (for now, only ext2/ext3/jfs/reiserfs)
> >  #
>
> What about the rename() path?  A file can be unlinked by mv'ing a file
> over another.

Good point. Will definitely add it.

> --
> David Kleikamp
> IBM Linux Technology Center
>
>
-
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists