Re: [RFC PATCH] smb: client: add support for O_TMPFILE

[Paulo Alcantara <pc@manguebit.org>]

Al Viro <viro@zeniv.linux.org.uk> writes:

> On Tue, Mar 31, 2026 at 10:11:53PM -0300, Paulo Alcantara wrote:
>
>>  	d_drop(direntry);
>> -	d_add(direntry, newinode);
>> -
>> +	inode_set_mtime_to_ts(inode, inode_set_ctime_current(inode));
>> +	if (oflags & O_TMPFILE) {
>> +		set_nlink(newinode, 0);
>> +		mark_inode_dirty(newinode);
>> +		d_instantiate(direntry, newinode);
>> +	} else {
>> +		d_add(direntry, newinode);
>> +	}
>
> What d_unhashed(dentry) is going to be when we arrive to that thing
> with O_TMPFILE?

It's called with an unhashed negative dentry.  Do you see any problems
with it?

>> +static void cifs_d_mark_tmpfile(struct file *file,
>> +				const unsigned char *name,
>> +				size_t namelen)
>> +{
>> +	struct dentry *dentry = file->f_path.dentry;
>> +
>> +	BUG_ON(dentry->d_name.name != dentry->d_shortname.string ||
>> +	       !hlist_unhashed(&dentry->d_u.d_alias) ||
>> +	       !d_unlinked(dentry) ||
>> +	       namelen > DNAME_INLINE_LEN - 1);
>> +	spin_lock(&dentry->d_parent->d_lock);
>> +	spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_NESTED);
>> +	dentry->__d_name.len = sprintf(dentry->d_shortname.string, "%.*s",
>> +				       (int)namelen, name);
>
> That's one hell of an odd way to spell that...  What's wrong with using union
> shortname_store for an argument?  Just memchr() for '\0' to verify it's there
> and use the result to calculate the length and plain assignment to d_shortname
> for copying...
>
> And in any case, that !hlist_unhashed(....) in there is d_really_is_positive().

I was thinking more along these lines (e.g. add a new VFS helper and
then call it in CIFS, as you suggested earlier)

    diff --git a/fs/dcache.c b/fs/dcache.c
    index 7ba1801d8132..c20a9c9e921c 100644
    --- a/fs/dcache.c
    +++ b/fs/dcache.c
    @@ -3196,6 +3196,25 @@ void d_mark_tmpfile(struct file *file, struct inode *inode)
     }
     EXPORT_SYMBOL(d_mark_tmpfile);
     
    +void d_mark_tmpfile_name(struct file *file, const struct qstr *name)
    +{
    +       struct dentry *dentry = file->f_path.dentry;
    +       char *dname = dentry->d_shortname.string;
    +
    +       BUG_ON(dname_external(dentry) ||
    +              d_really_is_positive(dentry) ||
    +              !d_unlinked(dentry) ||
    +              name->len > DNAME_INLINE_LEN - 1);
    +       spin_lock(&dentry->d_parent->d_lock);
    +       spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_NESTED);
    +       dentry->__d_name.len = name->len;
    +       memcpy(dname, name->name, name->len);
    +       dname[name->len] = '\0';
    +       spin_unlock(&dentry->d_lock);
    +       spin_unlock(&dentry->d_parent->d_lock);
    +}
    +EXPORT_SYMBOL(d_mark_tmpfile_name);
    +
     void d_tmpfile(struct file *file, struct inode *inode)
     {
            struct dentry *dentry = file->f_path.dentry;
    diff --git a/include/linux/dcache.h b/include/linux/dcache.h
    index 898c60d21c92..f60819dcfebd 100644
    --- a/include/linux/dcache.h
    +++ b/include/linux/dcache.h
    @@ -264,6 +264,7 @@ extern void d_invalidate(struct dentry *);
     extern struct dentry * d_make_root(struct inode *);
     
     extern void d_mark_tmpfile(struct file *, struct inode *);
    +void d_mark_tmpfile_name(struct file *file, const struct qstr *name);
     extern void d_tmpfile(struct file *, struct inode *);
     
     extern struct dentry *d_find_alias(struct inode *);

Looks good?

>> +static int set_tmpfile_name(struct file *file)
>> +{
>> +	struct dentry *dentry = file->f_path.dentry;
>> +	unsigned char name[CIFS_TMPNAME_LEN + 1];
>> +	struct dentry *sdentry = NULL;
>> +
>> +	do {
>> +		dput(sdentry);
>> +		scnprintf(name, sizeof(name),
>> +			  CIFS_TMPNAME_PREFIX "%0*x",
>> +			  CIFS_TMPNAME_COUNTER_LEN,
>> +			  atomic_inc_return(&cifs_tmpcounter));
>> +		sdentry = lookup_noperm_unlocked(&QSTR(name), dentry->d_parent);
>> +		if (IS_ERR(sdentry))
>> +			return -EBUSY;
>> +	} while (!d_is_negative(sdentry));
>> +	dput(sdentry);
>
> That looks racy.  Checking it doesn't exist at the moment is fine, but what if
> it's created right after that lookup?  You are not holding any locks, so even
> the same-client race (with plain create()) is possible...

Yeah, that's definitely a TOCTOU race.  I should've looped over
cifs_do_create() instead to make sure that the tmpfile is created in the
server, by using a sane retry counter.