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,
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;
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
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,
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
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.
linux-stable-mirror@lists.linaro.org
-
Joel Becker -
Zijun Hu