From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx1.manguebit.org (mx1.manguebit.org [143.255.12.172]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id F31AB30FF31; Tue, 14 Apr 2026 03:13:37 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=143.255.12.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1776136419; cv=none; b=FPOpcRl0u5CtAkT7VmRkHY/DHjnZmfBVV/kTy6D9OcCGZwQeYBvfJvesF7KGfSKUHptOP5yn6qos2G4/XnPWSisq1Izf80Ts+67b2T1QbBSCvnBtUzpXrEbGKCMoRiH0pOkpRVDnuS4M/RUhCHQ7Qit5Dn6UYqpHCPje3QaawH8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1776136419; c=relaxed/simple; bh=akOyNyISt6Rkx9V99cfwF1E10fplo7tgj4K9G7X7PJ0=; h=Message-ID:From:To:Cc:Subject:In-Reply-To:References:Date: MIME-Version:Content-Type; b=ZMnba+r9+dOHmf+CyJ8QvEvvmg7BEZ4NxmwemmiuIoja7v7F8HWwRgToP5fyOa32Fkzcxs4SsAGf+6Mf7hpbDPZVXPzxkoGED7hlev3TpvdPOjhSYZc7iYNMHNMU5+/paXPBobjTH/ncH5TzcZ73Q25HiDPY+jZjwhmySZe1vPY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=manguebit.org; spf=pass smtp.mailfrom=manguebit.org; dkim=pass (2048-bit key) header.d=manguebit.org header.i=@manguebit.org header.b=t8eAVXMu; arc=none smtp.client-ip=143.255.12.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=manguebit.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=manguebit.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=manguebit.org header.i=@manguebit.org header.b="t8eAVXMu" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=manguebit.org; s=dkim; h=Content-Type:MIME-Version:Date:References: In-Reply-To:Subject:Cc:To:From:Message-ID:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=3rR0D0UvNAXit50J7295ttnGV9xG3zEcvphJRyM6oiw=; b=t8eAVXMuSQfnAhqd4fI+PxiPxq ky/Lv4IX88sUpCqmuMUr4dZHq7HcTFvRr4hKgpwA+pYh0jHrXz5Qp6jhKeZKdBbNX2FbtT3JCJEz4 qs0CnhoODzd+JnYB0x9XOPdEvYFhUB0yZlVIqAxFC3nv+vLcKA8LfS8yAvyXGHj3HvT10hw1PGknB zlo2jIN8CJW9I7gs55yKX0YxXD6B2wHCYDAH5q74puhqcZs8yKumIziu3ddYIekBPcCIane7Mlmv8 EalYXLqfdrbaAdacutJekOBiBzziXL1Hyj2JERxM1fPwTTQda8Qkn2CPFaKDj+70pV7/fovx4cUsM ukW4Vfmg==; Received: from pc by mx1.manguebit.org with local (Exim 4.99.1) id 1wCUDR-000000007ld-1OaE; Tue, 14 Apr 2026 00:13:29 -0300 Message-ID: From: Paulo Alcantara To: Linus Torvalds , Steve French Cc: LKML , CIFS Subject: Re: [GIT PULL] smb3 client fixes In-Reply-To: References: Date: Tue, 14 Apr 2026 00:13:27 -0300 Precedence: bulk X-Mailing-List: linux-cifs@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain Linus Torvalds writes: > On Mon, 13 Apr 2026 at 15:06, Steve French 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 +#include #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 *);