* [RFC PATCH 0/1] vfs: pass S_IFDIR mode to vfs_prepare_mode()
@ 2026-06-11 14:57 Jori Koolstra
2026-06-11 14:57 ` [RFC PATCH 1/1] " Jori Koolstra
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Jori Koolstra @ 2026-06-11 14:57 UTC (permalink / raw)
To: Christian Brauner, Jan Kara, Steve French, Steve French, Al Viro,
NeilBrown, Jeff Layton
Cc: linux-cifs, linux-fsdevel, linux-kernel, jkoolstra
There is a comment in vfs_prepare_mode() that says:
Note that it's currently valid for @type to be 0 if a directory is
created. Filesystems raise that flag individually and we need to check
whether each filesystem can deal with receiving S_IFDIR from the vfs
before we enforce a non-zero type.
This is a bit challenging since there are many filesystems. Claude Opus
4.8 was used to generate the context for each mkdir implementation from
which it can be judged whether passing S_IFDIR is safe. The result was
then verified by hand by looking at how the mode argument is used in
each case. To check whether all mkdir implementations are convered,
'rg "\.mkdir" ' was used and checked against the list of uses Claude
found.
The only mkdir implementation that I am not as sure of is
cifs_mkdir(). So this goes cc to the cifs people. But of course, I might
have made errors in other places (except probably not for the A list as
those are all just mode | S_IFDIR cases without other uses of mode
besides logging)
This is the AI context I worked with:
Here's each filesystem with the exact line(s) that make it safe, grouped by why.
A. Forces S_IFDIR itself (redundant if pre-set → harmless)
9p fs/9p/vfs_inode.c:683 perm = unixmode2p9mode(v9ses, mode | S_IFDIR);
9p (dotl) fs/9p/vfs_inode_dotl.c:365 omode |= S_IFDIR;
affs fs/affs/namei.c:290 inode->i_mode = S_IFDIR | mode;
afs fs/afs/dir.c:1326 op->create.mode = S_IFDIR | mode;
autofs fs/autofs/root.c:744 inode = autofs_get_inode(dir->i_sb, S_IFDIR | mode);
btrfs fs/btrfs/inode.c:7123 inode_init_owner(idmap, inode, dir, S_IFDIR | mode);
ceph fs/ceph/dir.c:1145 mode |= S_IFDIR;
ext2 fs/ext2/namei.c:239 inode = ext2_new_inode(dir, S_IFDIR | mode, &dentry->d_name);
ext4 fs/ext4/namei.c:3012 inode = ext4_new_inode_start_handle(idmap, dir, S_IFDIR | mode,
f2fs fs/f2fs/namei.c:745 inode = f2fs_new_inode(idmap, dir, S_IFDIR | mode, NULL);
fuse fs/fuse/dir.c:1131 return create_new_entry(idmap, fm, &args, dir, entry, S_IFDIR);
gfs2 fs/gfs2/inode.c:1351 gfs2_create_inode(dir, dentry, NULL, S_IFDIR | mode, ...)
hfs fs/hfs/dir.c:228 inode = hfs_new_inode(dir, &dentry->d_name, S_IFDIR | mode);
hfsplus fs/hfsplus/dir.c:579 hfsplus_mknod(&nop_mnt_idmap, dir, dentry, mode | S_IFDIR, 0)
hpfs fs/hpfs/namei.c:66 result->i_mode |= S_IFDIR;
hugetlbfs fs/hugetlbfs/inode.c:974 mode | S_IFDIR, 0);
jffs2 fs/jffs2/dir.c:465 mode |= S_IFDIR;
jfs fs/jfs/namei.c:226 ip = ialloc(dip, S_IFDIR | mode);
minix fs/minix/namei.c:113 inode = minix_new_inode(dir, S_IFDIR | mode);
nilfs2 fs/nilfs2/namei.c:234 inode = nilfs_new_inode(dir, S_IFDIR | mode);
ntfs3 fs/ntfs3/namei.c:216 S_IFDIR | mode, 0, NULL, 0, NULL));
ocfs2 fs/ocfs2/namei.c:660 ocfs2_mknod(&nop_mnt_idmap, dir, dentry, mode | S_IFDIR, 0);
dlmfs fs/ocfs2/dlmfs/dlmfs.c:425 inode = dlmfs_get_inode(dir, dentry, mode | S_IFDIR);
omfs fs/omfs/dir.c:285 return ERR_PTR(omfs_add_node(dir, dentry, mode | S_IFDIR));
orangefs fs/orangefs/namei.c:336 orangefs_new_inode(dir->i_sb, dir, S_IFDIR | mode, 0, &ref);
ramfs fs/ramfs/inode.c:124 ramfs_mknod(&nop_mnt_idmap, dir, dentry, mode | S_IFDIR, 0);
ubifs fs/ubifs/dir.c:1034 inode = ubifs_new_inode(c, dir, S_IFDIR | mode, false);
udf fs/udf/namei.c:431 inode = udf_new_inode(dir, S_IFDIR | mode);
ufs fs/ufs/namei.c:177 inode = ufs_new_inode(dir, S_IFDIR|mode);
xfs fs/xfs/xfs_iops.c:309 xfs_generic_create(idmap, dir, dentry, mode | S_IFDIR, 0, NULL)
nfs fs/nfs/dir.c:2475 attr.ia_mode = mode | S_IFDIR;
B. Masks the type bits away before use (pre-set S_IFDIR stripped)
overlayfs fs/overlayfs/dir.c:743 ovl_create_object(dentry, (mode & 07777) | S_IFDIR, 0, NULL)
vboxsf fs/vboxsf/dir.c:272-273 params.info.attr.mode = (mode & 0777) |
(is_dir ? SHFL_TYPE_DIRECTORY : SHFL_TYPE_FILE);
nfs (v3) fs/nfs/nfs3xdr.c:562 *p++ = cpu_to_be32(attr->ia_mode & S_IALLUGO);
nfs (v4) fs/nfs/nfs4xdr.c:1170,1196 *p++ = cpu_to_be32(iap->ia_mode & S_IALLUGO);
C. Type comes from elsewhere — incoming mode's type is never consulted
exfat fs/exfat/namei.c:856 exfat_add_entry(dir, ..., TYPE_DIR, &info); // literal dir type
fat/msdos fs/fat/namei_msdos.c:376 msdos_add_entry(dir, ..., 1, ...); // is_dir=1 literal
fat/vfat fs/fat/namei_vfat.c:867 vfat_add_entry(dir, &dentry->d_name, 1, ...); // is_dir=1 literal
coda fs/coda/dir.c:182 attrs.va_mode = mode; // type assigned by Venus userspace server
hostfs fs/hostfs/hostfs_kern.c:698 err = do_mkdir(file, mode); // type re-read from host stat()
cifs fs/smb/client/inode.c:2253 cifs_mkdir(...) // SMB protocol carries dir-vs-file separately, not in mode
ecryptfs fs/ecryptfs/inode.c:521 vfs_mkdir(&nop_mnt_idmap, lower_dir, ...) // delegates to lower fs (cat.
A/B)
bad_inode fs/bad_inode.c:64 return ERR_PTR(-EIO); // never creates anything
These ignore the type field of mode entirely (or never create an inode), so a pre-set S_IFDIR is irrelevant.
D. Pseudo-filesystems — mode is permission-only or dropped
configfs fs/configfs/dir.c mode unused; dir created by make_group()/make_item() callbacks
kernfs fs/kernfs/dir.c:1382 scops->mkdir(parent, dentry->d_name.name, mode) // passed on, never
type-switched
resctrl fs/resctrl/rdtgroup.c:3936 rdtgroup_mkdir() branches on parent/name, not on mode's type bits
tracefs fs/tracefs/inode.c:~222 tracefs_ops.mkdir(name.name.name) // mode dropped entirely
Jori Koolstra (1):
vfs: pass S_IFDIR mode to vfs_prepare_mode()
fs/namei.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
--
2.54.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [RFC PATCH 1/1] vfs: pass S_IFDIR mode to vfs_prepare_mode()
2026-06-11 14:57 [RFC PATCH 0/1] vfs: pass S_IFDIR mode to vfs_prepare_mode() Jori Koolstra
@ 2026-06-11 14:57 ` Jori Koolstra
2026-06-11 15:33 ` [RFC PATCH 0/1] " Jan Kara
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Jori Koolstra @ 2026-06-11 14:57 UTC (permalink / raw)
To: Christian Brauner, Jan Kara, Steve French, Steve French, Al Viro,
NeilBrown, Jeff Layton
Cc: linux-cifs, linux-fsdevel, linux-kernel, jkoolstra
There is a comment in vfs_prepare_mode() that says:
Note that it's currently valid for @type to be 0 if a directory is
created. Filesystems raise that flag individually and we need to check
whether each filesystem can deal with receiving S_IFDIR from the vfs
before we enforce a non-zero type.
This is a bit challenging since there are many filesystems. Claude Opus
4.8 was used to generate the context for each mkdir implementation from
which it can be judged whether passing S_IFDIR is safe. The result was
then verified by hand by looking at how the mode argument is used in
each case. To check whether all mkdir implementations are covered,
'rg "\.mkdir" ' was used and checked against the list of uses the AI
assistent found.
Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
Assisted-by: Claude:Opus 4.8
---
fs/namei.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index 4787244ca4a7..5ae466100fb4 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4142,11 +4142,6 @@ EXPORT_SYMBOL(end_renaming);
* after setgid stripping allows the same ordering for both non-POSIX ACL and
* POSIX ACL supporting filesystems.
*
- * Note that it's currently valid for @type to be 0 if a directory is created.
- * Filesystems raise that flag individually and we need to check whether each
- * filesystem can deal with receiving S_IFDIR from the vfs before we enforce a
- * non-zero type.
- *
* Returns: mode to be passed to the filesystem
*/
static inline umode_t vfs_prepare_mode(struct mnt_idmap *idmap,
@@ -5255,7 +5250,7 @@ struct dentry *vfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
if (!dir->i_op->mkdir)
goto err;
- mode = vfs_prepare_mode(idmap, dir, mode, S_IRWXUGO | S_ISVTX, 0);
+ mode = vfs_prepare_mode(idmap, dir, mode, S_IRWXUGO | S_ISVTX, S_IFDIR);
error = security_inode_mkdir(dir, dentry, mode);
if (error)
goto err;
--
2.54.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH 0/1] vfs: pass S_IFDIR mode to vfs_prepare_mode()
2026-06-11 14:57 [RFC PATCH 0/1] vfs: pass S_IFDIR mode to vfs_prepare_mode() Jori Koolstra
2026-06-11 14:57 ` [RFC PATCH 1/1] " Jori Koolstra
@ 2026-06-11 15:33 ` Jan Kara
2026-06-11 20:20 ` Jori Koolstra
2026-06-11 16:37 ` Al Viro
2026-06-13 1:44 ` NeilBrown
3 siblings, 1 reply; 10+ messages in thread
From: Jan Kara @ 2026-06-11 15:33 UTC (permalink / raw)
To: Jori Koolstra
Cc: Christian Brauner, Jan Kara, Steve French, Steve French, Al Viro,
NeilBrown, Jeff Layton, linux-cifs, linux-fsdevel, linux-kernel
On Thu 11-06-26 16:57:06, Jori Koolstra wrote:
> There is a comment in vfs_prepare_mode() that says:
>
> Note that it's currently valid for @type to be 0 if a directory is
> created. Filesystems raise that flag individually and we need to check
> whether each filesystem can deal with receiving S_IFDIR from the vfs
> before we enforce a non-zero type.
>
> This is a bit challenging since there are many filesystems. Claude Opus
> 4.8 was used to generate the context for each mkdir implementation from
> which it can be judged whether passing S_IFDIR is safe. The result was
> then verified by hand by looking at how the mode argument is used in
> each case. To check whether all mkdir implementations are convered,
> 'rg "\.mkdir" ' was used and checked against the list of uses Claude
> found.
>
> The only mkdir implementation that I am not as sure of is
> cifs_mkdir(). So this goes cc to the cifs people. But of course, I might
> have made errors in other places (except probably not for the A list as
> those are all just mode | S_IFDIR cases without other uses of mode
> besides logging)
For the filesystems I know this looks sound. Thanks for the cleanup. But
this series should then follow with the removal of now-pointless "|
S_IFDIR" statements.
Honza
> This is the AI context I worked with:
>
> Here's each filesystem with the exact line(s) that make it safe, grouped by why.
>
> A. Forces S_IFDIR itself (redundant if pre-set → harmless)
>
> 9p fs/9p/vfs_inode.c:683 perm = unixmode2p9mode(v9ses, mode | S_IFDIR);
> 9p (dotl) fs/9p/vfs_inode_dotl.c:365 omode |= S_IFDIR;
> affs fs/affs/namei.c:290 inode->i_mode = S_IFDIR | mode;
> afs fs/afs/dir.c:1326 op->create.mode = S_IFDIR | mode;
> autofs fs/autofs/root.c:744 inode = autofs_get_inode(dir->i_sb, S_IFDIR | mode);
> btrfs fs/btrfs/inode.c:7123 inode_init_owner(idmap, inode, dir, S_IFDIR | mode);
> ceph fs/ceph/dir.c:1145 mode |= S_IFDIR;
> ext2 fs/ext2/namei.c:239 inode = ext2_new_inode(dir, S_IFDIR | mode, &dentry->d_name);
> ext4 fs/ext4/namei.c:3012 inode = ext4_new_inode_start_handle(idmap, dir, S_IFDIR | mode,
> f2fs fs/f2fs/namei.c:745 inode = f2fs_new_inode(idmap, dir, S_IFDIR | mode, NULL);
> fuse fs/fuse/dir.c:1131 return create_new_entry(idmap, fm, &args, dir, entry, S_IFDIR);
> gfs2 fs/gfs2/inode.c:1351 gfs2_create_inode(dir, dentry, NULL, S_IFDIR | mode, ...)
> hfs fs/hfs/dir.c:228 inode = hfs_new_inode(dir, &dentry->d_name, S_IFDIR | mode);
> hfsplus fs/hfsplus/dir.c:579 hfsplus_mknod(&nop_mnt_idmap, dir, dentry, mode | S_IFDIR, 0)
> hpfs fs/hpfs/namei.c:66 result->i_mode |= S_IFDIR;
> hugetlbfs fs/hugetlbfs/inode.c:974 mode | S_IFDIR, 0);
> jffs2 fs/jffs2/dir.c:465 mode |= S_IFDIR;
> jfs fs/jfs/namei.c:226 ip = ialloc(dip, S_IFDIR | mode);
> minix fs/minix/namei.c:113 inode = minix_new_inode(dir, S_IFDIR | mode);
> nilfs2 fs/nilfs2/namei.c:234 inode = nilfs_new_inode(dir, S_IFDIR | mode);
> ntfs3 fs/ntfs3/namei.c:216 S_IFDIR | mode, 0, NULL, 0, NULL));
> ocfs2 fs/ocfs2/namei.c:660 ocfs2_mknod(&nop_mnt_idmap, dir, dentry, mode | S_IFDIR, 0);
> dlmfs fs/ocfs2/dlmfs/dlmfs.c:425 inode = dlmfs_get_inode(dir, dentry, mode | S_IFDIR);
> omfs fs/omfs/dir.c:285 return ERR_PTR(omfs_add_node(dir, dentry, mode | S_IFDIR));
> orangefs fs/orangefs/namei.c:336 orangefs_new_inode(dir->i_sb, dir, S_IFDIR | mode, 0, &ref);
> ramfs fs/ramfs/inode.c:124 ramfs_mknod(&nop_mnt_idmap, dir, dentry, mode | S_IFDIR, 0);
> ubifs fs/ubifs/dir.c:1034 inode = ubifs_new_inode(c, dir, S_IFDIR | mode, false);
> udf fs/udf/namei.c:431 inode = udf_new_inode(dir, S_IFDIR | mode);
> ufs fs/ufs/namei.c:177 inode = ufs_new_inode(dir, S_IFDIR|mode);
> xfs fs/xfs/xfs_iops.c:309 xfs_generic_create(idmap, dir, dentry, mode | S_IFDIR, 0, NULL)
> nfs fs/nfs/dir.c:2475 attr.ia_mode = mode | S_IFDIR;
>
> B. Masks the type bits away before use (pre-set S_IFDIR stripped)
>
> overlayfs fs/overlayfs/dir.c:743 ovl_create_object(dentry, (mode & 07777) | S_IFDIR, 0, NULL)
> vboxsf fs/vboxsf/dir.c:272-273 params.info.attr.mode = (mode & 0777) |
> (is_dir ? SHFL_TYPE_DIRECTORY : SHFL_TYPE_FILE);
> nfs (v3) fs/nfs/nfs3xdr.c:562 *p++ = cpu_to_be32(attr->ia_mode & S_IALLUGO);
> nfs (v4) fs/nfs/nfs4xdr.c:1170,1196 *p++ = cpu_to_be32(iap->ia_mode & S_IALLUGO);
>
> C. Type comes from elsewhere — incoming mode's type is never consulted
>
> exfat fs/exfat/namei.c:856 exfat_add_entry(dir, ..., TYPE_DIR, &info); // literal dir type
> fat/msdos fs/fat/namei_msdos.c:376 msdos_add_entry(dir, ..., 1, ...); // is_dir=1 literal
> fat/vfat fs/fat/namei_vfat.c:867 vfat_add_entry(dir, &dentry->d_name, 1, ...); // is_dir=1 literal
> coda fs/coda/dir.c:182 attrs.va_mode = mode; // type assigned by Venus userspace server
> hostfs fs/hostfs/hostfs_kern.c:698 err = do_mkdir(file, mode); // type re-read from host stat()
> cifs fs/smb/client/inode.c:2253 cifs_mkdir(...) // SMB protocol carries dir-vs-file separately, not in mode
> ecryptfs fs/ecryptfs/inode.c:521 vfs_mkdir(&nop_mnt_idmap, lower_dir, ...) // delegates to lower fs (cat.
> A/B)
> bad_inode fs/bad_inode.c:64 return ERR_PTR(-EIO); // never creates anything
>
> These ignore the type field of mode entirely (or never create an inode), so a pre-set S_IFDIR is irrelevant.
>
> D. Pseudo-filesystems — mode is permission-only or dropped
>
> configfs fs/configfs/dir.c mode unused; dir created by make_group()/make_item() callbacks
> kernfs fs/kernfs/dir.c:1382 scops->mkdir(parent, dentry->d_name.name, mode) // passed on, never
> type-switched
> resctrl fs/resctrl/rdtgroup.c:3936 rdtgroup_mkdir() branches on parent/name, not on mode's type bits
> tracefs fs/tracefs/inode.c:~222 tracefs_ops.mkdir(name.name.name) // mode dropped entirely
>
> Jori Koolstra (1):
> vfs: pass S_IFDIR mode to vfs_prepare_mode()
>
> fs/namei.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> --
> 2.54.0
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH 0/1] vfs: pass S_IFDIR mode to vfs_prepare_mode()
2026-06-11 14:57 [RFC PATCH 0/1] vfs: pass S_IFDIR mode to vfs_prepare_mode() Jori Koolstra
2026-06-11 14:57 ` [RFC PATCH 1/1] " Jori Koolstra
2026-06-11 15:33 ` [RFC PATCH 0/1] " Jan Kara
@ 2026-06-11 16:37 ` Al Viro
2026-06-11 20:16 ` Jori Koolstra
2026-06-13 1:44 ` NeilBrown
3 siblings, 1 reply; 10+ messages in thread
From: Al Viro @ 2026-06-11 16:37 UTC (permalink / raw)
To: Jori Koolstra
Cc: Christian Brauner, Jan Kara, Steve French, Steve French,
NeilBrown, Jeff Layton, linux-cifs, linux-fsdevel, linux-kernel
On Thu, Jun 11, 2026 at 04:57:06PM +0200, Jori Koolstra wrote:
> There is a comment in vfs_prepare_mode() that says:
>
> Note that it's currently valid for @type to be 0 if a directory is
> created. Filesystems raise that flag individually and we need to check
> whether each filesystem can deal with receiving S_IFDIR from the vfs
> before we enforce a non-zero type.
>
> This is a bit challenging since there are many filesystems. Claude Opus
> 4.8 was used to generate the context for each mkdir implementation from
> which it can be judged whether passing S_IFDIR is safe. The result was
> then verified by hand by looking at how the mode argument is used in
> each case. To check whether all mkdir implementations are convered,
> 'rg "\.mkdir" ' was used and checked against the list of uses Claude
> found.
>
> The only mkdir implementation that I am not as sure of is
> cifs_mkdir(). So this goes cc to the cifs people. But of course, I might
> have made errors in other places (except probably not for the A list as
> those are all just mode | S_IFDIR cases without other uses of mode
> besides logging)
There's a missing bit here: what's the point?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH 0/1] vfs: pass S_IFDIR mode to vfs_prepare_mode()
2026-06-11 16:37 ` Al Viro
@ 2026-06-11 20:16 ` Jori Koolstra
0 siblings, 0 replies; 10+ messages in thread
From: Jori Koolstra @ 2026-06-11 20:16 UTC (permalink / raw)
To: Al Viro
Cc: Christian Brauner, Jan Kara, Steve French, Steve French,
NeilBrown, Jeff Layton, linux-cifs, linux-fsdevel, linux-kernel
> Op 11-06-2026 18:37 CEST schreef Al Viro <viro@zeniv.linux.org.uk>:
>
> There's a missing bit here: what's the point?
To clean up a bit ahead of O_CREAT|O_DIRECTORY. Specifically, in lookup_open()
we need to replace the vfs_prepare_mode() with something that also handles dirs.
I don't really want to push the odd
mode = vfs_prepare_mode(idmap, dir, mode, S_IRWXUGO | S_ISVTX, 0);
further into that code, and neither do I want this to be different from the
regular vfs_mkdir() path. We can then also match on S_IFMT in may_o_create(),
instead of passing a bool to signal whether we are creating a dir (and assuming
0 means a dir is really ugly).
This was discussed a bit in an rfc patch for O_CREAT|O_DIRECTORY, and Christian
said when he wrote that comment he "might've been overly cautious"[1]
I should have added this explanation to the commit message, you are right about
that. Thanks for pointing that out.
Best,
Jori.
[1]: https://lore.kernel.org/linux-fsdevel/20260527-fotowettbewerb-abwinken-einfach-83db3411945b@brauner/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH 0/1] vfs: pass S_IFDIR mode to vfs_prepare_mode()
2026-06-11 15:33 ` [RFC PATCH 0/1] " Jan Kara
@ 2026-06-11 20:20 ` Jori Koolstra
2026-06-12 10:33 ` Jan Kara
0 siblings, 1 reply; 10+ messages in thread
From: Jori Koolstra @ 2026-06-11 20:20 UTC (permalink / raw)
To: Jan Kara
Cc: Christian Brauner, Steve French, Steve French, Al Viro,
NeilBrown, Jeff Layton, linux-cifs, linux-fsdevel, linux-kernel
Hi Jan,
> Op 11-06-2026 17:33 CEST schreef Jan Kara <jack@suse.cz>:
>
> For the filesystems I know this looks sound. Thanks for the cleanup. But
> this series should then follow with the removal of now-pointless "|
> S_IFDIR" statements.
>
Right, perhaps it should. But wouldn't that be too much churn? I know Al can
really be on the brake sometimes with regards to that. But it isn't a lot of
effort, so I am fine with it.
Best,
Jori.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH 0/1] vfs: pass S_IFDIR mode to vfs_prepare_mode()
2026-06-11 20:20 ` Jori Koolstra
@ 2026-06-12 10:33 ` Jan Kara
2026-06-12 12:19 ` Christian Brauner
0 siblings, 1 reply; 10+ messages in thread
From: Jan Kara @ 2026-06-12 10:33 UTC (permalink / raw)
To: Jori Koolstra
Cc: Jan Kara, Christian Brauner, Steve French, Steve French, Al Viro,
NeilBrown, Jeff Layton, linux-cifs, linux-fsdevel, linux-kernel
On Thu 11-06-26 22:20:40, Jori Koolstra wrote:
> Hi Jan,
>
> > Op 11-06-2026 17:33 CEST schreef Jan Kara <jack@suse.cz>:
> >
> > For the filesystems I know this looks sound. Thanks for the cleanup. But
> > this series should then follow with the removal of now-pointless "|
> > S_IFDIR" statements.
> >
>
> Right, perhaps it should. But wouldn't that be too much churn? I know Al can
> really be on the brake sometimes with regards to that. But it isn't a lot of
> effort, so I am fine with it.
I agree it will be more churn but leaving the now-pointless | S_IFDIR in
the filesystems is confusing for code readers which isn't good either. So
at least for filesystems I maintain I would like that cleanup to be done.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH 0/1] vfs: pass S_IFDIR mode to vfs_prepare_mode()
2026-06-12 10:33 ` Jan Kara
@ 2026-06-12 12:19 ` Christian Brauner
0 siblings, 0 replies; 10+ messages in thread
From: Christian Brauner @ 2026-06-12 12:19 UTC (permalink / raw)
To: Jan Kara
Cc: Jori Koolstra, Steve French, Steve French, Al Viro, NeilBrown,
Jeff Layton, linux-cifs, linux-fsdevel, linux-kernel
On Fri, Jun 12, 2026 at 12:33:53PM +0200, Jan Kara wrote:
> On Thu 11-06-26 22:20:40, Jori Koolstra wrote:
> > Hi Jan,
> >
> > > Op 11-06-2026 17:33 CEST schreef Jan Kara <jack@suse.cz>:
> > >
> > > For the filesystems I know this looks sound. Thanks for the cleanup. But
> > > this series should then follow with the removal of now-pointless "|
> > > S_IFDIR" statements.
> > >
> >
> > Right, perhaps it should. But wouldn't that be too much churn? I know Al can
> > really be on the brake sometimes with regards to that. But it isn't a lot of
> > effort, so I am fine with it.
>
> I agree it will be more churn but leaving the now-pointless | S_IFDIR in
> the filesystems is confusing for code readers which isn't good either. So
> at least for filesystems I maintain I would like that cleanup to be done.
Jan is (as usual) right.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH 0/1] vfs: pass S_IFDIR mode to vfs_prepare_mode()
2026-06-11 14:57 [RFC PATCH 0/1] vfs: pass S_IFDIR mode to vfs_prepare_mode() Jori Koolstra
` (2 preceding siblings ...)
2026-06-11 16:37 ` Al Viro
@ 2026-06-13 1:44 ` NeilBrown
2026-06-14 16:57 ` Jori Koolstra
3 siblings, 1 reply; 10+ messages in thread
From: NeilBrown @ 2026-06-13 1:44 UTC (permalink / raw)
To: Jori Koolstra
Cc: Christian Brauner, Jan Kara, Steve French, Steve French, Al Viro,
Jeff Layton, linux-cifs, linux-fsdevel, linux-kernel, jkoolstra
On Fri, 12 Jun 2026, Jori Koolstra wrote:
> There is a comment in vfs_prepare_mode() that says:
>
> Note that it's currently valid for @type to be 0 if a directory is
> created. Filesystems raise that flag individually and we need to check
> whether each filesystem can deal with receiving S_IFDIR from the vfs
> before we enforce a non-zero type.
>
> This is a bit challenging since there are many filesystems. Claude Opus
> 4.8 was used to generate the context for each mkdir implementation from
> which it can be judged whether passing S_IFDIR is safe. The result was
> then verified by hand by looking at how the mode argument is used in
> each case. To check whether all mkdir implementations are convered,
> 'rg "\.mkdir" ' was used and checked against the list of uses Claude
> found.
>
> The only mkdir implementation that I am not as sure of is
> cifs_mkdir(). So this goes cc to the cifs people. But of course, I might
> have made errors in other places (except probably not for the A list as
> those are all just mode | S_IFDIR cases without other uses of mode
> besides logging)
I agree that cifs_mkdir() is not obviously safe.
I think coda_mkdir() and fuse_mkdir() at also not obviously safe.
In all three cases the mode, which now has S_IFDIR included, is sent
unchanged to something outside of Linux - either user-space or over at
network connection.
I think you patch which adds S_IFDIR to mode should mask it back out in
those three functions. Then we could be certain it is safe.
The maintainers of those filesystems might then choose to remove the
mask if they know it to be safe to do so
Thanks,
NeilBrown
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH 0/1] vfs: pass S_IFDIR mode to vfs_prepare_mode()
2026-06-13 1:44 ` NeilBrown
@ 2026-06-14 16:57 ` Jori Koolstra
0 siblings, 0 replies; 10+ messages in thread
From: Jori Koolstra @ 2026-06-14 16:57 UTC (permalink / raw)
To: NeilBrown, NeilBrown
Cc: Christian Brauner, Jan Kara, Steve French, Steve French, Al Viro,
Jeff Layton, linux-cifs, linux-fsdevel, linux-kernel
> Op 13-06-2026 03:44 CEST schreef NeilBrown <neilb@ownmail.net>:
>
>
> On Fri, 12 Jun 2026, Jori Koolstra wrote:
> > There is a comment in vfs_prepare_mode() that says:
> >
> > Note that it's currently valid for @type to be 0 if a directory is
> > created. Filesystems raise that flag individually and we need to check
> > whether each filesystem can deal with receiving S_IFDIR from the vfs
> > before we enforce a non-zero type.
> >
> > This is a bit challenging since there are many filesystems. Claude Opus
> > 4.8 was used to generate the context for each mkdir implementation from
> > which it can be judged whether passing S_IFDIR is safe. The result was
> > then verified by hand by looking at how the mode argument is used in
> > each case. To check whether all mkdir implementations are convered,
> > 'rg "\.mkdir" ' was used and checked against the list of uses Claude
> > found.
> >
> > The only mkdir implementation that I am not as sure of is
> > cifs_mkdir(). So this goes cc to the cifs people. But of course, I might
> > have made errors in other places (except probably not for the A list as
> > those are all just mode | S_IFDIR cases without other uses of mode
> > besides logging)
>
> I agree that cifs_mkdir() is not obviously safe.
> I think coda_mkdir() and fuse_mkdir() at also not obviously safe.
> In all three cases the mode, which now has S_IFDIR included, is sent
> unchanged to something outside of Linux - either user-space or over at
> network connection.
You are (of course) right. I missed to mention coda. Also, I missed that
for FUSE inarg.mode = mode; is set, and this is passed, despite the mode
in create_new_entry(idmap, fm, &args, dir, entry, S_IFDIR); being hardwired
to S_IFDIR.
>
> I think you patch which adds S_IFDIR to mode should mask it back out in
> those three functions. Then we could be certain it is safe.
> The maintainers of those filesystems might then choose to remove the
> mask if they know it to be safe to do so
That seems like a good idea, agreed.
>
> Thanks,
> NeilBrown
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2026-06-14 16:58 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-06-11 14:57 [RFC PATCH 0/1] vfs: pass S_IFDIR mode to vfs_prepare_mode() Jori Koolstra
2026-06-11 14:57 ` [RFC PATCH 1/1] " Jori Koolstra
2026-06-11 15:33 ` [RFC PATCH 0/1] " Jan Kara
2026-06-11 20:20 ` Jori Koolstra
2026-06-12 10:33 ` Jan Kara
2026-06-12 12:19 ` Christian Brauner
2026-06-11 16:37 ` Al Viro
2026-06-11 20:16 ` Jori Koolstra
2026-06-13 1:44 ` NeilBrown
2026-06-14 16:57 ` Jori Koolstra
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®