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

["Ernesto A. Fernández" <ernesto.mnd.fernandez@gmail.com>]

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.