From: Viacheslav Dubeyko <slava@dubeyko.com>
To: Chung-Chiang Cheng <cccheng@synology.com>
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
Subject: Re: [RESEND PATCH v2] hfsplus: prevent negative dentries when casefolded
Date: Mon, 19 Jul 2021 12:11:47 -0700 [thread overview]
Message-ID: <E4E0A1E4-52A9-45D0-A179-B5289641FE22@dubeyko.com> (raw)
In-Reply-To: <a2c84cfa-b6ed-c86c-0bb1-d05087c141d7@synology.com>
> On Jul 19, 2021, at 2:03 AM, Chung-Chiang Cheng <cccheng@synology.com> wrote:
>
> 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.
>
> 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
>
>
> [1] https://www.kernel.org/doc/Documentation/filesystems/vfs.txt
>
I am still not convinced by the explanation.
>> This patch takes the same approach to drop negative dentires as vfat does.
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) ‘aaa’ -> ‘AAA’, (2) ‘AAA’ -> ‘aaa’, (3) ‘aaa’ -> ‘aaa’, (4) ‘AAA’ -> ‘AAA’. 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
>
>>> +
>>> +int hfsplus_revalidate_dentry(struct dentry *dentry, unsigned int flags)
>>> +{
>> What’s about this code?
>>
>> If (flags & LOOKUP_RCU)
>> return -ECHILD;
>>
>> Do we really need to miss it here?
>>
>> Thanks,
>> Slava.
>>
>>
>>> + /*
>>> + * 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;
>>> +}
>>> --
>>> 2.25.1
>>>
prev parent reply other threads:[~2021-07-19 19:44 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-16 7:36 [RESEND PATCH v2] hfsplus: prevent negative dentries when casefolded Chung-Chiang Cheng
2021-07-16 19:30 ` Viacheslav Dubeyko
2021-07-19 9:03 ` Chung-Chiang Cheng
2021-07-19 19:11 ` Viacheslav Dubeyko [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=E4E0A1E4-52A9-45D0-A179-B5289641FE22@dubeyko.com \
--to=slava@dubeyko.com \
--cc=akpm@linux-foundation.org \
--cc=cccheng@synology.com \
--cc=gregkh@linuxfoundation.org \
--cc=gustavoars@kernel.org \
--cc=keescook@chromium.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mszeredi@redhat.com \
--cc=shepjeng@gmail.com \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).