Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.100.0.2.22\))
Subject: Re: [RESEND PATCH v2] hfsplus: prevent negative dentries when
 casefolded
From:   Viacheslav Dubeyko <slava@dubeyko.com>
In-Reply-To: <a2c84cfa-b6ed-c86c-0bb1-d05087c141d7@synology.com>
Date:   Mon, 19 Jul 2021 12:11:47 -0700
Cc:     Linux FS Devel <linux-fsdevel@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Al Viro <viro@zeniv.linux.org.uk>,
        Andrew Morton <akpm@linux-foundation.org>,
        gustavoars@kernel.org, gregkh@linuxfoundation.org,
        keescook@chromium.org, mszeredi@redhat.com, shepjeng@gmail.com
Content-Transfer-Encoding: quoted-printable
Message-Id: <E4E0A1E4-52A9-45D0-A179-B5289641FE22@dubeyko.com>
References: <20210716073635.1613671-1-cccheng@synology.com>
 <02B9566C-A78E-42FB-924B-A503E4BC6D2F@dubeyko.com>
 <a2c84cfa-b6ed-c86c-0bb1-d05087c141d7@synology.com>
To:     Chung-Chiang Cheng <cccheng@synology.com>
Precedence: bulk


> On Jul 19, 2021, at 2:03 AM, Chung-Chiang Cheng <cccheng@synology.com> =
wrote:
>=20
> This function revalidates dentries without blocking and storing to the
> dentry. As the document mentioned [1], I think it's safe in rcu-walk
> mode. I also found jfs_ci_revalidate() takes the same approach.
>=20
>         d_revalidate may be called in rcu-walk mode (flags & =
LOOKUP_RCU).
>         If in rcu-walk mode, the filesystem must revalidate the dentry =
without
>         blocking or storing to the dentry, d_parent and d_inode should =
not be
>         used without care (because they can change and, in d_inode =
case, even
>         become NULL under us
>=20
>=20
> [1] https://www.kernel.org/doc/Documentation/filesystems/vfs.txt
>=20


I am still not convinced by the explanation.

>> This patch takes the same approach to drop negative dentires as vfat =
does.=20

You mentioned that you follows by vfat approach. But this code contains =
this code, as far as I can see. How could you prove that we will not =
introduce some weird bug here? What if code of this function will be =
changed in the future? I suppose that missing of this code could be the =
way to introduce some bug, anyway.

>> touch aaa
>> rm aaa
>> touch AAA

By the way, have you tested other possible combinations? I mean (1) =
=E2=80=98aaa=E2=80=99 -> =E2=80=98AAA=E2=80=99, (2) =E2=80=98AAA=E2=80=99 =
-> =E2=80=98aaa=E2=80=99, (3) =E2=80=98aaa=E2=80=99 -> =E2=80=98aaa=E2=80=99=
, (4) =E2=80=98AAA=E2=80=99 -> =E2=80=98AAA=E2=80=99. Could you please =
add in the comment that it was tested? Could we create the file in =
case-insensitive mode and, then, try to delete in case-sensitive and =
vise versa? Do we define this flag during volume creation? Can we change =
the flag by volume tuning?

Thanks,
Slava.


> Thanks,
> C.C.Cheng
>=20
>>> +
>>> +int hfsplus_revalidate_dentry(struct dentry *dentry, unsigned int =
flags)
>>> +{
>> What=E2=80=99s about this code?
>>=20
>> If (flags & LOOKUP_RCU)
>>    return -ECHILD;
>>=20
>> Do we really need to miss it here?
>>=20
>> Thanks,
>> Slava.
>>=20
>>=20
>>> +	/*
>>> +	 * dentries are always valid when disabling casefold.
>>> +	 */
>>> +	if (!test_bit(HFSPLUS_SB_CASEFOLD, =
&HFSPLUS_SB(dentry->d_sb)->flags))
>>> +		return 1;
>>> +
>>> +	/*
>>> +	 * Positive dentries are valid when enabling casefold.
>>> +	 *
>>> +	 * Note, rename() to existing directory entry will have =
->d_inode, and
>>> +	 * will use existing name which isn't specified name by user.
>>> +	 *
>>> +	 * We may be able to drop this positive dentry here. But =
dropping
>>> +	 * positive dentry isn't good idea. So it's unsupported like
>>> +	 * rename("filename", "FILENAME") for now.
>>> +	 */
>>> +	if (d_really_is_positive(dentry))
>>> +		return 1;
>>> +
>>> +	/*
>>> +	 * Drop the negative dentry, in order to make sure to use the =
case
>>> +	 * sensitive name which is specified by user if this is for =
creation.
>>> +	 */
>>> +	if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET))
>>> +		return 0;
>>> +
>>> +	return 1;
>>> +}
>>> --=20
>>> 2.25.1
>>>=20