Message-Id: <1170263470.12392.23.camel@kleikamp.austin.ibm.com>
Date:	Wed, 31 Jan 2007 11:11:10 -0600
From:	Dave Kleikamp <shaggy@...ux.vnet.ibm.com>
To:	Harry Papaxenopoulos <harry@...sunysb.edu>
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

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.

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.

> @@ -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.

> +#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:.

> +		}
> +	}
> +#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.
-- 
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