[RESEND PATCH v2] hfsplus: prevent negative dentries when casefolded

[RESEND PATCH v2] hfsplus: prevent negative dentries when casefolded

[Chung-Chiang Cheng]

hfsplus uses the case-insensitive filenames by default, but VFS negative
dentries are incompatible with case-insensitive. For example, the
following instructions will get a cached filename 'aaa' which isn't
expected. There is no such problem in macOS.

  touch aaa
  rm aaa
  touch AAA

This patch takes the same approach to drop negative dentires as vfat does.
The dentry is revalidated without blocking and storing to the dentry,
and should be safe in rcu-walk.

Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com>
---
 fs/hfsplus/hfsplus_fs.h |  1 +
 fs/hfsplus/inode.c      |  1 +
 fs/hfsplus/unicode.c    | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 34 insertions(+)

diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index 1798949f269b..4ae7f1ca1584 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -520,6 +520,7 @@ int hfsplus_asc2uni(struct super_block *sb, struct hfsplus_unistr *ustr,
 int hfsplus_hash_dentry(const struct dentry *dentry, struct qstr *str);
 int hfsplus_compare_dentry(const struct dentry *dentry, unsigned int len,
 			   const char *str, const struct qstr *name);
+int hfsplus_revalidate_dentry(struct dentry *dentry, unsigned int flags);
 
 /* wrapper.c */
 int hfsplus_submit_bio(struct super_block *sb, sector_t sector, void *buf,
diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
index 6fef67c2a9f0..4188a0760118 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -179,6 +179,7 @@ const struct address_space_operations hfsplus_aops = {
 const struct dentry_operations hfsplus_dentry_operations = {
 	.d_hash       = hfsplus_hash_dentry,
 	.d_compare    = hfsplus_compare_dentry,
+	.d_revalidate = hfsplus_revalidate_dentry,
 };
 
 static void hfsplus_get_perms(struct inode *inode,
diff --git a/fs/hfsplus/unicode.c b/fs/hfsplus/unicode.c
index 73342c925a4b..e336631334eb 100644
--- a/fs/hfsplus/unicode.c
+++ b/fs/hfsplus/unicode.c
@@ -10,6 +10,7 @@
  */
 
 #include <linux/types.h>
+#include <linux/namei.h>
 #include <linux/nls.h>
 #include "hfsplus_fs.h"
 #include "hfsplus_raw.h"
@@ -518,3 +519,34 @@ int hfsplus_compare_dentry(const struct dentry *dentry,
 		return 1;
 	return 0;
 }
+
+int hfsplus_revalidate_dentry(struct dentry *dentry, unsigned int flags)
+{
+	/*
+	 * 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

Re: [RESEND PATCH v2] hfsplus: prevent negative dentries when casefolded

[Viacheslav Dubeyko]

> On Jul 16, 2021, at 12:36 AM, Chung-Chiang Cheng <cccheng@synology.com> wrote:
> 
> hfsplus uses the case-insensitive filenames by default, but VFS negative
> dentries are incompatible with case-insensitive. For example, the
> following instructions will get a cached filename 'aaa' which isn't
> expected. There is no such problem in macOS.
> 
>  touch aaa
>  rm aaa
>  touch AAA
> 
> This patch takes the same approach to drop negative dentires as vfat does.
> The dentry is revalidated without blocking and storing to the dentry,
> and should be safe in rcu-walk.
> 
> Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com>
> ---
> fs/hfsplus/hfsplus_fs.h |  1 +
> fs/hfsplus/inode.c      |  1 +
> fs/hfsplus/unicode.c    | 32 ++++++++++++++++++++++++++++++++
> 3 files changed, 34 insertions(+)
> 
> diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
> index 1798949f269b..4ae7f1ca1584 100644
> --- a/fs/hfsplus/hfsplus_fs.h
> +++ b/fs/hfsplus/hfsplus_fs.h
> @@ -520,6 +520,7 @@ int hfsplus_asc2uni(struct super_block *sb, struct hfsplus_unistr *ustr,
> int hfsplus_hash_dentry(const struct dentry *dentry, struct qstr *str);
> int hfsplus_compare_dentry(const struct dentry *dentry, unsigned int len,
> 			   const char *str, const struct qstr *name);
> +int hfsplus_revalidate_dentry(struct dentry *dentry, unsigned int flags);
> 
> /* wrapper.c */
> int hfsplus_submit_bio(struct super_block *sb, sector_t sector, void *buf,
> diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
> index 6fef67c2a9f0..4188a0760118 100644
> --- a/fs/hfsplus/inode.c
> +++ b/fs/hfsplus/inode.c
> @@ -179,6 +179,7 @@ const struct address_space_operations hfsplus_aops = {
> const struct dentry_operations hfsplus_dentry_operations = {
> 	.d_hash       = hfsplus_hash_dentry,
> 	.d_compare    = hfsplus_compare_dentry,
> +	.d_revalidate = hfsplus_revalidate_dentry,
> };
> 
> static void hfsplus_get_perms(struct inode *inode,
> diff --git a/fs/hfsplus/unicode.c b/fs/hfsplus/unicode.c
> index 73342c925a4b..e336631334eb 100644
> --- a/fs/hfsplus/unicode.c
> +++ b/fs/hfsplus/unicode.c
> @@ -10,6 +10,7 @@
>  */
> 
> #include <linux/types.h>
> +#include <linux/namei.h>
> #include <linux/nls.h>
> #include "hfsplus_fs.h"
> #include "hfsplus_raw.h"
> @@ -518,3 +519,34 @@ int hfsplus_compare_dentry(const struct dentry *dentry,
> 		return 1;
> 	return 0;
> }
> +
> +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
> 

Re: [RESEND PATCH v2] hfsplus: prevent negative dentries when casefolded

[Chung-Chiang Cheng]

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

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
>>

Re: [RESEND PATCH v2] hfsplus: prevent negative dentries when casefolded

[Viacheslav Dubeyko]

> 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
>>>