From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-fsdevel-owner@vger.kernel.org>
Received: from mail-qk1-f195.google.com ([209.85.222.195]:42953 "EHLO
        mail-qk1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1726936AbeJQHH4 (ORCPT
        <rfc822;linux-fsdevel@vger.kernel.org>);
        Wed, 17 Oct 2018 03:07:56 -0400
Received: by mail-qk1-f195.google.com with SMTP id u20-v6so734587qkk.9
        for <linux-fsdevel@vger.kernel.org>; Tue, 16 Oct 2018 16:15:15 -0700 (PDT)
Date: Tue, 16 Oct 2018 20:15:09 -0300
From: Ernesto =?utf-8?Q?A=2E_Fern=C3=A1ndez?=
        <ernesto.mnd.fernandez@gmail.com>
To: Al Viro <viro@ZenIV.linux.org.uk>
Cc: Viacheslav Dubeyko <slava@dubeyko.com>,
        linux-fsdevel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH 1/2] hfsplus: update timestamps on truncate()
Message-ID: <20181016231509.dftjjdqqrdm3boy4@eaf>
References: <9beb0913eea37288599e8e1b7cec8768fb52d1b8.1539316825.git.ernesto.mnd.fernandez@gmail.com>
 <1539381441.3203.6.camel@slavad-ubuntu-14.04>
 <20181013024256.GI32577@ZenIV.linux.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20181013024256.GI32577@ZenIV.linux.org.uk>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

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.