From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-fsdevel-owner@vger.kernel.org>
Received: from zeniv.linux.org.uk ([195.92.253.2]:44860 "EHLO
        ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1725954AbeJMKSV (ORCPT
        <rfc822;linux-fsdevel@vger.kernel.org>);
        Sat, 13 Oct 2018 06:18:21 -0400
Date: Sat, 13 Oct 2018 03:42:56 +0100
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Viacheslav Dubeyko <slava@dubeyko.com>
Cc: Ernesto =?iso-8859-1?Q?A=2E_Fern=E1ndez?=
        <ernesto.mnd.fernandez@gmail.com>, linux-fsdevel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH 1/2] hfsplus: update timestamps on truncate()
Message-ID: <20181013024256.GI32577@ZenIV.linux.org.uk>
References: <9beb0913eea37288599e8e1b7cec8768fb52d1b8.1539316825.git.ernesto.mnd.fernandez@gmail.com>
 <1539381441.3203.6.camel@slavad-ubuntu-14.04>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1539381441.3203.6.camel@slavad-ubuntu-14.04>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

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?