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