| Message ID | 20230718012437.1841868-1-wentao@uniontech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] cifs: fix charset issue in reconnection | expand |
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
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
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.
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.
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. >
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. > > >
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; }