| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250617230142.ol3rc76uamwsd4rk@pali>
Date: Wed, 18 Jun 2025 01:01:42 +0200
From: Pali Rohár <pali@...nel.org>
To: Paulo Alcantara <pc@...guebit.org>
Cc: Steve French <sfrench@...ba.org>, Tom Talpey <tom@...pey.com>,
linux-cifs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] cifs: Show reason why autodisabling serverino support
On Tuesday 17 June 2025 19:23:15 Paulo Alcantara wrote:
> Pali Rohár <pali@...nel.org> writes:
>
> > Extend cifs_autodisable_serverino() function to print also text message why
> > the function was called.
> >
> > The text message is printed just once for mount then autodisabling
> > serverino support. Once the serverino support is disabled for mount it will
> > not be re-enabled. So those text messages do not cause flooding logs.
> >
> > This change allows to debug issues why cifs.ko decide to turn off server
> > inode number support and hence disable support for detection of hardlinks.
> >
> > Signed-off-by: Pali Rohár <pali@...nel.org>
> > ---
> > Paulo and Tom, could you check if this change is better now for you?
> > It should address problems with logs flooding and also information about
> > harlinks (it is already printed as can be seen also in this diff).
> > I would like to get your ACK, so I'm trying to improve it.
> > ---
> > fs/smb/client/cifsproto.h | 2 +-
> > fs/smb/client/connect.c | 2 +-
> > fs/smb/client/dfs_cache.c | 2 +-
> > fs/smb/client/inode.c | 6 +++---
> > fs/smb/client/misc.c | 6 +++++-
> > fs/smb/client/readdir.c | 4 ++--
> > 6 files changed, 13 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
> > index d550662b4e72..07a67c8c37ce 100644
> > --- a/fs/smb/client/cifsproto.h
> > +++ b/fs/smb/client/cifsproto.h
> > @@ -586,9 +586,9 @@ extern int cifs_do_set_acl(const unsigned int xid, struct cifs_tcon *tcon,
> > const struct nls_table *nls_codepage, int remap);
> > extern int CIFSGetExtAttr(const unsigned int xid, struct cifs_tcon *tcon,
> > const int netfid, __u64 *pExtAttrBits, __u64 *pMask);
> > #endif /* CIFS_ALLOW_INSECURE_LEGACY */
> > -extern void cifs_autodisable_serverino(struct cifs_sb_info *cifs_sb);
> > +extern void cifs_autodisable_serverino(struct cifs_sb_info *cifs_sb, const char *reason, int rc);
> > extern bool couldbe_mf_symlink(const struct cifs_fattr *fattr);
> > extern int check_mf_symlink(unsigned int xid, struct cifs_tcon *tcon,
> > struct cifs_sb_info *cifs_sb,
> > struct cifs_fattr *fattr,
> > diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
> > index 6bf04d9a5491..819721dfd5bb 100644
> > --- a/fs/smb/client/connect.c
> > +++ b/fs/smb/client/connect.c
> > @@ -3907,9 +3907,9 @@ int cifs_mount(struct cifs_sb_info *cifs_sb, struct smb3_fs_context *ctx)
> > /*
> > * After reconnecting to a different server, unique ids won't match anymore, so we disable
> > * serverino. This prevents dentry revalidation to think the dentry are stale (ESTALE).
> > */
> > - cifs_autodisable_serverino(cifs_sb);
> > + cifs_autodisable_serverino(cifs_sb, "Reconnecting to different server, inode numbers won't match anymore", 0);
>
> We are mounting an DFS share, not reconnecting. The message is
> misleading.
I mostly copied the comment above the cifs_autodisable_serverino() call.
Does it mean that the existing comment about reconnecting is wrong too?
> > /*
> > * Force the use of prefix path to support failover on DFS paths that resolve to targets
> > * that have different prefix paths.
> > */
> > diff --git a/fs/smb/client/dfs_cache.c b/fs/smb/client/dfs_cache.c
> > index 4dada26d56b5..c3fe85c31e2b 100644
> > --- a/fs/smb/client/dfs_cache.c
> > +++ b/fs/smb/client/dfs_cache.c
> > @@ -1288,9 +1288,9 @@ int dfs_cache_remount_fs(struct cifs_sb_info *cifs_sb)
> > /*
> > * After reconnecting to a different server, unique ids won't match anymore, so we disable
> > * serverino. This prevents dentry revalidation to think the dentry are stale (ESTALE).
> > */
> > - cifs_autodisable_serverino(cifs_sb);
> > + cifs_autodisable_serverino(cifs_sb, "Reconnecting to different server, inode numbers won't match anymore", 0);
>
> Ditto.
>
> > /*
> > * Force the use of prefix path to support failover on DFS paths that resolve to targets
> > * that have different prefix paths.
> > */
> > diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
> > index cd06598eacbd..b1c6e3986278 100644
> > --- a/fs/smb/client/inode.c
> > +++ b/fs/smb/client/inode.c
> > @@ -1076,9 +1076,9 @@ static void cifs_set_fattr_ino(int xid, struct cifs_tcon *tcon, struct super_blo
> > if (*inode)
> > fattr->cf_uniqueid = CIFS_I(*inode)->uniqueid;
> > else {
> > fattr->cf_uniqueid = iunique(sb, ROOT_I);
> > - cifs_autodisable_serverino(cifs_sb);
> > + cifs_autodisable_serverino(cifs_sb, "Cannot retrieve inode number via get_srv_inum", rc);
>
> Looks good.
>
> > }
> > return;
> > }
> >
> > @@ -1529,9 +1529,9 @@ cifs_iget(struct super_block *sb, struct cifs_fattr *fattr)
> > if (fattr->cf_flags & CIFS_FATTR_INO_COLLISION) {
> > fattr->cf_flags &= ~CIFS_FATTR_INO_COLLISION;
> >
> > if (inode_has_hashed_dentries(inode)) {
> > - cifs_autodisable_serverino(CIFS_SB(sb));
> > + cifs_autodisable_serverino(CIFS_SB(sb), "Inode number collision detected", 0);
>
> Looks good.
>
> > iput(inode);
> > fattr->cf_uniqueid = iunique(sb, ROOT_I);
> > goto retry_iget5_locked;
> > }
> > @@ -1596,9 +1596,9 @@ struct inode *cifs_root_iget(struct super_block *sb)
> > iget_root:
> > if (!rc) {
> > if (fattr.cf_flags & CIFS_FATTR_JUNCTION) {
> > fattr.cf_flags &= ~CIFS_FATTR_JUNCTION;
> > - cifs_autodisable_serverino(cifs_sb);
> > + cifs_autodisable_serverino(cifs_sb, "Cannot retrieve attributes for junction point", rc);
>
> This has nothing to do with not being able to retrieve attributes. It
> is simply disabling 'serverino' to prevent inode collisions with
> surrogate reparse points (automounts). This should also printed with
> FYI.
Ok. So then I misunderstood the code around. Do you know when exactly
can this case happen? And it is really a problem? Because name surrogate
reparse point already creates a new mount hierarchy for which is
generated new st_dev major and minor numbers and hence inode collisions
should not happen (they do not share st_dev anymore).
> > }
> > inode = cifs_iget(sb, &fattr);
> > }
> >
> > diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
> > index e77017f47084..409277883e8a 100644
> > --- a/fs/smb/client/misc.c
> > +++ b/fs/smb/client/misc.c
> > @@ -551,9 +551,9 @@ dump_smb(void *buf, int smb_buf_length)
> > smb_buf_length, true);
> > }
> >
> > void
> > -cifs_autodisable_serverino(struct cifs_sb_info *cifs_sb)
> > +cifs_autodisable_serverino(struct cifs_sb_info *cifs_sb, const char *reason, int rc)
> > {
> > if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM) {
> > struct cifs_tcon *tcon = NULL;
> >
> > @@ -561,8 +561,12 @@ cifs_autodisable_serverino(struct cifs_sb_info *cifs_sb)
> > tcon = cifs_sb_master_tcon(cifs_sb);
> >
> > cifs_sb->mnt_cifs_flags &= ~CIFS_MOUNT_SERVER_INUM;
> > cifs_sb->mnt_cifs_serverino_autodisabled = true;
> > + if (rc)
> > + cifs_dbg(VFS, "%s: %d\n", reason, rc);
> > + else
> > + cifs_dbg(VFS, "%s\n", reason);
> > cifs_dbg(VFS, "Autodisabling the use of server inode numbers on %s\n",
> > tcon ? tcon->tree_name : "new server");
> > cifs_dbg(VFS, "The server doesn't seem to support them properly or the files might be on different servers (DFS)\n");
> > cifs_dbg(VFS, "Hardlinks will not be recognized on this mount. Consider mounting with the \"noserverino\" option to silence this message.\n");
> > diff --git a/fs/smb/client/readdir.c b/fs/smb/client/readdir.c
> > index 787d6bcb5d1d..06e90921f751 100644
> > --- a/fs/smb/client/readdir.c
> > +++ b/fs/smb/client/readdir.c
> > @@ -412,9 +412,9 @@ _initiate_cifs_search(const unsigned int xid, struct file *file,
> > if (rc == 0) {
> > cifsFile->invalidHandle = false;
> > } else if ((rc == -EOPNOTSUPP) &&
> > (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM)) {
> > - cifs_autodisable_serverino(cifs_sb);
> > + cifs_autodisable_serverino(cifs_sb, "Cannot retrieve inode number via query_dir_first", rc);
>
> Looks good.
>
> > goto ffirst_retry;
> > }
> > error_exit:
> > cifs_put_tlink(tlink);
> > @@ -1006,9 +1006,9 @@ static int cifs_filldir(char *find_entry, struct file *file,
> > if (de.ino && (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM)) {
> > fattr.cf_uniqueid = de.ino;
> > } else {
> > fattr.cf_uniqueid = iunique(sb, ROOT_I);
> > - cifs_autodisable_serverino(cifs_sb);
> > + cifs_autodisable_serverino(cifs_sb, "Cannot retrieve inode number", 0);
>
> Perhaps also mention which function it wasn't able to retrieve inode
> number from like above?
I quickly look at this code around and I was not able to figure out what
is that function which was not able to retrieve inode. So I did not
write it into the message. Do you know, or could you figure out what is
that function / callback?
Powered by blists - more mailing lists