[PATCH 1/2] hfsplus: update timestamps on truncate()

[PATCH 1/2] hfsplus: update timestamps on truncate()

[Ernesto A. Fernández]

The vfs takes care of updating ctime and mtime on ftruncate(), but on
truncate() it must be done by the module.

This patch can be tested with xfstests generic/313.

Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
---
 fs/hfsplus/inode.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
index 8e9427a42b81..d7ab9d8c4b67 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -261,6 +261,7 @@ static int hfsplus_setattr(struct dentry *dentry, struct iattr *attr)
 		}
 		truncate_setsize(inode, attr->ia_size);
 		hfsplus_file_truncate(inode);
+		inode->i_mtime = inode->i_ctime = current_time(inode);
 	}
 
 	setattr_copy(inode, attr);
-- 
2.11.0

[PATCH 2/2] hfs: update timestamp on truncate()

[Ernesto A. Fernández]

The vfs takes care of updating mtime on ftruncate(), but on truncate()
it must be done by the module.

Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
---
 fs/hfs/inode.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
index a2dfa1b2a89c..da243c84e93b 100644
--- a/fs/hfs/inode.c
+++ b/fs/hfs/inode.c
@@ -642,6 +642,8 @@ int hfs_inode_setattr(struct dentry *dentry, struct iattr * attr)
 
 		truncate_setsize(inode, attr->ia_size);
 		hfs_file_truncate(inode);
+		inode->i_atime = inode->i_mtime = inode->i_ctime =
+						  current_time(inode);
 	}
 
 	setattr_copy(inode, attr);
-- 
2.11.0

Re: [PATCH 2/2] hfs: update timestamp on truncate()

[Viacheslav Dubeyko]

On Fri, 2018-10-12 at 01:23 -0300, Ernesto A. Fernández wrote:
> The vfs takes care of updating mtime on ftruncate(), but on truncate()
> it must be done by the module.
> 
> Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
> ---
>  fs/hfs/inode.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
> index a2dfa1b2a89c..da243c84e93b 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -642,6 +642,8 @@ int hfs_inode_setattr(struct dentry *dentry, struct iattr * attr)
>  
>  		truncate_setsize(inode, attr->ia_size);
>  		hfs_file_truncate(inode);
> +		inode->i_atime = inode->i_mtime = inode->i_ctime =
> +						  current_time(inode);
>  	}
>  
>  	setattr_copy(inode, attr);

Looks good.

Reviewed-by: Vyacheslav Dubeyko <slava@dubeyko.com>

Thanks,
Vyacheslav Dubeyko.

Re: [PATCH 1/2] hfsplus: update timestamps on truncate()

[Viacheslav Dubeyko]

On Fri, 2018-10-12 at 01:22 -0300, Ernesto A. Fernández wrote:
> The vfs takes care of updating ctime and mtime on ftruncate(), but on
> truncate() it must be done by the module.
> 
> This patch can be tested with xfstests generic/313.
> 
> Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
> ---
>  fs/hfsplus/inode.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
> index 8e9427a42b81..d7ab9d8c4b67 100644
> --- a/fs/hfsplus/inode.c
> +++ b/fs/hfsplus/inode.c
> @@ -261,6 +261,7 @@ static int hfsplus_setattr(struct dentry *dentry, struct iattr *attr)
>  		}
>  		truncate_setsize(inode, attr->ia_size);
>  		hfsplus_file_truncate(inode);
> +		inode->i_mtime = inode->i_ctime = current_time(inode);
>  	}
>  
>  	setattr_copy(inode, attr);

Looks good.

Reviewed-by: Vyacheslav Dubeyko <slava@dubeyko.com>

Thanks,
Vyacheslav Dubeyko.

Re: [PATCH 1/2] hfsplus: update timestamps on truncate()

[Al Viro]

On Fri, Oct 12, 2018 at 02:57:21PM -0700, Viacheslav Dubeyko wrote:

> Looks good.
> 
> Reviewed-by: Vyacheslav Dubeyko <slava@dubeyko.com>

Looking at the vicinity of that code has brought something that looks
fishy: suppose we have the file opened and close() races with unlink()
and open()

1) unlink() finds the victim and locks it

2) in hfsplus_file_release():
        if (atomic_dec_and_test(&HFSPLUS_I(inode)->opencnt)) {
got to 0
                inode_lock(inode);
block waiting for unlink

3) open() finds the sucker in dcache and hits hfsplus_file_open(), where
we do
        atomic_inc(&HFSPLUS_I(inode)->opencnt);
and now opencnt is 1.

4) on the unlink side:
        if (inode->i_ino == cnid &&
            atomic_read(&HFSPLUS_I(inode)->opencnt)) {
                str.name = name;
                str.len = sprintf(name, "temp%lu", inode->i_ino);
                res = hfsplus_rename_cat(inode->i_ino,
                                         dir, &dentry->d_name,
                                         sbi->hidden_dir, &str);
                if (!res) {
                        inode->i_flags |= S_DEAD;
                        drop_nlink(inode);
                }
                goto out;
        }
nlink is zero now, the sucker got renamed and marked S_DEAD

5) ->release() finally got through inode_lock() and
                hfsplus_file_truncate(inode);
                if (inode->i_flags & S_DEAD) {
                        hfsplus_delete_cat(inode->i_ino,
                                           HFSPLUS_SB(sb)->hidden_dir, NULL);
                        hfsplus_delete_inode(inode);
                }
                inode_unlock(inode);
... and now we have killed everything we used to have associated with that
inode on disk.  While it's still open.  What's to stop CNID to be reused,
etc. and what's to preserve sanity in that situation?

What am I missing there?

Re: [PATCH 1/2] hfsplus: update timestamps on truncate()

[Viacheslav Dubeyko]

On Sat, 2018-10-13 at 03:42 +0100, Al Viro wrote:
> On Fri, Oct 12, 2018 at 02:57:21PM -0700, Viacheslav Dubeyko wrote:
> 
> > Looks good.
> > 
> > Reviewed-by: Vyacheslav Dubeyko <slava@dubeyko.com>
> 
> Looking at the vicinity of that code has brought something that looks
> fishy: suppose we have the file opened and close() races with unlink()
> and open()
> 
> 1) unlink() finds the victim and locks it
> 
> 2) in hfsplus_file_release():
>         if (atomic_dec_and_test(&HFSPLUS_I(inode)->opencnt)) {
> got to 0
>                 inode_lock(inode);
> block waiting for unlink
> 
> 3) open() finds the sucker in dcache and hits hfsplus_file_open(), where
> we do
>         atomic_inc(&HFSPLUS_I(inode)->opencnt);
> and now opencnt is 1.
> 
> 4) on the unlink side:
>         if (inode->i_ino == cnid &&
>             atomic_read(&HFSPLUS_I(inode)->opencnt)) {
>                 str.name = name;
>                 str.len = sprintf(name, "temp%lu", inode->i_ino);
>                 res = hfsplus_rename_cat(inode->i_ino,
>                                          dir, &dentry->d_name,
>                                          sbi->hidden_dir, &str);
>                 if (!res) {
>                         inode->i_flags |= S_DEAD;
>                         drop_nlink(inode);
>                 }
>                 goto out;
>         }
> nlink is zero now, the sucker got renamed and marked S_DEAD
> 
> 5) ->release() finally got through inode_lock() and
>                 hfsplus_file_truncate(inode);
>                 if (inode->i_flags & S_DEAD) {
>                         hfsplus_delete_cat(inode->i_ino,
>                                            HFSPLUS_SB(sb)->hidden_dir, NULL);
>                         hfsplus_delete_inode(inode);
>                 }
>                 inode_unlock(inode);
> ... and now we have killed everything we used to have associated with that
> inode on disk.  While it's still open.  What's to stop CNID to be reused,
> etc. and what's to preserve sanity in that situation?
> 
> What am I missing there?

OK. If you believe that the patch is in the bad shape then what's your
suggestion for improving the patch?

Thanks,
Vyacheslav Dubeyko.

Re: [PATCH 1/2] hfsplus: update timestamps on truncate()

[Al Viro]

On Mon, Oct 15, 2018 at 02:02:01PM -0700, Viacheslav Dubeyko wrote:
> > 1) unlink() finds the victim and locks it
> > 
> > 2) in hfsplus_file_release():
> >         if (atomic_dec_and_test(&HFSPLUS_I(inode)->opencnt)) {
> > got to 0
> >                 inode_lock(inode);
> > block waiting for unlink
> > 
> > 3) open() finds the sucker in dcache and hits hfsplus_file_open(), where
> > we do
> >         atomic_inc(&HFSPLUS_I(inode)->opencnt);
> > and now opencnt is 1.
> > 
> > 4) on the unlink side:
> >         if (inode->i_ino == cnid &&
> >             atomic_read(&HFSPLUS_I(inode)->opencnt)) {
> >                 str.name = name;
> >                 str.len = sprintf(name, "temp%lu", inode->i_ino);
> >                 res = hfsplus_rename_cat(inode->i_ino,
> >                                          dir, &dentry->d_name,
> >                                          sbi->hidden_dir, &str);
> >                 if (!res) {
> >                         inode->i_flags |= S_DEAD;
> >                         drop_nlink(inode);
> >                 }
> >                 goto out;
> >         }
> > nlink is zero now, the sucker got renamed and marked S_DEAD
> > 
> > 5) ->release() finally got through inode_lock() and
> >                 hfsplus_file_truncate(inode);
> >                 if (inode->i_flags & S_DEAD) {
> >                         hfsplus_delete_cat(inode->i_ino,
> >                                            HFSPLUS_SB(sb)->hidden_dir, NULL);
> >                         hfsplus_delete_inode(inode);
> >                 }
> >                 inode_unlock(inode);
> > ... and now we have killed everything we used to have associated with that
> > inode on disk.  While it's still open.  What's to stop CNID to be reused,
> > etc. and what's to preserve sanity in that situation?
> > 
> > What am I missing there?
> 
> OK. If you believe that the patch is in the bad shape then what's your
> suggestion for improving the patch?

Tha patch is OK; the problem, AFAICS, is neither introduced nor fixed by it.
I might be wrong regarding the locking problem I described, but it does
appear to be real and I'd like somebody more familiar with HFS+ to comment
on it.

Re: [PATCH 1/2] hfsplus: update timestamps on truncate()

[Viacheslav Dubeyko]

On Mon, 2018-10-15 at 22:24 +0100, Al Viro wrote:
> On Mon, Oct 15, 2018 at 02:02:01PM -0700, Viacheslav Dubeyko wrote:
> > > 1) unlink() finds the victim and locks it
> > > 
> > > 2) in hfsplus_file_release():
> > >         if (atomic_dec_and_test(&HFSPLUS_I(inode)->opencnt)) {
> > > got to 0
> > >                 inode_lock(inode);
> > > block waiting for unlink
> > > 
> > > 3) open() finds the sucker in dcache and hits hfsplus_file_open(), where
> > > we do
> > >         atomic_inc(&HFSPLUS_I(inode)->opencnt);
> > > and now opencnt is 1.
> > > 
> > > 4) on the unlink side:
> > >         if (inode->i_ino == cnid &&
> > >             atomic_read(&HFSPLUS_I(inode)->opencnt)) {
> > >                 str.name = name;
> > >                 str.len = sprintf(name, "temp%lu", inode->i_ino);
> > >                 res = hfsplus_rename_cat(inode->i_ino,
> > >                                          dir, &dentry->d_name,
> > >                                          sbi->hidden_dir, &str);
> > >                 if (!res) {
> > >                         inode->i_flags |= S_DEAD;
> > >                         drop_nlink(inode);
> > >                 }
> > >                 goto out;
> > >         }
> > > nlink is zero now, the sucker got renamed and marked S_DEAD
> > > 
> > > 5) ->release() finally got through inode_lock() and
> > >                 hfsplus_file_truncate(inode);
> > >                 if (inode->i_flags & S_DEAD) {
> > >                         hfsplus_delete_cat(inode->i_ino,
> > >                                            HFSPLUS_SB(sb)->hidden_dir, NULL);
> > >                         hfsplus_delete_inode(inode);
> > >                 }
> > >                 inode_unlock(inode);
> > > ... and now we have killed everything we used to have associated with that
> > > inode on disk.  While it's still open.  What's to stop CNID to be reused,
> > > etc. and what's to preserve sanity in that situation?
> > > 
> > > What am I missing there?
> > 
> > OK. If you believe that the patch is in the bad shape then what's your
> > suggestion for improving the patch?
> 
> Tha patch is OK; the problem, AFAICS, is neither introduced nor fixed by it.
> I might be wrong regarding the locking problem I described, but it does
> appear to be real and I'd like somebody more familiar with HFS+ to comment
> on it.

Yes, it looks like a mess. Let me take a deeper look in the code.

Thanks,
Vyacheslav Dubeyko.

Re: [PATCH 1/2] hfsplus: update timestamps on truncate()

[Viacheslav Dubeyko]

On Mon, 2018-10-15 at 22:24 +0100, Al Viro wrote:
> On Mon, Oct 15, 2018 at 02:02:01PM -0700, Viacheslav Dubeyko wrote:
> > > 1) unlink() finds the victim and locks it
> > > 
> > > 2) in hfsplus_file_release():
> > >         if (atomic_dec_and_test(&HFSPLUS_I(inode)->opencnt)) {
> > > got to 0
> > >                 inode_lock(inode);
> > > block waiting for unlink
> > > 
> > > 3) open() finds the sucker in dcache and hits hfsplus_file_open(), where
> > > we do
> > >         atomic_inc(&HFSPLUS_I(inode)->opencnt);
> > > and now opencnt is 1.
> > > 
> > > 4) on the unlink side:
> > >         if (inode->i_ino == cnid &&
> > >             atomic_read(&HFSPLUS_I(inode)->opencnt)) {
> > >                 str.name = name;
> > >                 str.len = sprintf(name, "temp%lu", inode->i_ino);
> > >                 res = hfsplus_rename_cat(inode->i_ino,
> > >                                          dir, &dentry->d_name,
> > >                                          sbi->hidden_dir, &str);
> > >                 if (!res) {
> > >                         inode->i_flags |= S_DEAD;
> > >                         drop_nlink(inode);
> > >                 }
> > >                 goto out;
> > >         }
> > > nlink is zero now, the sucker got renamed and marked S_DEAD
> > > 
> > > 5) ->release() finally got through inode_lock() and
> > >                 hfsplus_file_truncate(inode);
> > >                 if (inode->i_flags & S_DEAD) {
> > >                         hfsplus_delete_cat(inode->i_ino,
> > >                                            HFSPLUS_SB(sb)->hidden_dir, NULL);
> > >                         hfsplus_delete_inode(inode);
> > >                 }
> > >                 inode_unlock(inode);
> > > ... and now we have killed everything we used to have associated with that
> > > inode on disk.  While it's still open.  What's to stop CNID to be reused,
> > > etc. and what's to preserve sanity in that situation?
> > >

The hfsplus_link() increments CNID always [1] and never tries to reuse
the same CNID. HFS+ specification [2] declares that CNID can be to wrap
around and be reused - "HFS Plus volumes now allow CNID values to wrap
around and be reused. The kHFSCatalogNodeIDsReusedBit in the attributes
field of the volume header is set to indicate when CNID values have
wrapped around and been reused. When kHFSCatalogNodeIDsReusedBit is set,
the nextCatalogID field is no longer required to be greater than any
existing CNID. When kHFSCatalogNodeIDsReusedBit is set, nextCatalogID
may still be used as a hint for the CNID to assign to newly created
files or directories, but the implementation must verify that CNID is
not currently in use (and pick another value if it is in use). When CNID
number nextCatalogID is already in use, an implementation could just
increment nextCatalogID until it finds a CNID that is not in use. If
nextCatalogID overflows to zero, kHFSCatalogNodeIDsReusedBit must be set
and nextCatalogID set to kHFSFirstUserCatalogNodeID (to avoid using any
reserved CNID values)". However, HFS+ driver in Linux kernel believes
that u32 type is so huge that never will be exhausted.

I try to show the whole picture:

1) unlink() finds the victim and locks it.

2) in hfsplus_file_release():
         if (atomic_dec_and_test(&HFSPLUS_I(inode)->opencnt)) {
 got to 0
                 inode_lock(inode);
 block waiting for unlink

3) open() finds the sucker in dcache and hits hfsplus_file_open(), where
 we do
         atomic_inc(&HFSPLUS_I(inode)->opencnt);
 and now opencnt is 1.

4) on the unlink side:
if opencnt > 0 then we rename the file by temp<inode_id>, move into the
hidden_dir, mark as S_DEAD and go out. It means that if file is opened
currently then we cannot simply delete the file. The file converted in
the temporary state and the responsibility to delete the file is
delegated to the thread that opened the file during hfsplus_release(). 

5) ->release() finally got through inode_lock()
It looks like that hfsplus_release() has to analyze opencnt more
properly. I suppose that it should look something like this:

    inode_lock(inode);
    mutex_lock(&sbi_vh_mutex);

    if (atomic_dec_and_test(&HFSPLUS_I(inode)->opencnt)) {
         hfsplus_file_truncate(inode);
         if (inode->i_flags & S_DEAD) {
             hfsplus_delete_cat(inode->i_ino,
                                HFSPLUS_SB(sb)->hidden_dir, NULL);
             hfsplus_delete_inode(inode);
         }
    }

    mutex_unlock(&sbi_vh_mutex);
    inode_unlock(inode);

What do you think? Do I miss something?

Thanks,
Vyacheslav Dubeyko.

[1] https://elixir.bootlin.com/linux/latest/source/fs/hfsplus/dir.c#L342
[2] https://developer.apple.com/library/archive/technotes/tn/tn1150.html

Re: [PATCH 1/2] hfsplus: update timestamps on truncate()

[Ernesto A. Fernández]

On Sat, Oct 13, 2018 at 03:42:56AM +0100, Al Viro wrote:
> On Fri, Oct 12, 2018 at 02:57:21PM -0700, Viacheslav Dubeyko wrote:
> 
> > Looks good.
> > 
> > Reviewed-by: Vyacheslav Dubeyko <slava@dubeyko.com>
> 
> Looking at the vicinity of that code has brought something that looks
> fishy: suppose we have the file opened and close() races with unlink()
> and open()
> 
> 1) unlink() finds the victim and locks it
> 
> 2) in hfsplus_file_release():
>         if (atomic_dec_and_test(&HFSPLUS_I(inode)->opencnt)) {
> got to 0
>                 inode_lock(inode);
> block waiting for unlink
> 
> 3) open() finds the sucker in dcache and hits hfsplus_file_open(), where
> we do
>         atomic_inc(&HFSPLUS_I(inode)->opencnt);
> and now opencnt is 1.
> 
> 4) on the unlink side:
>         if (inode->i_ino == cnid &&
>             atomic_read(&HFSPLUS_I(inode)->opencnt)) {
>                 str.name = name;
>                 str.len = sprintf(name, "temp%lu", inode->i_ino);
>                 res = hfsplus_rename_cat(inode->i_ino,
>                                          dir, &dentry->d_name,
>                                          sbi->hidden_dir, &str);
>                 if (!res) {
>                         inode->i_flags |= S_DEAD;
>                         drop_nlink(inode);
>                 }
>                 goto out;
>         }
> nlink is zero now, the sucker got renamed and marked S_DEAD
> 
> 5) ->release() finally got through inode_lock() and
>                 hfsplus_file_truncate(inode);
>                 if (inode->i_flags & S_DEAD) {
>                         hfsplus_delete_cat(inode->i_ino,
>                                            HFSPLUS_SB(sb)->hidden_dir, NULL);
>                         hfsplus_delete_inode(inode);
>                 }
>                 inode_unlock(inode);
> ... and now we have killed everything we used to have associated with that
> inode on disk.  While it's still open.  What's to stop CNID to be reused,
> etc. and what's to preserve sanity in that situation?
> 
> What am I missing there?

Right, that looks like a bug.  Also, the HFS module always deletes open
inodes on ->unlink().

Maybe we could just free the inodes on ->evict_inode(), like most other
file systems?  I guess there must be a reason this wasn't done in the
first place, but I can't figure it out.