mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [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®