Unexpected additional umh-based DNS lookup in 6.6.0

Unexpected additional umh-based DNS lookup in 6.6.0

[Eduard Bachmakov]

When attempting to mount (mount.cifs version 7.0) a share using

    $ mount -t cifs -o
vers=3.1.1,cred=/home/u/.secret.txt,uid=1000,gid=100
//smb.server.example.com/scans /home/u/mnt

it succeeds on 6.5.9:

    mount("//smb.server.example.com/scans", ".", "cifs", 0,
"ip=192.168.5.43,unc=\\\\smb.server.example.com\\scans,vers=3.1.1,uid=1000,gid=100,user=u,pass=mypassword")
= 0

but fails on 6.0.0:

    mount("//smb.server.example.com/scans", ".", "cifs", 0,
"ip=192.168.5.43,unc=\\\\smb.server.example.com\\scans,vers=3.1.1,uid=1000,gid=100,user=u,pass=mypassword")
= -1 ENOKEY (Required key not available)

(or ENOENT) though it still works with using the IP instead of the domain:

    mount("//192.168.5.43/scans", ".", "cifs", 0,
"ip=192.168.5.43,unc=\\\\192.168.5.43\\scans,vers=3.1.1,uid=1000,gid=100,user=u,pass=mypassword")
= 0

Based on my reading ever since 348a04a ("smb: client: get rid of dfs
code dep in namespace.c") dfs_mount_share() is now calling
dns_resolve_server_name_to_ip() early and unconditionally. This can be
verified on a running system by enabling dns_resolver logging (echo 1
| sudo tee /sys/module/dns_resolver/parameters/debug + watch dmesg).
An additional DNS lookup is attempted in 6.0.0 that previously wasn't.
My best guess is that ENOENT is "didn't work" and ENOKEY means "didn't
work but cached".

On my system the request-key mechanism is not set up so this fails.
I'm no expert on SMB so I don't know if things just happened to work
previously by me relying on a bug but this change broke my setup. Is
this expected?

Re: Unexpected additional umh-based DNS lookup in 6.6.0

[Paulo Alcantara]

[-- Attachment #1: Type: text/plain, Size: 1756 bytes --]

Eduard Bachmakov <e.bachmakov@gmail.com> writes:

> When attempting to mount (mount.cifs version 7.0) a share using
>
>     $ mount -t cifs -o
> vers=3.1.1,cred=/home/u/.secret.txt,uid=1000,gid=100
> //smb.server.example.com/scans /home/u/mnt
>
> it succeeds on 6.5.9:
>
>     mount("//smb.server.example.com/scans", ".", "cifs", 0,
> "ip=192.168.5.43,unc=\\\\smb.server.example.com\\scans,vers=3.1.1,uid=1000,gid=100,user=u,pass=mypassword")
> = 0
>
> but fails on 6.0.0:
>
>     mount("//smb.server.example.com/scans", ".", "cifs", 0,
> "ip=192.168.5.43,unc=\\\\smb.server.example.com\\scans,vers=3.1.1,uid=1000,gid=100,user=u,pass=mypassword")
> = -1 ENOKEY (Required key not available)
>
> (or ENOENT) though it still works with using the IP instead of the domain:
>
>     mount("//192.168.5.43/scans", ".", "cifs", 0,
> "ip=192.168.5.43,unc=\\\\192.168.5.43\\scans,vers=3.1.1,uid=1000,gid=100,user=u,pass=mypassword")
> = 0
>
> Based on my reading ever since 348a04a ("smb: client: get rid of dfs
> code dep in namespace.c") dfs_mount_share() is now calling
> dns_resolve_server_name_to_ip() early and unconditionally. This can be
> verified on a running system by enabling dns_resolver logging (echo 1
> | sudo tee /sys/module/dns_resolver/parameters/debug + watch dmesg).
> An additional DNS lookup is attempted in 6.0.0 that previously wasn't.
> My best guess is that ENOENT is "didn't work" and ENOKEY means "didn't
> work but cached".

Yes, this is a regression.  Commit assumed that there would be always a
dns_resolver key set up on kernels built with CONFIG_CIFS_DFS_UPCALL=y
so that cifs.ko could safely upcall to resolve UNC hostname regardless
whether mounting a DFS share or a regular share.

Could you please try attached patch?

Thanks.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 2715 bytes --]

diff --git a/fs/smb/client/dfs.c b/fs/smb/client/dfs.c
index 81b84151450d..a8a1d386da65 100644
--- a/fs/smb/client/dfs.c
+++ b/fs/smb/client/dfs.c
@@ -263,15 +263,23 @@ static int __dfs_mount_share(struct cifs_mount_ctx *mnt_ctx)
 	return rc;
 }
 
-/* Resolve UNC hostname in @ctx->source and set ip addr in @ctx->dstaddr */
+/*
+ * If @ctx->dfs_automount, then update @ctx->dstaddr earlier with the DFS root
+ * server from where we'll start following any referrals.  Otherwise rely on the
+ * value provided by mount(2) as the user might not have dns_resolver key set up
+ * and therefore failing to upcall to resolve UNC hostname under @ctx->source.
+ */
 static int update_fs_context_dstaddr(struct smb3_fs_context *ctx)
 {
 	struct sockaddr *addr = (struct sockaddr *)&ctx->dstaddr;
-	int rc;
+	int rc = 0;
 
-	rc = dns_resolve_server_name_to_ip(ctx->source, addr, NULL);
-	if (!rc)
-		cifs_set_port(addr, ctx->port);
+	if (!ctx->nodfs && ctx->dfs_automount) {
+		rc = dns_resolve_server_name_to_ip(ctx->source, addr, NULL);
+		if (!rc)
+			cifs_set_port(addr, ctx->port);
+		ctx->dfs_automount = false;
+	}
 	return rc;
 }
 
diff --git a/fs/smb/client/fs_context.h b/fs/smb/client/fs_context.h
index 9d8d34af0211..cf46916286d0 100644
--- a/fs/smb/client/fs_context.h
+++ b/fs/smb/client/fs_context.h
@@ -268,6 +268,7 @@ struct smb3_fs_context {
 	bool witness:1; /* use witness protocol */
 	char *leaf_fullpath;
 	struct cifs_ses *dfs_root_ses;
+	bool dfs_automount:1; /* set for dfs automount only */
 };
 
 extern const struct fs_parameter_spec smb3_fs_parameters[];
diff --git a/fs/smb/client/namespace.c b/fs/smb/client/namespace.c
index c8f5ed8a69f1..a6968573b775 100644
--- a/fs/smb/client/namespace.c
+++ b/fs/smb/client/namespace.c
@@ -117,6 +117,18 @@ cifs_build_devname(char *nodename, const char *prepath)
 	return dev;
 }
 
+static bool is_dfs_mount(struct dentry *dentry)
+{
+	struct cifs_sb_info *cifs_sb = CIFS_SB(dentry->d_sb);
+	struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
+	bool ret;
+
+	spin_lock(&tcon->tc_lock);
+	ret = !!tcon->origin_fullpath;
+	spin_unlock(&tcon->tc_lock);
+	return ret;
+}
+
 /* Return full path out of a dentry set for automount */
 static char *automount_fullpath(struct dentry *dentry, void *page)
 {
@@ -212,8 +224,9 @@ static struct vfsmount *cifs_do_automount(struct path *path)
 		ctx->source = NULL;
 		goto out;
 	}
-	cifs_dbg(FYI, "%s: ctx: source=%s UNC=%s prepath=%s\n",
-		 __func__, ctx->source, ctx->UNC, ctx->prepath);
+	ctx->dfs_automount = is_dfs_mount(mntpt);
+	cifs_dbg(FYI, "%s: ctx: source=%s UNC=%s prepath=%s dfs_automount=%d\n",
+		 __func__, ctx->source, ctx->UNC, ctx->prepath, ctx->dfs_automount);
 
 	mnt = fc_mount(fc);
 out:

Re: Unexpected additional umh-based DNS lookup in 6.6.0

[Eduard Bachmakov]

On Thu, Nov 9, 2023 at 4:43 AM Paulo Alcantara <pc@manguebit.com> wrote:
>
> Eduard Bachmakov <e.bachmakov@gmail.com> writes:
>
> > When attempting to mount (mount.cifs version 7.0) a share using
> >
> >     $ mount -t cifs -o
> > vers=3.1.1,cred=/home/u/.secret.txt,uid=1000,gid=100
> > //smb.server.example.com/scans /home/u/mnt
> >
> > it succeeds on 6.5.9:
> >
> >     mount("//smb.server.example.com/scans", ".", "cifs", 0,
> > "ip=192.168.5.43,unc=\\\\smb.server.example.com\\scans,vers=3.1.1,uid=1000,gid=100,user=u,pass=mypassword")
> > = 0
> >
> > but fails on 6.0.0:
> >
> >     mount("//smb.server.example.com/scans", ".", "cifs", 0,
> > "ip=192.168.5.43,unc=\\\\smb.server.example.com\\scans,vers=3.1.1,uid=1000,gid=100,user=u,pass=mypassword")
> > = -1 ENOKEY (Required key not available)
> >
> > (or ENOENT) though it still works with using the IP instead of the domain:
> >
> >     mount("//192.168.5.43/scans", ".", "cifs", 0,
> > "ip=192.168.5.43,unc=\\\\192.168.5.43\\scans,vers=3.1.1,uid=1000,gid=100,user=u,pass=mypassword")
> > = 0
> >
> > Based on my reading ever since 348a04a ("smb: client: get rid of dfs
> > code dep in namespace.c") dfs_mount_share() is now calling
> > dns_resolve_server_name_to_ip() early and unconditionally. This can be
> > verified on a running system by enabling dns_resolver logging (echo 1
> > | sudo tee /sys/module/dns_resolver/parameters/debug + watch dmesg).
> > An additional DNS lookup is attempted in 6.0.0 that previously wasn't.
> > My best guess is that ENOENT is "didn't work" and ENOKEY means "didn't
> > work but cached".
>
> Yes, this is a regression.  Commit assumed that there would be always a
> dns_resolver key set up on kernels built with CONFIG_CIFS_DFS_UPCALL=y
> so that cifs.ko could safely upcall to resolve UNC hostname regardless
> whether mounting a DFS share or a regular share.
>
> Could you please try attached patch?

Can confirm, this works. No more UMH DNS lookup, share is mounted successfully.

I don't see is_dfs_mount() being called so in my scenario we're simply
relying on kzalloc initializing dfs_automount to false.

> Thanks.

[PATCH] smb: client: fix mount when dns_resolver key is not available

[Paulo Alcantara]

There was a wrong assumption that with CONFIG_CIFS_DFS_UPCALL=y there
would always be a dns_resolver key set up so we could unconditionally
upcall to resolve UNC hostname rather than using the value provided by
mount(2).

Only require it when performing automount of junctions within a DFS
share so users that don't have dns_resolver key still can mount their
regular shares with server hostname resolved by mount.cifs(8).

Fixes: 348a04a8d113 ("smb: client: get rid of dfs code dep in namespace.c")
Tested-by: Eduard Bachmakov <e.bachmakov@gmail.com>
Reported-by: Eduard Bachmakov <e.bachmakov@gmail.com>
Closes: https://lore.kernel.org/all/CADCRUiNvZuiUZ0VGZZO9HRyPyw6x92kiA7o7Q4tsX5FkZqUkKg@mail.gmail.com/
Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
---
 fs/smb/client/dfs.c        | 18 +++++++++++++-----
 fs/smb/client/fs_context.h |  1 +
 fs/smb/client/namespace.c  | 17 +++++++++++++++--
 3 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/fs/smb/client/dfs.c b/fs/smb/client/dfs.c
index 81b84151450d..a8a1d386da65 100644
--- a/fs/smb/client/dfs.c
+++ b/fs/smb/client/dfs.c
@@ -263,15 +263,23 @@ static int __dfs_mount_share(struct cifs_mount_ctx *mnt_ctx)
 	return rc;
 }
 
-/* Resolve UNC hostname in @ctx->source and set ip addr in @ctx->dstaddr */
+/*
+ * If @ctx->dfs_automount, then update @ctx->dstaddr earlier with the DFS root
+ * server from where we'll start following any referrals.  Otherwise rely on the
+ * value provided by mount(2) as the user might not have dns_resolver key set up
+ * and therefore failing to upcall to resolve UNC hostname under @ctx->source.
+ */
 static int update_fs_context_dstaddr(struct smb3_fs_context *ctx)
 {
 	struct sockaddr *addr = (struct sockaddr *)&ctx->dstaddr;
-	int rc;
+	int rc = 0;
 
-	rc = dns_resolve_server_name_to_ip(ctx->source, addr, NULL);
-	if (!rc)
-		cifs_set_port(addr, ctx->port);
+	if (!ctx->nodfs && ctx->dfs_automount) {
+		rc = dns_resolve_server_name_to_ip(ctx->source, addr, NULL);
+		if (!rc)
+			cifs_set_port(addr, ctx->port);
+		ctx->dfs_automount = false;
+	}
 	return rc;
 }
 
diff --git a/fs/smb/client/fs_context.h b/fs/smb/client/fs_context.h
index 9d8d34af0211..cf46916286d0 100644
--- a/fs/smb/client/fs_context.h
+++ b/fs/smb/client/fs_context.h
@@ -268,6 +268,7 @@ struct smb3_fs_context {
 	bool witness:1; /* use witness protocol */
 	char *leaf_fullpath;
 	struct cifs_ses *dfs_root_ses;
+	bool dfs_automount:1; /* set for dfs automount only */
 };
 
 extern const struct fs_parameter_spec smb3_fs_parameters[];
diff --git a/fs/smb/client/namespace.c b/fs/smb/client/namespace.c
index c8f5ed8a69f1..a6968573b775 100644
--- a/fs/smb/client/namespace.c
+++ b/fs/smb/client/namespace.c
@@ -117,6 +117,18 @@ cifs_build_devname(char *nodename, const char *prepath)
 	return dev;
 }
 
+static bool is_dfs_mount(struct dentry *dentry)
+{
+	struct cifs_sb_info *cifs_sb = CIFS_SB(dentry->d_sb);
+	struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
+	bool ret;
+
+	spin_lock(&tcon->tc_lock);
+	ret = !!tcon->origin_fullpath;
+	spin_unlock(&tcon->tc_lock);
+	return ret;
+}
+
 /* Return full path out of a dentry set for automount */
 static char *automount_fullpath(struct dentry *dentry, void *page)
 {
@@ -212,8 +224,9 @@ static struct vfsmount *cifs_do_automount(struct path *path)
 		ctx->source = NULL;
 		goto out;
 	}
-	cifs_dbg(FYI, "%s: ctx: source=%s UNC=%s prepath=%s\n",
-		 __func__, ctx->source, ctx->UNC, ctx->prepath);
+	ctx->dfs_automount = is_dfs_mount(mntpt);
+	cifs_dbg(FYI, "%s: ctx: source=%s UNC=%s prepath=%s dfs_automount=%d\n",
+		 __func__, ctx->source, ctx->UNC, ctx->prepath, ctx->dfs_automount);
 
 	mnt = fc_mount(fc);
 out:
-- 
2.42.0

Re: Unexpected additional umh-based DNS lookup in 6.6.0

[Paulo Alcantara]

Eduard Bachmakov <e.bachmakov@gmail.com> writes:

> I don't see is_dfs_mount() being called so in my scenario we're simply
> relying on kzalloc initializing dfs_automount to false.

It's expected.  If your share is DFS and client finds a DFS link or
reparse mount point, the dentry set for automount will get mounted with
new fs context where where @dfs_automount will be set.

Re: Unexpected additional umh-based DNS lookup in 6.6.0

[Eduard Bachmakov]

I noticed this got pulled into 6.7. Given this is a user-facing
regression, can this be proposed for the next 6.6 point release?
Sorry, if this is already the case and I missed it.

On Thu, Nov 9, 2023 at 6:29 PM Paulo Alcantara <pc@manguebit.com> wrote:
>
> Eduard Bachmakov <e.bachmakov@gmail.com> writes:
>
> > I don't see is_dfs_mount() being called so in my scenario we're simply
> > relying on kzalloc initializing dfs_automount to false.
>
> It's expected.  If your share is DFS and client finds a DFS link or
> reparse mount point, the dentry set for automount will get mounted with
> new fs context where where @dfs_automount will be set.

I see, thanks.

Re: Unexpected additional umh-based DNS lookup in 6.6.0

[Steve French]

> can this be proposed for the next 6.6 point release?

I don't see any updates to the 6.6.x stable tree in the last week -
but since this patch is tagged for stable (and has "Fixes:" tag as
well).  It is likely to be picked up automatically.   When next update
to 6.6 stable tree is made (to include recent merge window patches),
hopefully by Monday - we should recheck.

On Tue, Nov 14, 2023 at 4:00 PM Eduard Bachmakov <e.bachmakov@gmail.com> wrote:
>
> I noticed this got pulled into 6.7. Given this is a user-facing
> regression, can this be proposed for the next 6.6 point release?
> Sorry, if this is already the case and I missed it.
>
> On Thu, Nov 9, 2023 at 6:29 PM Paulo Alcantara <pc@manguebit.com> wrote:
> >
> > Eduard Bachmakov <e.bachmakov@gmail.com> writes:
> >
> > > I don't see is_dfs_mount() being called so in my scenario we're simply
> > > relying on kzalloc initializing dfs_automount to false.
> >
> > It's expected.  If your share is DFS and client finds a DFS link or
> > reparse mount point, the dentry set for automount will get mounted with
> > new fs context where where @dfs_automount will be set.
>
> I see, thanks.
>


-- 
Thanks,

Steve

Re: Unexpected additional umh-based DNS lookup in 6.6.0

[Paulo Alcantara]

Stable team,

Eduard Bachmakov <e.bachmakov@gmail.com> writes:

> I noticed this got pulled into 6.7. Given this is a user-facing
> regression, can this be proposed for the next 6.6 point release?
> Sorry, if this is already the case and I missed it.

Could you please backport

        5e2fd17f434d ("smb: client: fix mount when dns_resolver key is not available")

to v6.6.y?

Thanks.

Re: Unexpected additional umh-based DNS lookup in 6.6.0

[Greg KH]

On Thu, Nov 23, 2023 at 10:56:53PM -0300, Paulo Alcantara wrote:
> Stable team,
> 
> Eduard Bachmakov <e.bachmakov@gmail.com> writes:
> 
> > I noticed this got pulled into 6.7. Given this is a user-facing
> > regression, can this be proposed for the next 6.6 point release?
> > Sorry, if this is already the case and I missed it.
> 
> Could you please backport
> 
>         5e2fd17f434d ("smb: client: fix mount when dns_resolver key is not available")
> 
> to v6.6.y?

Will do so now, thanks, I'm way behind in catching up with stable
patches due to the merge window and then Plumbers and now some
additional travel, sorry about that.

greg k-h