Re: [Resubmit][PATCH 5/5] Secure Deletion and Trash-Bin Support for Ext4
On Thu, Feb 01, 2007 at 08:22:28PM -0600, Dave Kleikamp wrote: Al Viro will love that one. :-) Another idea might be to store those attribute flags in the generic inode. Yeah, that's really the right answer. Having a common way of storing attributes in struct kstat (and notified via struct iattr and notify_change) is the right way for us to store some of these filesystem-common flags, and would make a huge amount of sense. I don't really view ths secure deletion and trash-bin support as really being an ext4 feature, but really a generic feature which can enhance multiple filesystems *including* ext4. So it would be really good idea to give these patches an airing on linux-fsdevel and linux-kernel. Regards, - Ted - To unsubscribe from this list: send the line unsubscribe linux-ext4 in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
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 [EMAIL PROTECTED]
Signed-off-by: Nikolai Joukov [EMAIL PROTECTED]
Signed-off-by: Erez Zadok [EMAIL PROTECTED]
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:.
Re: [Resubmit][PATCH 5/5] Secure Deletion and Trash-Bin Support for Ext4
On Thu, 2007-02-01 at 06:05 -0500, Harry Papaxenopoulos wrote: On Wed, 31 Jan 2007, Dave Kleikamp wrote: 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. the rename already updates the change time. 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. I'd rather do the allocation after the new code anyway. It's not needed at all if we're moving the file to the trashbin. Thanks, Shaggy -- David Kleikamp IBM Linux Technology Center - To unsubscribe from this list: send the line unsubscribe linux-ext4 in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
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. Thank you! 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. Well, I guess lost+found has the same problem but it is not a problem at all to pick some other (longer) name. 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. Unfortunately, we need some file system-specific code to access per-file secure deletion and per-file trash bit attributes. These attributes are supported only by some file systems and in different ways. We need no file system-specific code to support trash-bin deletion for whole file systems. Nikolai. - To unsubscribe from this list: send the line unsubscribe linux-ext4 in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Resubmit][PATCH 5/5] Secure Deletion and Trash-Bin Support for Ext4
On Thu, 2007-02-01 at 12:17 -0500, Nikolai Joukov wrote: 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. Well, I guess lost+found has the same problem but it is not a problem at all to pick some other (longer) name. Right, I didn't see it as a show-stopper, just something to consider. 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. Unfortunately, we need some file system-specific code to access per-file secure deletion and per-file trash bit attributes. These attributes are supported only by some file systems and in different ways. Yeah, I did see that. I wonder adding some inode or file operation just to query the existence of those attributes (or something more generic) would be too ugly. We need no file system-specific code to support trash-bin deletion for whole file systems. Nikolai. -- David Kleikamp IBM Linux Technology Center - To unsubscribe from this list: send the line unsubscribe linux-ext4 in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Resubmit][PATCH 5/5] Secure Deletion and Trash-Bin Support for Ext4
On Feb 01, 2007 12:41 -0800, Mingming Cao wrote: On Thu, 2007-02-01 at 19:32 +, Dave Kleikamp wrote: Unfortunately, we need some file system-specific code to access per-file secure deletion and per-file trash bit attributes. These attributes are supported only by some file systems and in different ways. The check for fs specific attributes has to be underlying fs code. But the code the handling the secure delete and trash bin (although now is only two functions being called) are identical for all fs, could be move to VFS layer. Yeah, I did see that. I wonder adding some inode or file operation just to query the existence of those attributes (or something more generic) would be too ugly. I gave a brief thought on that yesterday, it was not very pretty:) Actually, the major filesystems (ext3, reiserfs, jfs, xfs) all use the same lsattr/chattr ioctl as ext2 (EXT2_IOC_GETFLAGS). Maybe this code can just do an ioctl inside the kernel? Cheers, Andreas -- Andreas Dilger Principal Software Engineer Cluster File Systems, Inc. - To unsubscribe from this list: send the line unsubscribe linux-ext4 in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Resubmit][PATCH 5/5] Secure Deletion and Trash-Bin Support for Ext4
On Thu, 2007-02-01 at 18:40 -0700, Andreas Dilger wrote: On Feb 01, 2007 12:41 -0800, Mingming Cao wrote: On Thu, 2007-02-01 at 19:32 +, Dave Kleikamp wrote: Yeah, I did see that. I wonder adding some inode or file operation just to query the existence of those attributes (or something more generic) would be too ugly. I gave a brief thought on that yesterday, it was not very pretty:) Actually, the major filesystems (ext3, reiserfs, jfs, xfs) all use the same lsattr/chattr ioctl as ext2 (EXT2_IOC_GETFLAGS). Maybe this code can just do an ioctl inside the kernel? Al Viro will love that one. :-) Another idea might be to store those attribute flags in the generic inode. Shaggy -- David Kleikamp IBM Linux Technology Center - To unsubscribe from this list: send the line unsubscribe linux-ext4 in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
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 [EMAIL PROTECTED]
Signed-off-by: Nikolai Joukov [EMAIL PROTECTED]
Signed-off-by: Erez Zadok [EMAIL PROTECTED]
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))) {
