Re: [RFC PATCH v6 7/9] vfs: add O_CREAT|O_DIRECTORY to open*(2)

[Jori Koolstra]

> Op 01-06-2026 05:16 CEST schreef NeilBrown <neilb@xxxxxxxxxxx>:
> > +
> > +static inline umode_t o_create_mode(struct mnt_idmap *idmap,
> > +		const struct inode *dir, umode_t mode, bool create_dir)
> > +{
> > +	if (create_dir)
> > +		return vfs_prepare_mode(idmap, dir, mode, S_IRWXUGO | S_ISVTX, 0);
> > +	else
> > +		return vfs_prepare_mode(idmap, dir, mode, S_IALLUGO, S_IFREG);
> 
> I still don't see a justification for different permission masks.
> There should be a comment explaining why there is a difference, though
> I'm not convinced a difference is needed.
> 

I just copied current semantics from vfs_mkdir() and vfs_create().

Let me add what you wrote before here for context:

> It isn't clear the purpose of this.
> In the create_dir case, shouldn't we pass S_IFDIR as the last arg: type?
> 

Yes, you are correct. This is being handled separately.[1] (I know you know
that, but let me add this to provide context for other readers).

> The difference between "S_IRWXUGO | S_ISVTX" and "S_IALLUGO" is not
> immediately obvious, but the former excludes S_ISUID and S_ISGID.
> The setuid bit is meaningless on directories so there is little point in
> stripping it, but I don't object as long as the intent is documented
> here.
> The setgid bit is meaningful but vfs_prepare_mode() seems to already
> handle it correctly for directories if you pass the type as S_IFDIR.

mode_strip_sgid() is just for regular files and is a noop when passed
a S_IFDIR. So I am not quite sure what you mean.

Apparenly, mkdir(2) cannot enable the setgid bit itself. This has been the
case since the initial git commit, which contains:

+       mode &= (S_IRWXUGO|S_ISVTX);
+       error = security_inode_mkdir(dir, dentry, mode);
+       if (error)
+               return error;
+
+       DQUOT_INIT(dir);
+       error = dir->i_op->mkdir(dir, dentry, mode);

I am not sure why.

@Al, you have been around for a long time? Do you know why mkdir(2) cannot
enable the setgid bit?

> If you have a good reason to impose different handling here, that might
> be sensible, but it should be documented.


> >  }
> >  
> >  /*
> > @@ -4379,8 +4409,12 @@ static struct dentry *atomic_open(const struct path *path, struct dentry *dentry
> >  
> >  	file->__f_path.dentry = DENTRY_NOT_SET;
> >  	file->__f_path.mnt = path->mnt;
> > -	error = dir->i_op->atomic_open(dir, dentry, file,
> > -				       open_to_namei_flags(open_flag), mode);
> > +
> > +	if (O_IS_MKDIR(open_flag))
> > +		error = EINVAL;
> > +	else
> > +		error = dir->i_op->atomic_open(dir, dentry, file,
> > +					       open_to_namei_flags(open_flag), mode);
> >  	d_lookup_done(dentry);
> >  	if (!error) {
> >  		if (file->f_mode & FMODE_OPENED) {
> > @@ -4413,6 +4447,10 @@ static struct dentry *atomic_open(const struct path *path, struct dentry *dentry
> >  	return dentry;
> >  }
> >  
> > +static inline
> > +struct dentry *vfs_mkdir_no_perm(struct mnt_idmap *, struct inode *,
> > +				 struct dentry *, umode_t,
> > +				 struct delegated_inode *);
> 
> forward declaration aren't ideal...  This is a new function that you
> introduced.  Could you place it earlier when you introduce it?
> 

I wanted to keep vfs_mkdir_no_perm() close to vfs_mkdir(). But I can
live with moving it.

Thanks,
Jori.