[PATCH v2] cifs: fix charset issue in reconnection

[PATCH v2] cifs: fix charset issue in reconnection

[Winston Wen]

We need to specify charset, like "iocharset=utf-8", in mount options for
Chinese path if the nls_default don't support it, such as iso8859-1, the
default value for CONFIG_NLS_DEFAULT.

But now in reconnection the nls_default is used, instead of the one we
specified and used in mount, and this can lead to mount failure.

Signed-off-by: Winston Wen <wentao@uniontech.com>
Reviewed-by: Paulo Alcantara <pc@manguebit.com>
---
 fs/smb/client/cifsglob.h | 1 +
 fs/smb/client/cifssmb.c  | 3 +--
 fs/smb/client/connect.c  | 5 +++++
 fs/smb/client/misc.c     | 1 +
 fs/smb/client/smb2pdu.c  | 3 +--
 5 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index c9a87d0123ce..31cadf9b2a98 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -1062,6 +1062,7 @@ struct cifs_ses {
 	unsigned long chans_need_reconnect;
 	/* ========= end: protected by chan_lock ======== */
 	struct cifs_ses *dfs_root_ses;
+	struct nls_table *local_nls;
 };
 
 static inline bool
diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
index 9dee267f1893..25503f1a4fd2 100644
--- a/fs/smb/client/cifssmb.c
+++ b/fs/smb/client/cifssmb.c
@@ -129,7 +129,7 @@ cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
 	}
 	spin_unlock(&server->srv_lock);
 
-	nls_codepage = load_nls_default();
+	nls_codepage = ses->local_nls;
 
 	/*
 	 * need to prevent multiple threads trying to simultaneously
@@ -200,7 +200,6 @@ cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
 		rc = -EAGAIN;
 	}
 
-	unload_nls(nls_codepage);
 	return rc;
 }
 
diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 5dd09c6d762e..bec0e893be06 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -1842,6 +1842,10 @@ static int match_session(struct cifs_ses *ses, struct smb3_fs_context *ctx)
 			    CIFS_MAX_PASSWORD_LEN))
 			return 0;
 	}
+
+	if (strcmp(ctx->local_nls->charset, ses->local_nls->charset))
+		return 0;
+
 	return 1;
 }
 
@@ -2286,6 +2290,7 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
 
 	ses->sectype = ctx->sectype;
 	ses->sign = ctx->sign;
+	ses->local_nls = load_nls(ctx->local_nls->charset);
 
 	/* add server as first channel */
 	spin_lock(&ses->chan_lock);
diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
index 70dbfe6584f9..d7e85d9a2655 100644
--- a/fs/smb/client/misc.c
+++ b/fs/smb/client/misc.c
@@ -95,6 +95,7 @@ sesInfoFree(struct cifs_ses *buf_to_free)
 		return;
 	}
 
+	unload_nls(buf_to_free->local_nls);
 	atomic_dec(&sesInfoAllocCount);
 	kfree(buf_to_free->serverOS);
 	kfree(buf_to_free->serverDomain);
diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
index e04766fe6f80..a457f07f820d 100644
--- a/fs/smb/client/smb2pdu.c
+++ b/fs/smb/client/smb2pdu.c
@@ -242,7 +242,7 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
 	}
 	spin_unlock(&server->srv_lock);
 
-	nls_codepage = load_nls_default();
+	nls_codepage = ses->local_nls;
 
 	/*
 	 * need to prevent multiple threads trying to simultaneously
@@ -324,7 +324,6 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
 		rc = -EAGAIN;
 	}
 failed:
-	unload_nls(nls_codepage);
 	return rc;
 }
 
-- 
2.40.1

Re: [PATCH v2] cifs: fix charset issue in reconnection

[kernel test robot]

Hi Winston,

kernel test robot noticed the following build warnings:

[auto build test WARNING on cifs/for-next]
[also build test WARNING on linus/master v6.5-rc2 next-20230718]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Winston-Wen/cifs-fix-charset-issue-in-reconnection/20230718-195832
base:   git://git.samba.org/sfrench/cifs-2.6.git for-next
patch link:    https://lore.kernel.org/r/20230718012437.1841868-1-wentao%40uniontech.com
patch subject: [PATCH v2] cifs: fix charset issue in reconnection
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230719/202307190007.EIqqUg1i-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230719/202307190007.EIqqUg1i-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202307190007.EIqqUg1i-lkp@intel.com/

All warnings (new ones prefixed by >>):

   fs/smb/client/connect.c: In function 'cifs_get_smb_ses':
>> fs/smb/client/connect.c:2293:49: warning: passing argument 1 of 'load_nls' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
    2293 |         ses->local_nls = load_nls(ctx->local_nls->charset);
         |                                   ~~~~~~~~~~~~~~^~~~~~~~~
   In file included from fs/smb/client/cifsproto.h:10,
                    from fs/smb/client/connect.c:37:
   include/linux/nls.h:50:35: note: expected 'char *' but argument is of type 'const char *'
      50 | extern struct nls_table *load_nls(char *);
         |                                   ^~~~~~


vim +2293 fs/smb/client/connect.c

  2190	
  2191	/**
  2192	 * cifs_get_smb_ses - get a session matching @ctx data from @server
  2193	 * @server: server to setup the session to
  2194	 * @ctx: superblock configuration context to use to setup the session
  2195	 *
  2196	 * This function assumes it is being called from cifs_mount() where we
  2197	 * already got a server reference (server refcount +1). See
  2198	 * cifs_get_tcon() for refcount explanations.
  2199	 */
  2200	struct cifs_ses *
  2201	cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
  2202	{
  2203		int rc = 0;
  2204		unsigned int xid;
  2205		struct cifs_ses *ses;
  2206		struct sockaddr_in *addr = (struct sockaddr_in *)&server->dstaddr;
  2207		struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *)&server->dstaddr;
  2208	
  2209		xid = get_xid();
  2210	
  2211		ses = cifs_find_smb_ses(server, ctx);
  2212		if (ses) {
  2213			cifs_dbg(FYI, "Existing smb sess found (status=%d)\n",
  2214				 ses->ses_status);
  2215	
  2216			spin_lock(&ses->chan_lock);
  2217			if (cifs_chan_needs_reconnect(ses, server)) {
  2218				spin_unlock(&ses->chan_lock);
  2219				cifs_dbg(FYI, "Session needs reconnect\n");
  2220	
  2221				mutex_lock(&ses->session_mutex);
  2222				rc = cifs_negotiate_protocol(xid, ses, server);
  2223				if (rc) {
  2224					mutex_unlock(&ses->session_mutex);
  2225					/* problem -- put our ses reference */
  2226					cifs_put_smb_ses(ses);
  2227					free_xid(xid);
  2228					return ERR_PTR(rc);
  2229				}
  2230	
  2231				rc = cifs_setup_session(xid, ses, server,
  2232							ctx->local_nls);
  2233				if (rc) {
  2234					mutex_unlock(&ses->session_mutex);
  2235					/* problem -- put our reference */
  2236					cifs_put_smb_ses(ses);
  2237					free_xid(xid);
  2238					return ERR_PTR(rc);
  2239				}
  2240				mutex_unlock(&ses->session_mutex);
  2241	
  2242				spin_lock(&ses->chan_lock);
  2243			}
  2244			spin_unlock(&ses->chan_lock);
  2245	
  2246			/* existing SMB ses has a server reference already */
  2247			cifs_put_tcp_session(server, 0);
  2248			free_xid(xid);
  2249			return ses;
  2250		}
  2251	
  2252		rc = -ENOMEM;
  2253	
  2254		cifs_dbg(FYI, "Existing smb sess not found\n");
  2255		ses = sesInfoAlloc();
  2256		if (ses == NULL)
  2257			goto get_ses_fail;
  2258	
  2259		/* new SMB session uses our server ref */
  2260		ses->server = server;
  2261		if (server->dstaddr.ss_family == AF_INET6)
  2262			sprintf(ses->ip_addr, "%pI6", &addr6->sin6_addr);
  2263		else
  2264			sprintf(ses->ip_addr, "%pI4", &addr->sin_addr);
  2265	
  2266		if (ctx->username) {
  2267			ses->user_name = kstrdup(ctx->username, GFP_KERNEL);
  2268			if (!ses->user_name)
  2269				goto get_ses_fail;
  2270		}
  2271	
  2272		/* ctx->password freed at unmount */
  2273		if (ctx->password) {
  2274			ses->password = kstrdup(ctx->password, GFP_KERNEL);
  2275			if (!ses->password)
  2276				goto get_ses_fail;
  2277		}
  2278		if (ctx->domainname) {
  2279			ses->domainName = kstrdup(ctx->domainname, GFP_KERNEL);
  2280			if (!ses->domainName)
  2281				goto get_ses_fail;
  2282		}
  2283	
  2284		strscpy(ses->workstation_name, ctx->workstation_name, sizeof(ses->workstation_name));
  2285	
  2286		if (ctx->domainauto)
  2287			ses->domainAuto = ctx->domainauto;
  2288		ses->cred_uid = ctx->cred_uid;
  2289		ses->linux_uid = ctx->linux_uid;
  2290	
  2291		ses->sectype = ctx->sectype;
  2292		ses->sign = ctx->sign;
> 2293		ses->local_nls = load_nls(ctx->local_nls->charset);
  2294	
  2295		/* add server as first channel */
  2296		spin_lock(&ses->chan_lock);
  2297		ses->chans[0].server = server;
  2298		ses->chan_count = 1;
  2299		ses->chan_max = ctx->multichannel ? ctx->max_channels:1;
  2300		ses->chans_need_reconnect = 1;
  2301		spin_unlock(&ses->chan_lock);
  2302	
  2303		mutex_lock(&ses->session_mutex);
  2304		rc = cifs_negotiate_protocol(xid, ses, server);
  2305		if (!rc)
  2306			rc = cifs_setup_session(xid, ses, server, ctx->local_nls);
  2307		mutex_unlock(&ses->session_mutex);
  2308	
  2309		/* each channel uses a different signing key */
  2310		spin_lock(&ses->chan_lock);
  2311		memcpy(ses->chans[0].signkey, ses->smb3signingkey,
  2312		       sizeof(ses->smb3signingkey));
  2313		spin_unlock(&ses->chan_lock);
  2314	
  2315		if (rc)
  2316			goto get_ses_fail;
  2317	
  2318		/*
  2319		 * success, put it on the list and add it as first channel
  2320		 * note: the session becomes active soon after this. So you'll
  2321		 * need to lock before changing something in the session.
  2322		 */
  2323		spin_lock(&cifs_tcp_ses_lock);
  2324		ses->dfs_root_ses = ctx->dfs_root_ses;
  2325		if (ses->dfs_root_ses)
  2326			ses->dfs_root_ses->ses_count++;
  2327		list_add(&ses->smb_ses_list, &server->smb_ses_list);
  2328		spin_unlock(&cifs_tcp_ses_lock);
  2329	
  2330		cifs_setup_ipc(ses, ctx);
  2331	
  2332		free_xid(xid);
  2333	
  2334		return ses;
  2335	
  2336	get_ses_fail:
  2337		sesInfoFree(ses);
  2338		free_xid(xid);
  2339		return ERR_PTR(rc);
  2340	}
  2341	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2] cifs: fix charset issue in reconnection

[kernel test robot]

Hi Winston,

kernel test robot noticed the following build errors:

[auto build test ERROR on cifs/for-next]
[also build test ERROR on linus/master v6.5-rc2 next-20230718]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Winston-Wen/cifs-fix-charset-issue-in-reconnection/20230718-195832
base:   git://git.samba.org/sfrench/cifs-2.6.git for-next
patch link:    https://lore.kernel.org/r/20230718012437.1841868-1-wentao%40uniontech.com
patch subject: [PATCH v2] cifs: fix charset issue in reconnection
config: i386-randconfig-i011-20230718 (https://download.01.org/0day-ci/archive/20230719/202307190835.WUgCXW8a-lkp@intel.com/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce: (https://download.01.org/0day-ci/archive/20230719/202307190835.WUgCXW8a-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202307190835.WUgCXW8a-lkp@intel.com/

All errors (new ones prefixed by >>):

>> fs/smb/client/connect.c:2293:28: error: passing 'const char *' to parameter of type 'char *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
           ses->local_nls = load_nls(ctx->local_nls->charset);
                                     ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/nls.h:50:41: note: passing argument to parameter here
   extern struct nls_table *load_nls(char *);
                                           ^
   1 error generated.


vim +2293 fs/smb/client/connect.c

  2190	
  2191	/**
  2192	 * cifs_get_smb_ses - get a session matching @ctx data from @server
  2193	 * @server: server to setup the session to
  2194	 * @ctx: superblock configuration context to use to setup the session
  2195	 *
  2196	 * This function assumes it is being called from cifs_mount() where we
  2197	 * already got a server reference (server refcount +1). See
  2198	 * cifs_get_tcon() for refcount explanations.
  2199	 */
  2200	struct cifs_ses *
  2201	cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
  2202	{
  2203		int rc = 0;
  2204		unsigned int xid;
  2205		struct cifs_ses *ses;
  2206		struct sockaddr_in *addr = (struct sockaddr_in *)&server->dstaddr;
  2207		struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *)&server->dstaddr;
  2208	
  2209		xid = get_xid();
  2210	
  2211		ses = cifs_find_smb_ses(server, ctx);
  2212		if (ses) {
  2213			cifs_dbg(FYI, "Existing smb sess found (status=%d)\n",
  2214				 ses->ses_status);
  2215	
  2216			spin_lock(&ses->chan_lock);
  2217			if (cifs_chan_needs_reconnect(ses, server)) {
  2218				spin_unlock(&ses->chan_lock);
  2219				cifs_dbg(FYI, "Session needs reconnect\n");
  2220	
  2221				mutex_lock(&ses->session_mutex);
  2222				rc = cifs_negotiate_protocol(xid, ses, server);
  2223				if (rc) {
  2224					mutex_unlock(&ses->session_mutex);
  2225					/* problem -- put our ses reference */
  2226					cifs_put_smb_ses(ses);
  2227					free_xid(xid);
  2228					return ERR_PTR(rc);
  2229				}
  2230	
  2231				rc = cifs_setup_session(xid, ses, server,
  2232							ctx->local_nls);
  2233				if (rc) {
  2234					mutex_unlock(&ses->session_mutex);
  2235					/* problem -- put our reference */
  2236					cifs_put_smb_ses(ses);
  2237					free_xid(xid);
  2238					return ERR_PTR(rc);
  2239				}
  2240				mutex_unlock(&ses->session_mutex);
  2241	
  2242				spin_lock(&ses->chan_lock);
  2243			}
  2244			spin_unlock(&ses->chan_lock);
  2245	
  2246			/* existing SMB ses has a server reference already */
  2247			cifs_put_tcp_session(server, 0);
  2248			free_xid(xid);
  2249			return ses;
  2250		}
  2251	
  2252		rc = -ENOMEM;
  2253	
  2254		cifs_dbg(FYI, "Existing smb sess not found\n");
  2255		ses = sesInfoAlloc();
  2256		if (ses == NULL)
  2257			goto get_ses_fail;
  2258	
  2259		/* new SMB session uses our server ref */
  2260		ses->server = server;
  2261		if (server->dstaddr.ss_family == AF_INET6)
  2262			sprintf(ses->ip_addr, "%pI6", &addr6->sin6_addr);
  2263		else
  2264			sprintf(ses->ip_addr, "%pI4", &addr->sin_addr);
  2265	
  2266		if (ctx->username) {
  2267			ses->user_name = kstrdup(ctx->username, GFP_KERNEL);
  2268			if (!ses->user_name)
  2269				goto get_ses_fail;
  2270		}
  2271	
  2272		/* ctx->password freed at unmount */
  2273		if (ctx->password) {
  2274			ses->password = kstrdup(ctx->password, GFP_KERNEL);
  2275			if (!ses->password)
  2276				goto get_ses_fail;
  2277		}
  2278		if (ctx->domainname) {
  2279			ses->domainName = kstrdup(ctx->domainname, GFP_KERNEL);
  2280			if (!ses->domainName)
  2281				goto get_ses_fail;
  2282		}
  2283	
  2284		strscpy(ses->workstation_name, ctx->workstation_name, sizeof(ses->workstation_name));
  2285	
  2286		if (ctx->domainauto)
  2287			ses->domainAuto = ctx->domainauto;
  2288		ses->cred_uid = ctx->cred_uid;
  2289		ses->linux_uid = ctx->linux_uid;
  2290	
  2291		ses->sectype = ctx->sectype;
  2292		ses->sign = ctx->sign;
> 2293		ses->local_nls = load_nls(ctx->local_nls->charset);
  2294	
  2295		/* add server as first channel */
  2296		spin_lock(&ses->chan_lock);
  2297		ses->chans[0].server = server;
  2298		ses->chan_count = 1;
  2299		ses->chan_max = ctx->multichannel ? ctx->max_channels:1;
  2300		ses->chans_need_reconnect = 1;
  2301		spin_unlock(&ses->chan_lock);
  2302	
  2303		mutex_lock(&ses->session_mutex);
  2304		rc = cifs_negotiate_protocol(xid, ses, server);
  2305		if (!rc)
  2306			rc = cifs_setup_session(xid, ses, server, ctx->local_nls);
  2307		mutex_unlock(&ses->session_mutex);
  2308	
  2309		/* each channel uses a different signing key */
  2310		spin_lock(&ses->chan_lock);
  2311		memcpy(ses->chans[0].signkey, ses->smb3signingkey,
  2312		       sizeof(ses->smb3signingkey));
  2313		spin_unlock(&ses->chan_lock);
  2314	
  2315		if (rc)
  2316			goto get_ses_fail;
  2317	
  2318		/*
  2319		 * success, put it on the list and add it as first channel
  2320		 * note: the session becomes active soon after this. So you'll
  2321		 * need to lock before changing something in the session.
  2322		 */
  2323		spin_lock(&cifs_tcp_ses_lock);
  2324		ses->dfs_root_ses = ctx->dfs_root_ses;
  2325		if (ses->dfs_root_ses)
  2326			ses->dfs_root_ses->ses_count++;
  2327		list_add(&ses->smb_ses_list, &server->smb_ses_list);
  2328		spin_unlock(&cifs_tcp_ses_lock);
  2329	
  2330		cifs_setup_ipc(ses, ctx);
  2331	
  2332		free_xid(xid);
  2333	
  2334		return ses;
  2335	
  2336	get_ses_fail:
  2337		sesInfoFree(ses);
  2338		free_xid(xid);
  2339		return ERR_PTR(rc);
  2340	}
  2341	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2] cifs: fix charset issue in reconnection

[Winston Wen]

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

On Tue, 18 Jul 2023 10:42:27 -0500
Steve French <smfrench@gmail.com> wrote:

> I get compile warning:
> 
> /home/smfrench/cifs-2.6/fs/smb/client/connect.c: In function
> ‘cifs_get_smb_ses’:
> /home/smfrench/cifs-2.6/fs/smb/client/connect.c:2293:49: warning:
> passing argument 1 of ‘load_nls’ discards ‘const’ qualifier from
> pointer target type [-Wdiscarded-qualifiers]


>  2293 |         ses->local_nls = load_nls(ctx->local_nls->charset);

Hi Steve,

Sorry about the mistake I made. I updated the patch with a very small
change:

	ses->local_nls = load_nls((char *)ctx->local_nls->charset);

It works, but I'm not sure whether it's clean enough. 

Perhaps I should make a change to load_nls() to take a const char *
instead of char *? If this make sense, I'll do it soon.

Find the updated patch as attachment.

-- 
Thanks,
Winston

[-- Attachment #2: 0001-cifs-fix-charset-issue-in-reconnection.patch --]
[-- Type: text/x-patch, Size: 3464 bytes --]

From d9d010797ef790a599b2c8f1993f5b4d64f46352 Mon Sep 17 00:00:00 2001
From: Winston Wen <wentao@uniontech.com>
Date: Mon, 17 Jul 2023 08:51:50 +0800
Subject: [PATCH] cifs: fix charset issue in reconnection

We need to specify charset, like "iocharset=utf-8", in mount options for
Chinese path if the nls_default don't support it, such as iso8859-1, the
default value for CONFIG_NLS_DEFAULT.

But now in reconnection the nls_default is used, instead of the one we
specified and used in mount, and this can lead to mount failure.

Signed-off-by: Winston Wen <wentao@uniontech.com>
Reviewed-by: Paulo Alcantara <pc@manguebit.com>
---
 fs/smb/client/cifsglob.h | 1 +
 fs/smb/client/cifssmb.c  | 3 +--
 fs/smb/client/connect.c  | 5 +++++
 fs/smb/client/misc.c     | 1 +
 fs/smb/client/smb2pdu.c  | 3 +--
 5 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index c9a87d0123ce..31cadf9b2a98 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -1062,6 +1062,7 @@ struct cifs_ses {
 	unsigned long chans_need_reconnect;
 	/* ========= end: protected by chan_lock ======== */
 	struct cifs_ses *dfs_root_ses;
+	struct nls_table *local_nls;
 };
 
 static inline bool
diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
index 9dee267f1893..25503f1a4fd2 100644
--- a/fs/smb/client/cifssmb.c
+++ b/fs/smb/client/cifssmb.c
@@ -129,7 +129,7 @@ cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
 	}
 	spin_unlock(&server->srv_lock);
 
-	nls_codepage = load_nls_default();
+	nls_codepage = ses->local_nls;
 
 	/*
 	 * need to prevent multiple threads trying to simultaneously
@@ -200,7 +200,6 @@ cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
 		rc = -EAGAIN;
 	}
 
-	unload_nls(nls_codepage);
 	return rc;
 }
 
diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 5dd09c6d762e..bec0e893be06 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -1842,6 +1842,10 @@ static int match_session(struct cifs_ses *ses, struct smb3_fs_context *ctx)
 			    CIFS_MAX_PASSWORD_LEN))
 			return 0;
 	}
+
+	if (strcmp(ctx->local_nls->charset, ses->local_nls->charset))
+		return 0;
+
 	return 1;
 }
 
@@ -2286,6 +2290,7 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
 
 	ses->sectype = ctx->sectype;
 	ses->sign = ctx->sign;
+	ses->local_nls = load_nls(ctx->local_nls->charset);
 
 	/* add server as first channel */
 	spin_lock(&ses->chan_lock);
diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
index 70dbfe6584f9..d7e85d9a2655 100644
--- a/fs/smb/client/misc.c
+++ b/fs/smb/client/misc.c
@@ -95,6 +95,7 @@ sesInfoFree(struct cifs_ses *buf_to_free)
 		return;
 	}
 
+	unload_nls(buf_to_free->local_nls);
 	atomic_dec(&sesInfoAllocCount);
 	kfree(buf_to_free->serverOS);
 	kfree(buf_to_free->serverDomain);
diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
index e04766fe6f80..a457f07f820d 100644
--- a/fs/smb/client/smb2pdu.c
+++ b/fs/smb/client/smb2pdu.c
@@ -242,7 +242,7 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
 	}
 	spin_unlock(&server->srv_lock);
 
-	nls_codepage = load_nls_default();
+	nls_codepage = ses->local_nls;
 
 	/*
 	 * need to prevent multiple threads trying to simultaneously
@@ -324,7 +324,6 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
 		rc = -EAGAIN;
 	}
 failed:
-	unload_nls(nls_codepage);
 	return rc;
 }
 
-- 
2.40.1

Re: [PATCH v2] cifs: fix charset issue in reconnection

[Tom Talpey]

On 7/19/2023 12:31 AM, Winston Wen wrote:
> On Tue, 18 Jul 2023 10:42:27 -0500
> Steve French <smfrench@gmail.com> wrote:
> 
>> I get compile warning:
>>
>> /home/smfrench/cifs-2.6/fs/smb/client/connect.c: In function
>> ‘cifs_get_smb_ses’:
>> /home/smfrench/cifs-2.6/fs/smb/client/connect.c:2293:49: warning:
>> passing argument 1 of ‘load_nls’ discards ‘const’ qualifier from
>> pointer target type [-Wdiscarded-qualifiers]
> 
> 
>>   2293 |         ses->local_nls = load_nls(ctx->local_nls->charset);
> 
> Hi Steve,
> 
> Sorry about the mistake I made. I updated the patch with a very small
> change:
> 
> 	ses->local_nls = load_nls((char *)ctx->local_nls->charset);

I don't like this approach. Removing a const via casting is not
a good idea. It invites write faults or outright bugs down
the call stack.

Why is the charset not const itself?

Tom.

> It works, but I'm not sure whether it's clean enough.
> 
> Perhaps I should make a change to load_nls() to take a const char *
> instead of char *? If this make sense, I'll do it soon.
> 
> Find the updated patch as attachment.
> 

Re: [PATCH v2] cifs: fix charset issue in reconnection

[Winston Wen]

On Thu, 20 Jul 2023 10:32:46 -0400
Tom Talpey <tom@talpey.com> wrote:

> On 7/19/2023 12:31 AM, Winston Wen wrote:
> > On Tue, 18 Jul 2023 10:42:27 -0500
> > Steve French <smfrench@gmail.com> wrote:
> >   
> >> I get compile warning:
> >>
> >> /home/smfrench/cifs-2.6/fs/smb/client/connect.c: In function
> >> ‘cifs_get_smb_ses’:
> >> /home/smfrench/cifs-2.6/fs/smb/client/connect.c:2293:49: warning:
> >> passing argument 1 of ‘load_nls’ discards ‘const’ qualifier from
> >> pointer target type [-Wdiscarded-qualifiers]  
> > 
> >   
> >>   2293 |         ses->local_nls =
> >> load_nls(ctx->local_nls->charset);  
> > 
> > Hi Steve,
> > 
> > Sorry about the mistake I made. I updated the patch with a very
> > small change:
> > 
> > 	ses->local_nls = load_nls((char *)ctx->local_nls->charset);
> >  
> 
> I don't like this approach. Removing a const via casting is not
> a good idea. It invites write faults or outright bugs down
> the call stack.

Agreed, I've sent a patch that make load_nls() take a const parameter,
so we don't need to do the cast any more:

https://lore.kernel.org/linux-cifs/20230720063414.2546451-1-wentao@uniontech.com/T/#u

> 
> Why is the charset not const itself?
> 
> Tom.
> 
> > It works, but I'm not sure whether it's clean enough.
> > 
> > Perhaps I should make a change to load_nls() to take a const char *
> > instead of char *? If this make sense, I'll do it soon.
> > 
> > Find the updated patch as attachment.
> >   
> 



-- 
Thanks,
Winston

Re: [PATCH v2] cifs: fix charset issue in reconnection

[Winston Wen]

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

On Wed, 19 Jul 2023 12:31:17 +0800
Winston Wen <wentao@uniontech.com> wrote:

> On Tue, 18 Jul 2023 10:42:27 -0500
> Steve French <smfrench@gmail.com> wrote:
> 
> > I get compile warning:
> > 
> > /home/smfrench/cifs-2.6/fs/smb/client/connect.c: In function
> > ‘cifs_get_smb_ses’:
> > /home/smfrench/cifs-2.6/fs/smb/client/connect.c:2293:49: warning:
> > passing argument 1 of ‘load_nls’ discards ‘const’ qualifier from
> > pointer target type [-Wdiscarded-qualifiers]
> 
> 
> >  2293 |         ses->local_nls = load_nls(ctx->local_nls->charset);
> 
> Hi Steve,
> 
> Sorry about the mistake I made. I updated the patch with a very small
> change:
> 
> 	ses->local_nls = load_nls((char *)ctx->local_nls->charset);
> 
> It works, but I'm not sure whether it's clean enough. 
> 
> Perhaps I should make a change to load_nls() to take a const char *
> instead of char *? If this make sense, I'll do it soon.
> 
> Find the updated patch as attachment.
> 

Sorry I picked the wrong version, updated.


-- 
Thanks,
Winston

[-- Attachment #2: 0001-cifs-fix-charset-issue-in-reconnection.patch --]
[-- Type: text/x-patch, Size: 3472 bytes --]

From f63be0e643f59a2dee119c3cf996f02b79b5e73c Mon Sep 17 00:00:00 2001
From: Winston Wen <wentao@uniontech.com>
Date: Mon, 17 Jul 2023 08:51:50 +0800
Subject: [PATCH] cifs: fix charset issue in reconnection

We need to specify charset, like "iocharset=utf-8", in mount options for
Chinese path if the nls_default don't support it, such as iso8859-1, the
default value for CONFIG_NLS_DEFAULT.

But now in reconnection the nls_default is used, instead of the one we
specified and used in mount, and this can lead to mount failure.

Signed-off-by: Winston Wen <wentao@uniontech.com>
Reviewed-by: Paulo Alcantara <pc@manguebit.com>
---
 fs/smb/client/cifsglob.h | 1 +
 fs/smb/client/cifssmb.c  | 3 +--
 fs/smb/client/connect.c  | 5 +++++
 fs/smb/client/misc.c     | 1 +
 fs/smb/client/smb2pdu.c  | 3 +--
 5 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
index c9a87d0123ce..31cadf9b2a98 100644
--- a/fs/smb/client/cifsglob.h
+++ b/fs/smb/client/cifsglob.h
@@ -1062,6 +1062,7 @@ struct cifs_ses {
 	unsigned long chans_need_reconnect;
 	/* ========= end: protected by chan_lock ======== */
 	struct cifs_ses *dfs_root_ses;
+	struct nls_table *local_nls;
 };
 
 static inline bool
diff --git a/fs/smb/client/cifssmb.c b/fs/smb/client/cifssmb.c
index 9dee267f1893..25503f1a4fd2 100644
--- a/fs/smb/client/cifssmb.c
+++ b/fs/smb/client/cifssmb.c
@@ -129,7 +129,7 @@ cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
 	}
 	spin_unlock(&server->srv_lock);
 
-	nls_codepage = load_nls_default();
+	nls_codepage = ses->local_nls;
 
 	/*
 	 * need to prevent multiple threads trying to simultaneously
@@ -200,7 +200,6 @@ cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
 		rc = -EAGAIN;
 	}
 
-	unload_nls(nls_codepage);
 	return rc;
 }
 
diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 5dd09c6d762e..7efae6f6f3b0 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -1842,6 +1842,10 @@ static int match_session(struct cifs_ses *ses, struct smb3_fs_context *ctx)
 			    CIFS_MAX_PASSWORD_LEN))
 			return 0;
 	}
+
+	if (strcmp(ctx->local_nls->charset, ses->local_nls->charset))
+		return 0;
+
 	return 1;
 }
 
@@ -2286,6 +2290,7 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
 
 	ses->sectype = ctx->sectype;
 	ses->sign = ctx->sign;
+	ses->local_nls = load_nls((char *)ctx->local_nls->charset);
 
 	/* add server as first channel */
 	spin_lock(&ses->chan_lock);
diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
index 70dbfe6584f9..d7e85d9a2655 100644
--- a/fs/smb/client/misc.c
+++ b/fs/smb/client/misc.c
@@ -95,6 +95,7 @@ sesInfoFree(struct cifs_ses *buf_to_free)
 		return;
 	}
 
+	unload_nls(buf_to_free->local_nls);
 	atomic_dec(&sesInfoAllocCount);
 	kfree(buf_to_free->serverOS);
 	kfree(buf_to_free->serverDomain);
diff --git a/fs/smb/client/smb2pdu.c b/fs/smb/client/smb2pdu.c
index e04766fe6f80..a457f07f820d 100644
--- a/fs/smb/client/smb2pdu.c
+++ b/fs/smb/client/smb2pdu.c
@@ -242,7 +242,7 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
 	}
 	spin_unlock(&server->srv_lock);
 
-	nls_codepage = load_nls_default();
+	nls_codepage = ses->local_nls;
 
 	/*
 	 * need to prevent multiple threads trying to simultaneously
@@ -324,7 +324,6 @@ smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
 		rc = -EAGAIN;
 	}
 failed:
-	unload_nls(nls_codepage);
 	return rc;
 }
 
-- 
2.40.1