[PATCH] cifs: On cifs_reconnect, resolve the hostname again.

[PATCH] cifs: On cifs_reconnect, resolve the hostname again.

[Shyam Prasad N]

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

Hi Steve,

Please consider the attached patch for performing the DNS query again
on reconnect.
This is important when connecting to Azure file shares. The UNC
generally contains the server name as a FQDN, and the IP address which
the name resolves to can change over time.

After our last conversation about this, I discovered that for the
non-DFS scenario, we never do DNS resolutions in cifs.ko, since
mount.cifs already resolves the name and passes the "addr=" arg during
mount.

Hi Paulo,

I noticed that you had a patch for this long back. But I don't see
that call happening in the latest code. Any idea why that was done?

========================
commit 28eb24ff75c5ac130eb326b3b4d0dcecfc0f427d
Author: Paulo Alcantara <paulo@paulo.ac>
Date:   Tue Nov 20 15:16:36 2018 -0200

    cifs: Always resolve hostname before reconnecting

    In case a hostname resolves to a different IP address (e.g. long
    running mounts), make sure to resolve it every time prior to calling
    generic_ip_connect() in reconnect.

    Suggested-by: Steve French <stfrench@microsoft.com>
    Signed-off-by: Paulo Alcantara <palcantara@suse.de>
    Signed-off-by: Steve French <stfrench@microsoft.com>
=========================

-- 
Regards,
Shyam

[-- Attachment #2: 0001-cifs-On-cifs_reconnect-resolve-the-hostname-again.patch --]
[-- Type: application/octet-stream, Size: 1393 bytes --]

From 289f7f0fa229ea181094821c309a2ba9358791a3 Mon Sep 17 00:00:00 2001
From: Shyam Prasad N <sprasad@microsoft.com>
Date: Wed, 31 Mar 2021 14:35:24 +0000
Subject: [PATCH] cifs: On cifs_reconnect, resolve the hostname again.

On cifs_reconnect, make sure that DNS resolution happens again.
It could be the cause of connection to go dead in the first place.

Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
 fs/cifs/connect.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index eec8a2052da2..3db3006bbb47 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -321,14 +321,29 @@ cifs_reconnect(struct TCP_Server_Info *server)
 #endif
 
 #ifdef CONFIG_CIFS_DFS_UPCALL
+		if (cifs_sb && cifs_sb->origin_fullpath)
 			/*
 			 * Set up next DFS target server (if any) for reconnect. If DFS
 			 * feature is disabled, then we will retry last server we
 			 * connected to before.
 			 */
 			reconn_set_next_dfs_target(server, cifs_sb, &tgt_list, &tgt_it);
+		else {
+#endif
+			/*
+			 * Resolve the hostname again to make sure that IP address is up-to-date.
+			 */
+			rc = reconn_set_ipaddr_from_hostname(server);
+			if (rc) {
+				cifs_dbg(FYI, "%s: failed to resolve hostname: %d\n",
+						__func__, rc);
+			}
+
+#ifdef CONFIG_CIFS_DFS_UPCALL
+		}
 #endif
 
+
 #ifdef CONFIG_CIFS_SWN_UPCALL
 		}
 #endif
-- 
2.25.1

Re: [PATCH] cifs: On cifs_reconnect, resolve the hostname again.

[Paulo Alcantara]

Shyam Prasad N <nspmangalore@gmail.com> writes:

> Please consider the attached patch for performing the DNS query again
> on reconnect.
> This is important when connecting to Azure file shares. The UNC
> generally contains the server name as a FQDN, and the IP address which
> the name resolves to can change over time.
>
> After our last conversation about this, I discovered that for the
> non-DFS scenario, we never do DNS resolutions in cifs.ko, since
> mount.cifs already resolves the name and passes the "addr=" arg during
> mount.

Yeah, this should happen for both cases.  Good catch!

> I noticed that you had a patch for this long back. But I don't see
> that call happening in the latest code. Any idea why that was done?

I don't know.  Maybe some other patch broke it.

We should probably mark it for stable as well.

> From 289f7f0fa229ea181094821c309a2ba9358791a3 Mon Sep 17 00:00:00 2001
> From: Shyam Prasad N <sprasad@microsoft.com>
> Date: Wed, 31 Mar 2021 14:35:24 +0000
> Subject: [PATCH] cifs: On cifs_reconnect, resolve the hostname again.
>
> On cifs_reconnect, make sure that DNS resolution happens again.
> It could be the cause of connection to go dead in the first place.
>
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> ---
>  fs/cifs/connect.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)

This patch breaks when CONFIG_CIFS_DFS_UPCALL isn't set.

Please declare reconn_set_ipaddr_from_hostname() outside the "#ifdef
CONFIG_CIFS_DFS_UPCALL" in connect.c.

Otherwise,

Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>

Re: [PATCH] cifs: On cifs_reconnect, resolve the hostname again.

[Shyam Prasad N]

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

Hi Paulo,

Thanks for the quick review. And nice catch about CONFIG_CIFS_DFS_UPCALL.
Fixed it, added CC for stable.
Attached updated patch.

Regards,
Shyam

On Mon, Apr 5, 2021 at 9:24 PM Paulo Alcantara <pc@cjr.nz> wrote:
>
> Shyam Prasad N <nspmangalore@gmail.com> writes:
>
> > Please consider the attached patch for performing the DNS query again
> > on reconnect.
> > This is important when connecting to Azure file shares. The UNC
> > generally contains the server name as a FQDN, and the IP address which
> > the name resolves to can change over time.
> >
> > After our last conversation about this, I discovered that for the
> > non-DFS scenario, we never do DNS resolutions in cifs.ko, since
> > mount.cifs already resolves the name and passes the "addr=" arg during
> > mount.
>
> Yeah, this should happen for both cases.  Good catch!
>
> > I noticed that you had a patch for this long back. But I don't see
> > that call happening in the latest code. Any idea why that was done?
>
> I don't know.  Maybe some other patch broke it.
>
> We should probably mark it for stable as well.
>
> > From 289f7f0fa229ea181094821c309a2ba9358791a3 Mon Sep 17 00:00:00 2001
> > From: Shyam Prasad N <sprasad@microsoft.com>
> > Date: Wed, 31 Mar 2021 14:35:24 +0000
> > Subject: [PATCH] cifs: On cifs_reconnect, resolve the hostname again.
> >
> > On cifs_reconnect, make sure that DNS resolution happens again.
> > It could be the cause of connection to go dead in the first place.
> >
> > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > ---
> >  fs/cifs/connect.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
>
> This patch breaks when CONFIG_CIFS_DFS_UPCALL isn't set.
>
> Please declare reconn_set_ipaddr_from_hostname() outside the "#ifdef
> CONFIG_CIFS_DFS_UPCALL" in connect.c.
>
> Otherwise,
>
> Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>



-- 
Regards,
Shyam

[-- Attachment #2: 0001-cifs-On-cifs_reconnect-resolve-the-hostname-again.patch --]
[-- Type: application/octet-stream, Size: 2080 bytes --]

From 49e8df707b4464d47deb6be73d1252b0775ab02e Mon Sep 17 00:00:00 2001
From: Shyam Prasad N <sprasad@microsoft.com>
Date: Wed, 31 Mar 2021 14:35:24 +0000
Subject: [PATCH] cifs: On cifs_reconnect, resolve the hostname again.

On cifs_reconnect, make sure that DNS resolution happens again.
It could be the cause of connection to go dead in the first place.

Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
CC: <stable@vger.kernel.org>
---
 fs/cifs/connect.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index eec8a2052da2..24668eb006c6 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -87,7 +87,6 @@ static void cifs_prune_tlinks(struct work_struct *work);
  *
  * This should be called with server->srv_mutex held.
  */
-#ifdef CONFIG_CIFS_DFS_UPCALL
 static int reconn_set_ipaddr_from_hostname(struct TCP_Server_Info *server)
 {
 	int rc;
@@ -124,6 +123,7 @@ static int reconn_set_ipaddr_from_hostname(struct TCP_Server_Info *server)
 	return !rc ? -1 : 0;
 }
 
+#ifdef CONFIG_CIFS_DFS_UPCALL
 /* These functions must be called with server->srv_mutex held */
 static void reconn_set_next_dfs_target(struct TCP_Server_Info *server,
 				       struct cifs_sb_info *cifs_sb,
@@ -321,14 +321,29 @@ cifs_reconnect(struct TCP_Server_Info *server)
 #endif
 
 #ifdef CONFIG_CIFS_DFS_UPCALL
+		if (cifs_sb && cifs_sb->origin_fullpath)
 			/*
 			 * Set up next DFS target server (if any) for reconnect. If DFS
 			 * feature is disabled, then we will retry last server we
 			 * connected to before.
 			 */
 			reconn_set_next_dfs_target(server, cifs_sb, &tgt_list, &tgt_it);
+		else {
+#endif
+			/*
+			 * Resolve the hostname again to make sure that IP address is up-to-date.
+			 */
+			rc = reconn_set_ipaddr_from_hostname(server);
+			if (rc) {
+				cifs_dbg(FYI, "%s: failed to resolve hostname: %d\n",
+						__func__, rc);
+			}
+
+#ifdef CONFIG_CIFS_DFS_UPCALL
+		}
 #endif
 
+
 #ifdef CONFIG_CIFS_SWN_UPCALL
 		}
 #endif
-- 
2.25.1

Re: [PATCH] cifs: On cifs_reconnect, resolve the hostname again.

[Pavel Shilovsky]

Looks good!

Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>

CC: <stable@vger.kernel.org> # 5.11+

--
Best regards,
Pavel Shilovsky

пн, 5 апр. 2021 г. в 19:07, Shyam Prasad N <nspmangalore@gmail.com>:
>
> Hi Paulo,
>
> Thanks for the quick review. And nice catch about CONFIG_CIFS_DFS_UPCALL.
> Fixed it, added CC for stable.
> Attached updated patch.
>
> Regards,
> Shyam
>
> On Mon, Apr 5, 2021 at 9:24 PM Paulo Alcantara <pc@cjr.nz> wrote:
> >
> > Shyam Prasad N <nspmangalore@gmail.com> writes:
> >
> > > Please consider the attached patch for performing the DNS query again
> > > on reconnect.
> > > This is important when connecting to Azure file shares. The UNC
> > > generally contains the server name as a FQDN, and the IP address which
> > > the name resolves to can change over time.
> > >
> > > After our last conversation about this, I discovered that for the
> > > non-DFS scenario, we never do DNS resolutions in cifs.ko, since
> > > mount.cifs already resolves the name and passes the "addr=" arg during
> > > mount.
> >
> > Yeah, this should happen for both cases.  Good catch!
> >
> > > I noticed that you had a patch for this long back. But I don't see
> > > that call happening in the latest code. Any idea why that was done?
> >
> > I don't know.  Maybe some other patch broke it.
> >
> > We should probably mark it for stable as well.
> >
> > > From 289f7f0fa229ea181094821c309a2ba9358791a3 Mon Sep 17 00:00:00 2001
> > > From: Shyam Prasad N <sprasad@microsoft.com>
> > > Date: Wed, 31 Mar 2021 14:35:24 +0000
> > > Subject: [PATCH] cifs: On cifs_reconnect, resolve the hostname again.
> > >
> > > On cifs_reconnect, make sure that DNS resolution happens again.
> > > It could be the cause of connection to go dead in the first place.
> > >
> > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > > ---
> > >  fs/cifs/connect.c | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> >
> > This patch breaks when CONFIG_CIFS_DFS_UPCALL isn't set.
> >
> > Please declare reconn_set_ipaddr_from_hostname() outside the "#ifdef
> > CONFIG_CIFS_DFS_UPCALL" in connect.c.
> >
> > Otherwise,
> >
> > Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
>
>
>
> --
> Regards,
> Shyam

Re: [PATCH] cifs: On cifs_reconnect, resolve the hostname again.

[Steve French]

merged into cifs-2.6.git for-next

On Tue, Apr 6, 2021 at 11:34 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> Looks good!
>
> Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
>
> CC: <stable@vger.kernel.org> # 5.11+
>
> --
> Best regards,
> Pavel Shilovsky
>
> пн, 5 апр. 2021 г. в 19:07, Shyam Prasad N <nspmangalore@gmail.com>:
> >
> > Hi Paulo,
> >
> > Thanks for the quick review. And nice catch about CONFIG_CIFS_DFS_UPCALL.
> > Fixed it, added CC for stable.
> > Attached updated patch.
> >
> > Regards,
> > Shyam
> >
> > On Mon, Apr 5, 2021 at 9:24 PM Paulo Alcantara <pc@cjr.nz> wrote:
> > >
> > > Shyam Prasad N <nspmangalore@gmail.com> writes:
> > >
> > > > Please consider the attached patch for performing the DNS query again
> > > > on reconnect.
> > > > This is important when connecting to Azure file shares. The UNC
> > > > generally contains the server name as a FQDN, and the IP address which
> > > > the name resolves to can change over time.
> > > >
> > > > After our last conversation about this, I discovered that for the
> > > > non-DFS scenario, we never do DNS resolutions in cifs.ko, since
> > > > mount.cifs already resolves the name and passes the "addr=" arg during
> > > > mount.
> > >
> > > Yeah, this should happen for both cases.  Good catch!
> > >
> > > > I noticed that you had a patch for this long back. But I don't see
> > > > that call happening in the latest code. Any idea why that was done?
> > >
> > > I don't know.  Maybe some other patch broke it.
> > >
> > > We should probably mark it for stable as well.
> > >
> > > > From 289f7f0fa229ea181094821c309a2ba9358791a3 Mon Sep 17 00:00:00 2001
> > > > From: Shyam Prasad N <sprasad@microsoft.com>
> > > > Date: Wed, 31 Mar 2021 14:35:24 +0000
> > > > Subject: [PATCH] cifs: On cifs_reconnect, resolve the hostname again.
> > > >
> > > > On cifs_reconnect, make sure that DNS resolution happens again.
> > > > It could be the cause of connection to go dead in the first place.
> > > >
> > > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > > > ---
> > > >  fs/cifs/connect.c | 15 +++++++++++++++
> > > >  1 file changed, 15 insertions(+)
> > >
> > > This patch breaks when CONFIG_CIFS_DFS_UPCALL isn't set.
> > >
> > > Please declare reconn_set_ipaddr_from_hostname() outside the "#ifdef
> > > CONFIG_CIFS_DFS_UPCALL" in connect.c.
> > >
> > > Otherwise,
> > >
> > > Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
> >
> >
> >
> > --
> > Regards,
> > Shyam



-- 
Thanks,

Steve

Re: [PATCH] cifs: On cifs_reconnect, resolve the hostname again.

[Shyam Prasad N]

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

The intel bot identified an issue with the earlier version of the fix,
when not compiled with DFS.
Here's an updated version with that fix too.

Regards,
Shyam

On Wed, Apr 7, 2021 at 9:13 AM Steve French <smfrench@gmail.com> wrote:
>
> merged into cifs-2.6.git for-next
>
> On Tue, Apr 6, 2021 at 11:34 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> >
> > Looks good!
> >
> > Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
> >
> > CC: <stable@vger.kernel.org> # 5.11+
> >
> > --
> > Best regards,
> > Pavel Shilovsky
> >
> > пн, 5 апр. 2021 г. в 19:07, Shyam Prasad N <nspmangalore@gmail.com>:
> > >
> > > Hi Paulo,
> > >
> > > Thanks for the quick review. And nice catch about CONFIG_CIFS_DFS_UPCALL.
> > > Fixed it, added CC for stable.
> > > Attached updated patch.
> > >
> > > Regards,
> > > Shyam
> > >
> > > On Mon, Apr 5, 2021 at 9:24 PM Paulo Alcantara <pc@cjr.nz> wrote:
> > > >
> > > > Shyam Prasad N <nspmangalore@gmail.com> writes:
> > > >
> > > > > Please consider the attached patch for performing the DNS query again
> > > > > on reconnect.
> > > > > This is important when connecting to Azure file shares. The UNC
> > > > > generally contains the server name as a FQDN, and the IP address which
> > > > > the name resolves to can change over time.
> > > > >
> > > > > After our last conversation about this, I discovered that for the
> > > > > non-DFS scenario, we never do DNS resolutions in cifs.ko, since
> > > > > mount.cifs already resolves the name and passes the "addr=" arg during
> > > > > mount.
> > > >
> > > > Yeah, this should happen for both cases.  Good catch!
> > > >
> > > > > I noticed that you had a patch for this long back. But I don't see
> > > > > that call happening in the latest code. Any idea why that was done?
> > > >
> > > > I don't know.  Maybe some other patch broke it.
> > > >
> > > > We should probably mark it for stable as well.
> > > >
> > > > > From 289f7f0fa229ea181094821c309a2ba9358791a3 Mon Sep 17 00:00:00 2001
> > > > > From: Shyam Prasad N <sprasad@microsoft.com>
> > > > > Date: Wed, 31 Mar 2021 14:35:24 +0000
> > > > > Subject: [PATCH] cifs: On cifs_reconnect, resolve the hostname again.
> > > > >
> > > > > On cifs_reconnect, make sure that DNS resolution happens again.
> > > > > It could be the cause of connection to go dead in the first place.
> > > > >
> > > > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > > > > ---
> > > > >  fs/cifs/connect.c | 15 +++++++++++++++
> > > > >  1 file changed, 15 insertions(+)
> > > >
> > > > This patch breaks when CONFIG_CIFS_DFS_UPCALL isn't set.
> > > >
> > > > Please declare reconn_set_ipaddr_from_hostname() outside the "#ifdef
> > > > CONFIG_CIFS_DFS_UPCALL" in connect.c.
> > > >
> > > > Otherwise,
> > > >
> > > > Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
> > >
> > >
> > >
> > > --
> > > Regards,
> > > Shyam
>
>
>
> --
> Thanks,
>
> Steve



-- 
Regards,
Shyam

[-- Attachment #2: 0001-cifs-On-cifs_reconnect-resolve-the-hostname-again.patch --]
[-- Type: application/octet-stream, Size: 3180 bytes --]

From 7b290d8f571be83b8c96fc08a9942111ff642550 Mon Sep 17 00:00:00 2001
From: Shyam Prasad N <sprasad@microsoft.com>
Date: Wed, 31 Mar 2021 14:35:24 +0000
Subject: [PATCH] cifs: On cifs_reconnect, resolve the hostname again.

On cifs_reconnect, make sure that DNS resolution happens again.
It could be the cause of connection to go dead in the first place.

This also contains the fix for a build issue identified by Intel bot.
Reported-by: kernel test robot <lkp@intel.com>

Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
CC: <stable@vger.kernel.org> # 5.11+
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/Makefile  |  5 +++--
 fs/cifs/connect.c | 17 ++++++++++++++++-
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/Makefile b/fs/cifs/Makefile
index 5213b20843b5..3ee3b7de4ded 100644
--- a/fs/cifs/Makefile
+++ b/fs/cifs/Makefile
@@ -10,13 +10,14 @@ cifs-y := trace.o cifsfs.o cifssmb.o cifs_debug.o connect.o dir.o file.o \
 	  cifs_unicode.o nterr.o cifsencrypt.o \
 	  readdir.o ioctl.o sess.o export.o smb1ops.o unc.o winucase.o \
 	  smb2ops.o smb2maperror.o smb2transport.o \
-	  smb2misc.o smb2pdu.o smb2inode.o smb2file.o cifsacl.o fs_context.o
+	  smb2misc.o smb2pdu.o smb2inode.o smb2file.o cifsacl.o fs_context.o \
+	  dns_resolve.o
 
 cifs-$(CONFIG_CIFS_XATTR) += xattr.o
 
 cifs-$(CONFIG_CIFS_UPCALL) += cifs_spnego.o
 
-cifs-$(CONFIG_CIFS_DFS_UPCALL) += dns_resolve.o cifs_dfs_ref.o dfs_cache.o
+cifs-$(CONFIG_CIFS_DFS_UPCALL) += cifs_dfs_ref.o dfs_cache.o
 
 cifs-$(CONFIG_CIFS_SWN_UPCALL) += netlink.o cifs_swn.o
 
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index eec8a2052da2..24668eb006c6 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -87,7 +87,6 @@ static void cifs_prune_tlinks(struct work_struct *work);
  *
  * This should be called with server->srv_mutex held.
  */
-#ifdef CONFIG_CIFS_DFS_UPCALL
 static int reconn_set_ipaddr_from_hostname(struct TCP_Server_Info *server)
 {
 	int rc;
@@ -124,6 +123,7 @@ static int reconn_set_ipaddr_from_hostname(struct TCP_Server_Info *server)
 	return !rc ? -1 : 0;
 }
 
+#ifdef CONFIG_CIFS_DFS_UPCALL
 /* These functions must be called with server->srv_mutex held */
 static void reconn_set_next_dfs_target(struct TCP_Server_Info *server,
 				       struct cifs_sb_info *cifs_sb,
@@ -321,14 +321,29 @@ cifs_reconnect(struct TCP_Server_Info *server)
 #endif
 
 #ifdef CONFIG_CIFS_DFS_UPCALL
+		if (cifs_sb && cifs_sb->origin_fullpath)
 			/*
 			 * Set up next DFS target server (if any) for reconnect. If DFS
 			 * feature is disabled, then we will retry last server we
 			 * connected to before.
 			 */
 			reconn_set_next_dfs_target(server, cifs_sb, &tgt_list, &tgt_it);
+		else {
+#endif
+			/*
+			 * Resolve the hostname again to make sure that IP address is up-to-date.
+			 */
+			rc = reconn_set_ipaddr_from_hostname(server);
+			if (rc) {
+				cifs_dbg(FYI, "%s: failed to resolve hostname: %d\n",
+						__func__, rc);
+			}
+
+#ifdef CONFIG_CIFS_DFS_UPCALL
+		}
 #endif
 
+
 #ifdef CONFIG_CIFS_SWN_UPCALL
 		}
 #endif
-- 
2.25.1

Re: [PATCH] cifs: On cifs_reconnect, resolve the hostname again.

[Steve French]

updated cifs-2.6.git for-next with newer version of this patch

On Wed, Apr 7, 2021 at 12:43 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> The intel bot identified an issue with the earlier version of the fix,
> when not compiled with DFS.
> Here's an updated version with that fix too.
>
> Regards,
> Shyam
>
> On Wed, Apr 7, 2021 at 9:13 AM Steve French <smfrench@gmail.com> wrote:
> >
> > merged into cifs-2.6.git for-next
> >
> > On Tue, Apr 6, 2021 at 11:34 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> > >
> > > Looks good!
> > >
> > > Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
> > >
> > > CC: <stable@vger.kernel.org> # 5.11+
> > >
> > > --
> > > Best regards,
> > > Pavel Shilovsky
> > >
> > > пн, 5 апр. 2021 г. в 19:07, Shyam Prasad N <nspmangalore@gmail.com>:
> > > >
> > > > Hi Paulo,
> > > >
> > > > Thanks for the quick review. And nice catch about CONFIG_CIFS_DFS_UPCALL.
> > > > Fixed it, added CC for stable.
> > > > Attached updated patch.
> > > >
> > > > Regards,
> > > > Shyam
> > > >
> > > > On Mon, Apr 5, 2021 at 9:24 PM Paulo Alcantara <pc@cjr.nz> wrote:
> > > > >
> > > > > Shyam Prasad N <nspmangalore@gmail.com> writes:
> > > > >
> > > > > > Please consider the attached patch for performing the DNS query again
> > > > > > on reconnect.
> > > > > > This is important when connecting to Azure file shares. The UNC
> > > > > > generally contains the server name as a FQDN, and the IP address which
> > > > > > the name resolves to can change over time.
> > > > > >
> > > > > > After our last conversation about this, I discovered that for the
> > > > > > non-DFS scenario, we never do DNS resolutions in cifs.ko, since
> > > > > > mount.cifs already resolves the name and passes the "addr=" arg during
> > > > > > mount.
> > > > >
> > > > > Yeah, this should happen for both cases.  Good catch!
> > > > >
> > > > > > I noticed that you had a patch for this long back. But I don't see
> > > > > > that call happening in the latest code. Any idea why that was done?
> > > > >
> > > > > I don't know.  Maybe some other patch broke it.
> > > > >
> > > > > We should probably mark it for stable as well.
> > > > >
> > > > > > From 289f7f0fa229ea181094821c309a2ba9358791a3 Mon Sep 17 00:00:00 2001
> > > > > > From: Shyam Prasad N <sprasad@microsoft.com>
> > > > > > Date: Wed, 31 Mar 2021 14:35:24 +0000
> > > > > > Subject: [PATCH] cifs: On cifs_reconnect, resolve the hostname again.
> > > > > >
> > > > > > On cifs_reconnect, make sure that DNS resolution happens again.
> > > > > > It could be the cause of connection to go dead in the first place.
> > > > > >
> > > > > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > > > > > ---
> > > > > >  fs/cifs/connect.c | 15 +++++++++++++++
> > > > > >  1 file changed, 15 insertions(+)
> > > > >
> > > > > This patch breaks when CONFIG_CIFS_DFS_UPCALL isn't set.
> > > > >
> > > > > Please declare reconn_set_ipaddr_from_hostname() outside the "#ifdef
> > > > > CONFIG_CIFS_DFS_UPCALL" in connect.c.
> > > > >
> > > > > Otherwise,
> > > > >
> > > > > Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
> > > >
> > > >
> > > >
> > > > --
> > > > Regards,
> > > > Shyam
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve
>
>
>
> --
> Regards,
> Shyam



-- 
Thanks,

Steve

Re: [PATCH] cifs: On cifs_reconnect, resolve the hostname again.

[Steve French]

It was missing a small piece (otherwise could have build failure if
dns_query not available)

$ git diff -a
diff --git a/fs/cifs/Kconfig b/fs/cifs/Kconfig
index fe03cbdae959..bf52e9326ebe 100644
--- a/fs/cifs/Kconfig
+++ b/fs/cifs/Kconfig
@@ -18,6 +18,7 @@ config CIFS
        select CRYPTO_AES
        select CRYPTO_LIB_DES
        select KEYS
+       select DNS_RESOLVER
        help
          This is the client VFS module for the SMB3 family of NAS protocols,
          (including support for the most recent, most secure dialect SMB3.1.1)
@@ -112,7 +113,6 @@ config CIFS_WEAK_PW_HASH
 config CIFS_UPCALL
        bool "Kerberos/SPNEGO advanced session setup"
        depends on CIFS
-       select DNS_RESOLVER
        help
          Enables an upcall mechanism for CIFS which accesses userspace helper
          utilities to provide SPNEGO packaged (RFC 4178) Kerberos tickets
@@ -179,7 +179,6 @@ config CIFS_DEBUG_DUMP_KEYS
 config CIFS_DFS_UPCALL
        bool "DFS feature support"
        depends on CIFS
-       select DNS_RESOLVER
        help
          Distributed File System (DFS) support is used to access shares
          transparently in an enterprise name space, even if the share

On Wed, Apr 7, 2021 at 2:00 PM Steve French <smfrench@gmail.com> wrote:
>
> updated cifs-2.6.git for-next with newer version of this patch
>
> On Wed, Apr 7, 2021 at 12:43 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
> >
> > The intel bot identified an issue with the earlier version of the fix,
> > when not compiled with DFS.
> > Here's an updated version with that fix too.
> >
> > Regards,
> > Shyam
> >
> > On Wed, Apr 7, 2021 at 9:13 AM Steve French <smfrench@gmail.com> wrote:
> > >
> > > merged into cifs-2.6.git for-next
> > >
> > > On Tue, Apr 6, 2021 at 11:34 AM Pavel Shilovsky <piastryyy@gmail.com> wrote:
> > > >
> > > > Looks good!
> > > >
> > > > Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
> > > >
> > > > CC: <stable@vger.kernel.org> # 5.11+
> > > >
> > > > --
> > > > Best regards,
> > > > Pavel Shilovsky
> > > >
> > > > пн, 5 апр. 2021 г. в 19:07, Shyam Prasad N <nspmangalore@gmail.com>:
> > > > >
> > > > > Hi Paulo,
> > > > >
> > > > > Thanks for the quick review. And nice catch about CONFIG_CIFS_DFS_UPCALL.
> > > > > Fixed it, added CC for stable.
> > > > > Attached updated patch.
> > > > >
> > > > > Regards,
> > > > > Shyam
> > > > >
> > > > > On Mon, Apr 5, 2021 at 9:24 PM Paulo Alcantara <pc@cjr.nz> wrote:
> > > > > >
> > > > > > Shyam Prasad N <nspmangalore@gmail.com> writes:
> > > > > >
> > > > > > > Please consider the attached patch for performing the DNS query again
> > > > > > > on reconnect.
> > > > > > > This is important when connecting to Azure file shares. The UNC
> > > > > > > generally contains the server name as a FQDN, and the IP address which
> > > > > > > the name resolves to can change over time.
> > > > > > >
> > > > > > > After our last conversation about this, I discovered that for the
> > > > > > > non-DFS scenario, we never do DNS resolutions in cifs.ko, since
> > > > > > > mount.cifs already resolves the name and passes the "addr=" arg during
> > > > > > > mount.
> > > > > >
> > > > > > Yeah, this should happen for both cases.  Good catch!
> > > > > >
> > > > > > > I noticed that you had a patch for this long back. But I don't see
> > > > > > > that call happening in the latest code. Any idea why that was done?
> > > > > >
> > > > > > I don't know.  Maybe some other patch broke it.
> > > > > >
> > > > > > We should probably mark it for stable as well.
> > > > > >
> > > > > > > From 289f7f0fa229ea181094821c309a2ba9358791a3 Mon Sep 17 00:00:00 2001
> > > > > > > From: Shyam Prasad N <sprasad@microsoft.com>
> > > > > > > Date: Wed, 31 Mar 2021 14:35:24 +0000
> > > > > > > Subject: [PATCH] cifs: On cifs_reconnect, resolve the hostname again.
> > > > > > >
> > > > > > > On cifs_reconnect, make sure that DNS resolution happens again.
> > > > > > > It could be the cause of connection to go dead in the first place.
> > > > > > >
> > > > > > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > > > > > > ---
> > > > > > >  fs/cifs/connect.c | 15 +++++++++++++++
> > > > > > >  1 file changed, 15 insertions(+)
> > > > > >
> > > > > > This patch breaks when CONFIG_CIFS_DFS_UPCALL isn't set.
> > > > > >
> > > > > > Please declare reconn_set_ipaddr_from_hostname() outside the "#ifdef
> > > > > > CONFIG_CIFS_DFS_UPCALL" in connect.c.
> > > > > >
> > > > > > Otherwise,
> > > > > >
> > > > > > Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Regards,
> > > > > Shyam
> > >
> > >
> > >
> > > --
> > > Thanks,
> > >
> > > Steve
> >
> >
> >
> > --
> > Regards,
> > Shyam
>
>
>
> --
> Thanks,
>
> Steve



-- 
Thanks,

Steve

Re: [PATCH] cifs: On cifs_reconnect, resolve the hostname again.

[Aurélien Aptel]

Shyam Prasad N <nspmangalore@gmail.com> writes:
> The intel bot identified an issue with the earlier version of the fix,
> when not compiled with DFS.
> Here's an updated version with that fix too.

Ah I guess that's why resolving wasn't done on reconnect outside of DFS:
it requires the DNS resolver which requires upcalling i.e. having
keys-utils, request-conf.conf and dns_resolver key installed and
properly configured on the system to be able to reconnect.

Maybe we should fallback to retrying the same ip if resolving isn't
available.

Cheers,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)

Re: [PATCH] cifs: On cifs_reconnect, resolve the hostname again.

[Shyam Prasad N]

Hi Aurélien,

That logic is already there.
If reconn_set_ipaddr_from_hostname() returns failure, we log it and
continue with the reconnect to the old server->dstaddr anyway,

Regards,
Shyam

On Thu, Apr 8, 2021 at 2:11 PM Aurélien Aptel <aaptel@suse.com> wrote:
>
> Shyam Prasad N <nspmangalore@gmail.com> writes:
> > The intel bot identified an issue with the earlier version of the fix,
> > when not compiled with DFS.
> > Here's an updated version with that fix too.
>
> Ah I guess that's why resolving wasn't done on reconnect outside of DFS:
> it requires the DNS resolver which requires upcalling i.e. having
> keys-utils, request-conf.conf and dns_resolver key installed and
> properly configured on the system to be able to reconnect.
>
> Maybe we should fallback to retrying the same ip if resolving isn't
> available.
>
> Cheers,
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
>


-- 
Regards,
Shyam