Re: [GIT PULL] smb3 client fixes

[Paulo Alcantara <pc@manguebit.org>]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Mon, 13 Apr 2026 at 15:06, Steve French <smfrench@gmail.com> wrote:
>>
>>   git://git.samba.org/sfrench/cifs-2.6.git tags/v7.1-rc1-part1-smb3-client-fixes
>
> I've pulled this, but then looking at the dcache changes I noted the
> big forest of BUG_ON() which really isn't valid. Error handling is a
> thing - BUG_ON() is *not* error handling.
>
> And then looking at verifying the length of the name - one of the
> things checked for in that forest of BUG_ON() calls - the call site is
> an unreadable mess.
>
> You have this:
>
>         size_t size = CIFS_TMPNAME_LEN + 1;
>
> fifty lines earlier, and then you do
>
>         d_mark_tmpfile_name(file, &QSTR_LEN(name, size - 1));
>
> which is not just illegible, it's also illogical. That "size" is just
> voodoo. The string is generated by
>
>                 scnprintf(name, size,
>                           CIFS_TMPNAME_PREFIX "%0*x",
>                           CIFS_TMPNAME_COUNTER_LEN,
>                           atomic_inc_return(&cifs_tmpcounter));
>
> which uses several other magic #define's, and yes, I'm sure it all
> adds up to CIFS_TMPNAME_LEN in the end, but this is basically all just
> line noise.
>
> PLEASE write this legibly instead, and make that new dentry helper
> actually do error handling, not BUG_ON().
>
> Because this kind of mess is simply not acceptable.
>
> I don't even understand why you use a variable for an insane constant.
> The code *could* have done something like this:
>
>         namelen = scnprintf(name, size,
>                           CIFS_TMPNAME_PREFIX "%0*x",
>                           CIFS_TMPNAME_COUNTER_LEN,
>                           atomic_inc_return(&cifs_tmpcounter));
>
> and it would all have actually made sense. But stating the final size
> like that really doesn't - not without at least a big comment on how
> those random things are interrelated.
>
> So this is in my tree now, but I expect it to be cleaned up and made sensible.

ACK.

Do the below changes look any better?  I wasn't sure what exact error
values to return from d_mark_tmpfile_name(), hopefully it's fine.

diff --git a/fs/dcache.c b/fs/dcache.c
index df11bbba0342..151f83f0a0e5 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -3196,15 +3196,20 @@ 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)
+int 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));
-	BUG_ON(d_really_is_positive(dentry));
-	BUG_ON(!d_unlinked(dentry));
-	BUG_ON(name->len > DNAME_INLINE_LEN - 1);
+	if (WARN_ON_ONCE(dname_external(dentry)))
+		return -EINVAL;
+	if (WARN_ON_ONCE(d_really_is_positive(dentry)))
+		return -EINVAL;
+	if (WARN_ON_ONCE(!d_unlinked(dentry)))
+		return -EINVAL;
+	if (WARN_ON_ONCE(name->len > DNAME_INLINE_LEN - 1))
+		return -ENAMETOOLONG;
+
 	spin_lock(&dentry->d_parent->d_lock);
 	spin_lock_nested(&dentry->d_lock, DENTRY_D_LOCK_NESTED);
 	dentry->__d_name.len = name->len;
@@ -3212,6 +3217,7 @@ void d_mark_tmpfile_name(struct file *file, const struct qstr *name)
 	dname[name->len] = '\0';
 	spin_unlock(&dentry->d_lock);
 	spin_unlock(&dentry->d_parent->d_lock);
+	return 0;
 }
 EXPORT_SYMBOL(d_mark_tmpfile_name);
 
diff --git a/fs/smb/client/cifsfs.h b/fs/smb/client/cifsfs.h
index 18f9f93a01b4..804b57595ab8 100644
--- a/fs/smb/client/cifsfs.h
+++ b/fs/smb/client/cifsfs.h
@@ -10,6 +10,7 @@
 #define _CIFSFS_H
 
 #include <linux/hash.h>
+#include <linux/dcache.h>
 
 #define ROOT_I 2
 
@@ -149,17 +150,8 @@ struct dentry *cifs_smb3_do_mount(struct file_system_type *fs_type, int flags,
 
 char *cifs_silly_fullpath(struct dentry *dentry);
 
-#define CIFS_TMPNAME_PREFIX        ".__smbfile_tmp"
-#define CIFS_TMPNAME_PREFIX_LEN    ((int)sizeof(CIFS_TMPNAME_PREFIX) - 1)
-#define CIFS_TMPNAME_COUNTER_LEN   ((int)sizeof(cifs_tmpcounter) * 2)
-#define CIFS_TMPNAME_LEN \
-	(CIFS_TMPNAME_PREFIX_LEN + CIFS_TMPNAME_COUNTER_LEN)
-
-#define CIFS_SILLYNAME_PREFIX	    ".__smbfile_silly"
-#define CIFS_SILLYNAME_PREFIX_LEN  ((int)sizeof(CIFS_SILLYNAME_PREFIX) - 1)
-#define CIFS_SILLYNAME_COUNTER_LEN ((int)sizeof(cifs_sillycounter) * 2)
-#define CIFS_SILLYNAME_LEN \
-	(CIFS_SILLYNAME_PREFIX_LEN + CIFS_SILLYNAME_COUNTER_LEN)
+#define CIFS_TMPNAME_LEN	(DNAME_INLINE_LEN - 1)
+#define CIFS_SILLYNAME_LEN	(DNAME_INLINE_LEN - 1)
 
 #ifdef CONFIG_CIFS_NFSD_EXPORT
 extern const struct export_operations cifs_export_ops;
diff --git a/fs/smb/client/dir.c b/fs/smb/client/dir.c
index 6ea1ae7f7a46..2abe76a7cec0 100644
--- a/fs/smb/client/dir.c
+++ b/fs/smb/client/dir.c
@@ -1056,9 +1056,9 @@ int cifs_tmpfile(struct mnt_idmap *idmap, struct inode *dir,
 {
 	struct dentry *dentry = file->f_path.dentry;
 	struct cifs_sb_info *cifs_sb = CIFS_SB(dir);
+	size_t namesize = CIFS_TMPNAME_LEN + 1;
 	char *path __free(kfree) = NULL, *name;
 	unsigned int oflags = file->f_flags;
-	size_t size = CIFS_TMPNAME_LEN + 1;
 	int retries = 0, max_retries = 16;
 	struct TCP_Server_Info *server;
 	struct cifs_pending_open open;
@@ -1070,6 +1070,7 @@ int cifs_tmpfile(struct mnt_idmap *idmap, struct inode *dir,
 	struct inode *inode;
 	unsigned int xid;
 	__u32 oplock;
+	int namelen;
 	int rc;
 
 	if (unlikely(cifs_forced_shutdown(cifs_sb)))
@@ -1093,7 +1094,7 @@ int cifs_tmpfile(struct mnt_idmap *idmap, struct inode *dir,
 		server->ops->new_lease_key(&fid);
 	cifs_add_pending_open(&fid, tlink, &open);
 
-	path = alloc_parent_path(dentry, size - 1);
+	path = alloc_parent_path(dentry, namesize - 1);
 	if (IS_ERR(path)) {
 		cifs_del_pending_open(&open);
 		rc = PTR_ERR(path);
@@ -1103,16 +1104,21 @@ int cifs_tmpfile(struct mnt_idmap *idmap, struct inode *dir,
 
 	name = path + strlen(path);
 	do {
-		scnprintf(name, size,
-			  CIFS_TMPNAME_PREFIX "%0*x",
-			  CIFS_TMPNAME_COUNTER_LEN,
-			  atomic_inc_return(&cifs_tmpcounter));
+		/* Append tmpfile name to @path */
+		namelen = scnprintf(name, namesize, ".__smbfile_tmp%0*x",
+				    (int)sizeof(cifs_tmpcounter) * 2,
+				    atomic_inc_return(&cifs_tmpcounter));
 		rc = __cifs_do_create(dir, dentry, path, xid, tlink, oflags,
 				      mode, &oplock, &fid, NULL, &inode);
 		if (!rc) {
+			rc = d_mark_tmpfile_name(file, &QSTR_LEN(name, namelen));
+			if (rc) {
+				rc = -EISDIR;
+				iput(inode);
+				goto err_open;
+			}
 			set_nlink(inode, 0);
 			mark_inode_dirty(inode);
-			d_mark_tmpfile_name(file, &QSTR_LEN(name, size - 1));
 			d_instantiate(dentry, inode);
 			break;
 		}
@@ -1168,9 +1174,8 @@ char *cifs_silly_fullpath(struct dentry *dentry)
 
 	do {
 		dput(sdentry);
-		scnprintf(name, namesize,
-			  CIFS_SILLYNAME_PREFIX "%0*x",
-			  CIFS_SILLYNAME_COUNTER_LEN,
+		scnprintf(name, namesize, ".__smbfile_silly%0*x",
+			  (int)sizeof(cifs_sillycounter) * 2,
 			  atomic_inc_return(&cifs_sillycounter));
 		sdentry = lookup_noperm(&QSTR(name), dentry->d_parent);
 		if (IS_ERR(sdentry))
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index f60819dcfebd..c5bd5a74baba 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -264,7 +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);
+int 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 *);