From: Miklos Szeredi <[email protected]>
hpfs_unlink() calls notify_change() to truncate the file before
deleting. Replace with explicit call to hpfs_notify_change().
This is equivalent, except that:
- security_inode_setattr() is not called before hpfs_notify_change()
- fsnotify_change() is not called after hpfs_notify_change()
The truncation is just an implementation detail, so both the security
check and the notification are unnecessary.
Possibly even the ctime modification is wrong?
Signed-off-by: Miklos Szeredi <[email protected]>
CC: Mikulas Patocka <[email protected]>
---
fs/hpfs/namei.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Index: linux-2.6/fs/hpfs/namei.c
===================================================================
--- linux-2.6.orig/fs/hpfs/namei.c 2008-07-23 00:10:13.000000000 +0200
+++ linux-2.6/fs/hpfs/namei.c 2008-07-23 00:10:22.000000000 +0200
@@ -426,7 +426,8 @@ again:
/*printk("HPFS: truncating file before delete.\n");*/
newattrs.ia_size = 0;
newattrs.ia_valid = ATTR_SIZE | ATTR_CTIME;
- err = notify_change(dentry, &newattrs);
+ newattrs.ia_ctime = current_fs_time(inode->i_sb);
+ err = hpfs_notify_change(dentry, &newattrs);
put_write_access(inode);
if (!err)
goto again;
--
What about down_write on i_alloc_sem --- notify change takes that and your
patch bypasses that.
Mikulas
> From: Miklos Szeredi <[email protected]>
>
> hpfs_unlink() calls notify_change() to truncate the file before
> deleting. Replace with explicit call to hpfs_notify_change().
>
> This is equivalent, except that:
> - security_inode_setattr() is not called before hpfs_notify_change()
> - fsnotify_change() is not called after hpfs_notify_change()
>
> The truncation is just an implementation detail, so both the security
> check and the notification are unnecessary.
>
> Possibly even the ctime modification is wrong?
>
> Signed-off-by: Miklos Szeredi <[email protected]>
> CC: Mikulas Patocka <[email protected]>
> ---
> fs/hpfs/namei.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> Index: linux-2.6/fs/hpfs/namei.c
> ===================================================================
> --- linux-2.6.orig/fs/hpfs/namei.c 2008-07-23 00:10:13.000000000 +0200
> +++ linux-2.6/fs/hpfs/namei.c 2008-07-23 00:10:22.000000000 +0200
> @@ -426,7 +426,8 @@ again:
> /*printk("HPFS: truncating file before delete.\n");*/
> newattrs.ia_size = 0;
> newattrs.ia_valid = ATTR_SIZE | ATTR_CTIME;
> - err = notify_change(dentry, &newattrs);
> + newattrs.ia_ctime = current_fs_time(inode->i_sb);
> + err = hpfs_notify_change(dentry, &newattrs);
> put_write_access(inode);
> if (!err)
> goto again;
>
> --
>
On Wed, 23 Jul 2008, Mikulas Patocka wrote:
> What about down_write on i_alloc_sem --- notify change takes that and your
> patch bypasses that.
i_alloc_sem is only relevant if filesystem uses blockdev_direct_IO()
and friends. It is not used for anything else, so for hpfs there's no
need to surround take that lock.
Thanks,
Miklos
> > From: Miklos Szeredi <[email protected]>
> >
> > hpfs_unlink() calls notify_change() to truncate the file before
> > deleting. Replace with explicit call to hpfs_notify_change().
> >
> > This is equivalent, except that:
> > - security_inode_setattr() is not called before hpfs_notify_change()
> > - fsnotify_change() is not called after hpfs_notify_change()
> >
> > The truncation is just an implementation detail, so both the security
> > check and the notification are unnecessary.
> >
> > Possibly even the ctime modification is wrong?
> >
> > Signed-off-by: Miklos Szeredi <[email protected]>
> > CC: Mikulas Patocka <[email protected]>
> > ---
> > fs/hpfs/namei.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > Index: linux-2.6/fs/hpfs/namei.c
> > ===================================================================
> > --- linux-2.6.orig/fs/hpfs/namei.c 2008-07-23 00:10:13.000000000 +0200
> > +++ linux-2.6/fs/hpfs/namei.c 2008-07-23 00:10:22.000000000 +0200
> > @@ -426,7 +426,8 @@ again:
> > /*printk("HPFS: truncating file before delete.\n");*/
> > newattrs.ia_size = 0;
> > newattrs.ia_valid = ATTR_SIZE | ATTR_CTIME;
> > - err = notify_change(dentry, &newattrs);
> > + newattrs.ia_ctime = current_fs_time(inode->i_sb);
> > + err = hpfs_notify_change(dentry, &newattrs);
> > put_write_access(inode);
> > if (!err)
> > goto again;
> >
> > --
> >
>
On Wed, 23 Jul 2008, Miklos Szeredi wrote:
> On Wed, 23 Jul 2008, Mikulas Patocka wrote:
>
> > > From: Miklos Szeredi <[email protected]>
> > >
> > > hpfs_unlink() calls notify_change() to truncate the file before
> > > deleting. Replace with explicit call to hpfs_notify_change().
> > >
> > > This is equivalent, except that:
> > > - security_inode_setattr() is not called before hpfs_notify_change()
> > > - fsnotify_change() is not called after hpfs_notify_change()
> > >
> > > The truncation is just an implementation detail, so both the security
> > > check and the notification are unnecessary.
> > >
> > > Possibly even the ctime modification is wrong?
> > >
> > > Signed-off-by: Miklos Szeredi <[email protected]>
> > > CC: Mikulas Patocka <[email protected]>
> > > ---
> > > fs/hpfs/namei.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > Index: linux-2.6/fs/hpfs/namei.c
> > > ===================================================================
> > > --- linux-2.6.orig/fs/hpfs/namei.c 2008-07-23 00:10:13.000000000 +0200
> > > +++ linux-2.6/fs/hpfs/namei.c 2008-07-23 00:10:22.000000000 +0200
> > > @@ -426,7 +426,8 @@ again:
> > > /*printk("HPFS: truncating file before delete.\n");*/
> > > newattrs.ia_size = 0;
> > > newattrs.ia_valid = ATTR_SIZE | ATTR_CTIME;
> > > - err = notify_change(dentry, &newattrs);
> > > + newattrs.ia_ctime = current_fs_time(inode->i_sb);
> > > + err = hpfs_notify_change(dentry, &newattrs);
> > > put_write_access(inode);
> > > if (!err)
> > > goto again;
> > >
> > > --
> > What about down_write on i_alloc_sem --- notify change takes that and your
> > patch bypasses that.
>
> i_alloc_sem is only relevant if filesystem uses blockdev_direct_IO()
> and friends. It is not used for anything else, so for hpfs there's no
> need to surround take that lock.
>
> Thanks,
> Miklos
>
I don't care much about this part anyway. This piece of code is not
supposed to work reliably (what if the file has zero size or has fragments
less then 2kb?), and if you change it to make it work more reliably under
a circumstance when someone is deleting file he doesn't own, so what.
This is just design flaw of hpfs.
If you are certain about correctness of your patch, change it (but test
it --- I placed a test partition at
http://artax.karlin.mff.cuni.cz/~mikulas/vyplody/hpfs/test-hpfs-partition.gz).
Or, the alternative is to remove the truncate code at all.
Mikulas