[PATCH 0/4] configfs: fix bugs

[PATCH 0/4] configfs: fix bugs

[Zijun Hu]

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
Zijun Hu (4):
      configfs: Delete semicolon from macro type_print() definition
      configfs: Do not override creating attribute file failure in populate_attrs()
      configfs: Correct error value returned by API config_item_set_name()
      configfs: Correct condition for returning -EEXIST in configfs_symlink()

 fs/configfs/dir.c     | 4 ++--
 fs/configfs/item.c    | 2 +-
 fs/configfs/symlink.c | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)
---
base-commit: 38fec10eb60d687e30c8c6b5420d86e8149f7557
change-id: 20250408-fix_configfs-699743163c64

Best regards,
-- 
Zijun Hu <quic_zijuhu@quicinc.com>

[PATCH 1/4] configfs: Delete semicolon from macro type_print() definition

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

Macro type_print() definition ends with semicolon, so will cause
the subsequent macro invocations end with two semicolons.

Fix by deleting the semicolon from the macro definition.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 fs/configfs/dir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 7d10278db30d667d0ef7e1140e54961c5a440c41..0a011bdad98c492227859ff328d61aeed2071e24 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -970,7 +970,7 @@ static void configfs_dump_one(struct configfs_dirent *sd, int level)
 {
 	pr_info("%*s\"%s\":\n", level, " ", configfs_get_name(sd));
 
-#define type_print(_type) if (sd->s_type & _type) pr_info("%*s %s\n", level, " ", #_type);
+#define type_print(_type) if (sd->s_type & _type) pr_info("%*s %s\n", level, " ", #_type)
 	type_print(CONFIGFS_ROOT);
 	type_print(CONFIGFS_DIR);
 	type_print(CONFIGFS_ITEM_ATTR);

-- 
2.34.1

Re: [PATCH 1/4] configfs: Delete semicolon from macro type_print() definition

[Joel Becker]

On Tue, Apr 08, 2025 at 09:26:07PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> Macro type_print() definition ends with semicolon, so will cause
> the subsequent macro invocations end with two semicolons.
> 
> Fix by deleting the semicolon from the macro definition.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>

Reviewed-by: Joel Becker <jlbec@evilplan.org>

> ---
>  fs/configfs/dir.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index 7d10278db30d667d0ef7e1140e54961c5a440c41..0a011bdad98c492227859ff328d61aeed2071e24 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -970,7 +970,7 @@ static void configfs_dump_one(struct configfs_dirent *sd, int level)
>  {
>  	pr_info("%*s\"%s\":\n", level, " ", configfs_get_name(sd));
>  
> -#define type_print(_type) if (sd->s_type & _type) pr_info("%*s %s\n", level, " ", #_type);
> +#define type_print(_type) if (sd->s_type & _type) pr_info("%*s %s\n", level, " ", #_type)
>  	type_print(CONFIGFS_ROOT);
>  	type_print(CONFIGFS_DIR);
>  	type_print(CONFIGFS_ITEM_ATTR);
> 
> -- 
> 2.34.1
> 

-- 

"There are only two ways to live your life. One is as though nothing
 is a miracle. The other is as though everything is a miracle."
        - Albert Einstein

			http://www.jlbec.org/
			jlbec@evilplan.org

Re: [PATCH 1/4] configfs: Delete semicolon from macro type_print() definition

[Breno Leitao]

Hello Zijun,

On Tue, Apr 08, 2025 at 09:26:07PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> Macro type_print() definition ends with semicolon, so will cause
> the subsequent macro invocations end with two semicolons.

where is the type_print() macro invoked today?

I haven't found any reference for it in fs/configfs/dir.c

Thanks
--breno

Re: [PATCH 1/4] configfs: Delete semicolon from macro type_print() definition

[Zijun Hu]

On 2025/4/17 22:38, Breno Leitao wrote:
> Hello Zijun,
> 
> On Tue, Apr 08, 2025 at 09:26:07PM +0800, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> Macro type_print() definition ends with semicolon, so will cause
>> the subsequent macro invocations end with two semicolons.
> 
> where is the type_print() macro invoked today?
> 
> I haven't found any reference for it in fs/configfs/dir.c
> 

both its definition and invocations are within

fs/configfs/dir.c:
static void configfs_dump_one(struct configfs_dirent *sd, int level)
{
        pr_info("%*s\"%s\":\n", level, " ", configfs_get_name(sd));

#define type_print(_type) if (sd->s_type & _type) pr_info("%*s %s\n",
level, " ", #_type);
        type_print(CONFIGFS_ROOT);
        type_print(CONFIGFS_DIR);
        type_print(CONFIGFS_ITEM_ATTR);
        type_print(CONFIGFS_ITEM_LINK);
        type_print(CONFIGFS_USET_DIR);
        type_print(CONFIGFS_USET_DEFAULT);
        type_print(CONFIGFS_USET_DROPPING);
#undef type_print
}

configfs_dump_one() is invoked by

> Thanks
> --breno

[PATCH 2/4] configfs: Do not override creating attribute file failure in populate_attrs()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

populate_attrs() may override failure for creating attribute files
by success for creating subsequent bin attribute files, and have
wrong return value.

Fix by creating bin attribute files under successfully creating
attribute files.

Fixes: 03607ace807b ("configfs: implement binary attributes")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 fs/configfs/dir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 0a011bdad98c492227859ff328d61aeed2071e24..64272d3946cc40757dca063190829958517eceb3 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -619,7 +619,7 @@ static int populate_attrs(struct config_item *item)
 				break;
 		}
 	}
-	if (t->ct_bin_attrs) {
+	if (!error && t->ct_bin_attrs) {
 		for (i = 0; (bin_attr = t->ct_bin_attrs[i]) != NULL; i++) {
 			if (ops && ops->is_bin_visible && !ops->is_bin_visible(item, bin_attr, i))
 				continue;

-- 
2.34.1

Re: [PATCH 2/4] configfs: Do not override creating attribute file failure in populate_attrs()

[Joel Becker]

On Tue, Apr 08, 2025 at 09:26:08PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> populate_attrs() may override failure for creating attribute files
> by success for creating subsequent bin attribute files, and have
> wrong return value.
> 
> Fix by creating bin attribute files under successfully creating
> attribute files.
> 
> Fixes: 03607ace807b ("configfs: implement binary attributes")
> Cc: stable@vger.kernel.org
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>

Reviewed-by: Joel Becker <jlbec@evilplan.org>

> ---
>  fs/configfs/dir.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index 0a011bdad98c492227859ff328d61aeed2071e24..64272d3946cc40757dca063190829958517eceb3 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -619,7 +619,7 @@ static int populate_attrs(struct config_item *item)
>  				break;
>  		}
>  	}
> -	if (t->ct_bin_attrs) {
> +	if (!error && t->ct_bin_attrs) {
>  		for (i = 0; (bin_attr = t->ct_bin_attrs[i]) != NULL; i++) {
>  			if (ops && ops->is_bin_visible && !ops->is_bin_visible(item, bin_attr, i))
>  				continue;
> 
> -- 
> 2.34.1
> 

-- 

Life's Little Instruction Book #511

	"Call your mother."

			http://www.jlbec.org/
			jlbec@evilplan.org

[PATCH 3/4] configfs: Correct error value returned by API config_item_set_name()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

kvasprintf() failure is often caused by memory allocation which has error
code -ENOMEM, but config_item_set_name() returns -EFAULT for the failure.

Fix by returning -ENOMEM instead of -EFAULT for the failure.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 fs/configfs/item.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/configfs/item.c b/fs/configfs/item.c
index 254170a82aa337d95cbfbdf1a2db1023db3a2907..c378b5cbf87d28387a509c3cabb93eccfb520c9c 100644
--- a/fs/configfs/item.c
+++ b/fs/configfs/item.c
@@ -66,7 +66,7 @@ int config_item_set_name(struct config_item *item, const char *fmt, ...)
 		name = kvasprintf(GFP_KERNEL, fmt, args);
 		va_end(args);
 		if (!name)
-			return -EFAULT;
+			return -ENOMEM;
 	}
 
 	/* Free the old name, if necessary. */

-- 
2.34.1

Re: [PATCH 3/4] configfs: Correct error value returned by API config_item_set_name()

[Joel Becker]

On Tue, Apr 08, 2025 at 09:26:09PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> kvasprintf() failure is often caused by memory allocation which has error
> code -ENOMEM, but config_item_set_name() returns -EFAULT for the failure.
> 
> Fix by returning -ENOMEM instead of -EFAULT for the failure.

The original code would return -ENOMEM on allocation failure before
calling vsnprintf().  The -EFAULT was because vsnprintf() failed.

This changed in 707c62352909a when vsnprintf() was changed to
kvasprintf().  Since kvasprintf() only really fails on -ENOMEM, the
error code should have been updated at the time.


> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>

Reviewed-by: Joel Becker <jlbec@evilplan.org>

> ---
>  fs/configfs/item.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/configfs/item.c b/fs/configfs/item.c
> index 254170a82aa337d95cbfbdf1a2db1023db3a2907..c378b5cbf87d28387a509c3cabb93eccfb520c9c 100644
> --- a/fs/configfs/item.c
> +++ b/fs/configfs/item.c
> @@ -66,7 +66,7 @@ int config_item_set_name(struct config_item *item, const char *fmt, ...)
>  		name = kvasprintf(GFP_KERNEL, fmt, args);
>  		va_end(args);
>  		if (!name)
> -			return -EFAULT;
> +			return -ENOMEM;
>  	}
>  
>  	/* Free the old name, if necessary. */
> 
> -- 
> 2.34.1
> 

-- 

Life's Little Instruction Book #274

	"Leave everything a little better than you found it."

			http://www.jlbec.org/
			jlbec@evilplan.org

[PATCH 4/4] configfs: Correct condition for returning -EEXIST in configfs_symlink()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

configfs_symlink() returns -EEXIST under condition d_unhashed(), but the
condition often means the dentry does not exist.

Fix by changing the condition to !d_unhashed().

Fixes: 351e5d869e5a ("configfs: fix a deadlock in configfs_symlink()")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 fs/configfs/symlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
index 69133ec1fac2a854241c2a08a3b48c4c2e8d5c24..cccf61fb8317d739643834e1810b7f136058f56c 100644
--- a/fs/configfs/symlink.c
+++ b/fs/configfs/symlink.c
@@ -193,7 +193,7 @@ int configfs_symlink(struct mnt_idmap *idmap, struct inode *dir,
 	if (ret)
 		goto out_put;
 
-	if (dentry->d_inode || d_unhashed(dentry))
+	if (dentry->d_inode || !d_unhashed(dentry))
 		ret = -EEXIST;
 	else
 		ret = inode_permission(&nop_mnt_idmap, dir,

-- 
2.34.1

Re: [PATCH 4/4] configfs: Correct condition for returning -EEXIST in configfs_symlink()

[Joel Becker]

On Tue, Apr 08, 2025 at 09:26:10PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> configfs_symlink() returns -EEXIST under condition d_unhashed(), but the
> condition often means the dentry does not exist.
> 
> Fix by changing the condition to !d_unhashed().

I don't think this is quite right.

viro put this together in 351e5d869e5ac, which was a while ago.  Read
his comment on 351e5d869e5ac.  Because I unlock the parent directory to
look up the target, we can't trust our symlink dentry hasn't been
changed underneath us.

* If there is now dentry->d_inode, some other inode has been put here.
  -EEXIST.
* If the dentry was unhashed, somehow the dentry we are creating was
  removed from the dcache, and adding things to our dentry will at best
  go nowhere, and at worst dangle in space.  I'm pretty sure viro
  returns -EEXIST because if this dentry is unhashed, some *other*
  dentry has entered the dcache in its place (another file type,
  perhaps).

If you instead check for !d_unhashed(), you're discovering our candidate
dentry is still live in the dcache, which is what we expect and want.

How did you identify this as a problem?  Perhaps we need a more nuanced
check than d_unhashed() these days (for example, d_is_positive/negative
didn't exist back then).

Thanks,
Joel

PS: I enjoyed the trip down memory lane to Al reaming me quite
    thoroughly for this API.

> 
> Fixes: 351e5d869e5a ("configfs: fix a deadlock in configfs_symlink()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  fs/configfs/symlink.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
> index 69133ec1fac2a854241c2a08a3b48c4c2e8d5c24..cccf61fb8317d739643834e1810b7f136058f56c 100644
> --- a/fs/configfs/symlink.c
> +++ b/fs/configfs/symlink.c
> @@ -193,7 +193,7 @@ int configfs_symlink(struct mnt_idmap *idmap, struct inode *dir,
>  	if (ret)
>  		goto out_put;
>  
> -	if (dentry->d_inode || d_unhashed(dentry))
> +	if (dentry->d_inode || !d_unhashed(dentry))
>  		ret = -EEXIST;
>  	else
>  		ret = inode_permission(&nop_mnt_idmap, dir,
> 
> -- 
> 2.34.1
> 

-- 

"We will have to repent in this generation not merely for the
 vitriolic words and actions of the bad people, but for the 
 appalling silence of the good people."
	- Rev. Dr. Martin Luther King, Jr.

			http://www.jlbec.org/
			jlbec@evilplan.org

Re: [PATCH 4/4] configfs: Correct condition for returning -EEXIST in configfs_symlink()

[Zijun Hu]

On 2025/4/9 06:49, Joel Becker wrote:
>> configfs_symlink() returns -EEXIST under condition d_unhashed(), but the
>> condition often means the dentry does not exist.
>>
>> Fix by changing the condition to !d_unhashed().
> I don't think this is quite right.
> 

agree.

> viro put this together in 351e5d869e5ac, which was a while ago.  Read
> his comment on 351e5d869e5ac.  Because I unlock the parent directory to
> look up the target, we can't trust our symlink dentry hasn't been
> changed underneath us.
> 
> * If there is now dentry->d_inode, some other inode has been put here.
>   -EEXIST.
> * If the dentry was unhashed, somehow the dentry we are creating was
>   removed from the dcache, and adding things to our dentry will at best
>   go nowhere, and at worst dangle in space.  I'm pretty sure viro
>   returns -EEXIST because if this dentry is unhashed, some *other*
>   dentry has entered the dcache in its place (another file type,
>   perhaps).
> 
> If you instead check for !d_unhashed(), you're discovering our candidate
> dentry is still live in the dcache, which is what we expect and want.
> 
> How did you identify this as a problem?  Perhaps we need a more nuanced

for current condition to return -EEXIST, if hit d_unhashed(dentry), that
means that "if ((dentry->d_inode == NULL) && d_unhashed(dentry)) return
-EEXIST" which looks weird and not right as well.

> check than d_unhashed() these days (for example, d_is_positive/negative
> didn't exist back then).
> 

any suggestions about how to correct the condition to return -EEXIST ?

> Thanks,
> Joel
> 
> PS: I enjoyed the trip down memory lane to Al reaming me quite
>     thoroughly for this API.