* re: cifs: Avoid doing network I/O while holding cache lock
@ 2020-01-16 19:10 Colin Ian King
2020-01-16 22:06 ` Steve French
0 siblings, 1 reply; 2+ messages in thread
From: Colin Ian King @ 2020-01-16 19:10 UTC (permalink / raw)
To: Paulo Alcantara (SUSE), Steve French, linux-cifs, samba-technical
Cc: linux-kernel
Hi,
static analysis with Coverity has detected an issue with the following
commit:
commit 03535b72873ba74e80e6938b5f772cf5b07ca76b
Author: Paulo Alcantara (SUSE) <pc@cjr.nz>
Date: Wed Dec 4 17:38:03 2019 -0300
cifs: Avoid doing network I/O while holding cache lock
This commit removed a memset on the vol object, causing the issue as
reported below by Coverity:
1342static struct cifs_ses *find_root_ses(struct vol_info *vi,
1343 struct cifs_tcon *tcon,
1344 const char *path)
1345{
1346 char *rpath;
1347 int rc;
1348 struct cache_entry *ce;
1349 struct dfs_info3_param ref = {0};
1350 char *mdata = NULL, *devname = NULL;
1351 struct TCP_Server_Info *server;
1352 struct cifs_ses *ses;
1. var_decl: Declaring variable vol without initializer.
1353 struct smb_vol vol;
1354
1355 rpath = get_dfs_root(path);
2. Condition IS_ERR(rpath), taking false branch.
1356 if (IS_ERR(rpath))
1357 return ERR_CAST(rpath);
1358
1359 down_read(&htable_rw_lock);
1360
1361 ce = lookup_cache_entry(rpath, NULL);
3. Condition IS_ERR(ce), taking true branch.
1362 if (IS_ERR(ce)) {
1363 up_read(&htable_rw_lock);
1364 ses = ERR_CAST(ce);
4. Jumping to label out.
1365 goto out;
1366 }
1367
1368 rc = setup_referral(path, ce, &ref, get_tgt_name(ce));
1369 if (rc) {
1370 up_read(&htable_rw_lock);
1371 ses = ERR_PTR(rc);
1372 goto out;
1373 }
1374
1375 up_read(&htable_rw_lock);
1376
1377 mdata = cifs_compose_mount_options(vi->mntdata, rpath, &ref,
1378 &devname);
1379 free_dfs_info_param(&ref);
1380
1381 if (IS_ERR(mdata)) {
1382 ses = ERR_CAST(mdata);
1383 mdata = NULL;
1384 goto out;
1385 }
1386
1387 rc = cifs_setup_volume_info(&vol, mdata, devname, false);
1388 kfree(devname);
1389
1390 if (rc) {
1391 ses = ERR_PTR(rc);
1392 goto out;
1393 }
1394
1395 server = get_tcp_server(&vol);
1396 if (!server) {
1397 ses = ERR_PTR(-EHOSTDOWN);
1398 goto out;
1399 }
1400
1401 ses = cifs_get_smb_ses(server, &vol);
1402
1403out:
5. uninit_use_in_call: Using uninitialized value vol.username when
calling cifs_cleanup_volume_info_contents.
6. uninit_use_in_call: Using uninitialized value vol.password when
calling cifs_cleanup_volume_info_contents.
7. uninit_use_in_call: Using uninitialized value vol.domainname when
calling cifs_cleanup_volume_info_contents.
8. uninit_use_in_call: Using uninitialized value vol.UNC when
calling cifs_cleanup_volume_info_contents.
9. uninit_use_in_call: Using uninitialized value vol.iocharset when
calling cifs_cleanup_volume_info_contents.
Uninitialized pointer read
10. uninit_use_in_call: Using uninitialized value vol.prepath when
calling cifs_cleanup_volume_info_contents.
1404 cifs_cleanup_volume_info_contents(&vol);
1405 kfree(mdata);
1406 kfree(rpath);
The vol object is memset as a result of calling cifs_setup_volume_info()
but this is too late for the earlier jumps to the error cleanup path at
label out. I did think about putting this back but it adds an
unnecessary memset, a better fix may be return immediately or to exit
via the kfree(mdata) at the end of the function.
Not sure what is best, so I'm flagging this up as an issue that needs
fixing.
Colin
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: cifs: Avoid doing network I/O while holding cache lock
2020-01-16 19:10 cifs: Avoid doing network I/O while holding cache lock Colin Ian King
@ 2020-01-16 22:06 ` Steve French
0 siblings, 0 replies; 2+ messages in thread
From: Steve French @ 2020-01-16 22:06 UTC (permalink / raw)
To: Colin Ian King
Cc: Paulo Alcantara (SUSE),
Steve French, CIFS, samba-technical, linux-kernel
Added the one line patch from Paulo to address what Colin reported and
added a "Reported-by" for Colin mentioning Coverity. merged into
cifs-2.6.git for-next
On Thu, Jan 16, 2020 at 1:10 PM Colin Ian King <colin.king@canonical.com> wrote:
>
> Hi,
>
> static analysis with Coverity has detected an issue with the following
> commit:
>
> commit 03535b72873ba74e80e6938b5f772cf5b07ca76b
> Author: Paulo Alcantara (SUSE) <pc@cjr.nz>
> Date: Wed Dec 4 17:38:03 2019 -0300
>
> cifs: Avoid doing network I/O while holding cache lock
>
>
>
> This commit removed a memset on the vol object, causing the issue as
> reported below by Coverity:
>
> 1342static struct cifs_ses *find_root_ses(struct vol_info *vi,
> 1343 struct cifs_tcon *tcon,
> 1344 const char *path)
> 1345{
> 1346 char *rpath;
> 1347 int rc;
> 1348 struct cache_entry *ce;
> 1349 struct dfs_info3_param ref = {0};
> 1350 char *mdata = NULL, *devname = NULL;
> 1351 struct TCP_Server_Info *server;
> 1352 struct cifs_ses *ses;
>
> 1. var_decl: Declaring variable vol without initializer.
> 1353 struct smb_vol vol;
> 1354
> 1355 rpath = get_dfs_root(path);
>
> 2. Condition IS_ERR(rpath), taking false branch.
> 1356 if (IS_ERR(rpath))
> 1357 return ERR_CAST(rpath);
> 1358
> 1359 down_read(&htable_rw_lock);
> 1360
> 1361 ce = lookup_cache_entry(rpath, NULL);
>
> 3. Condition IS_ERR(ce), taking true branch.
> 1362 if (IS_ERR(ce)) {
> 1363 up_read(&htable_rw_lock);
> 1364 ses = ERR_CAST(ce);
>
> 4. Jumping to label out.
> 1365 goto out;
> 1366 }
> 1367
> 1368 rc = setup_referral(path, ce, &ref, get_tgt_name(ce));
> 1369 if (rc) {
> 1370 up_read(&htable_rw_lock);
> 1371 ses = ERR_PTR(rc);
> 1372 goto out;
> 1373 }
> 1374
> 1375 up_read(&htable_rw_lock);
> 1376
> 1377 mdata = cifs_compose_mount_options(vi->mntdata, rpath, &ref,
> 1378 &devname);
> 1379 free_dfs_info_param(&ref);
> 1380
> 1381 if (IS_ERR(mdata)) {
> 1382 ses = ERR_CAST(mdata);
> 1383 mdata = NULL;
> 1384 goto out;
> 1385 }
> 1386
> 1387 rc = cifs_setup_volume_info(&vol, mdata, devname, false);
> 1388 kfree(devname);
> 1389
> 1390 if (rc) {
> 1391 ses = ERR_PTR(rc);
> 1392 goto out;
> 1393 }
> 1394
> 1395 server = get_tcp_server(&vol);
> 1396 if (!server) {
> 1397 ses = ERR_PTR(-EHOSTDOWN);
> 1398 goto out;
> 1399 }
> 1400
> 1401 ses = cifs_get_smb_ses(server, &vol);
> 1402
> 1403out:
>
> 5. uninit_use_in_call: Using uninitialized value vol.username when
> calling cifs_cleanup_volume_info_contents.
>
> 6. uninit_use_in_call: Using uninitialized value vol.password when
> calling cifs_cleanup_volume_info_contents.
>
> 7. uninit_use_in_call: Using uninitialized value vol.domainname when
> calling cifs_cleanup_volume_info_contents.
>
> 8. uninit_use_in_call: Using uninitialized value vol.UNC when
> calling cifs_cleanup_volume_info_contents.
>
> 9. uninit_use_in_call: Using uninitialized value vol.iocharset when
> calling cifs_cleanup_volume_info_contents.
>
> Uninitialized pointer read
> 10. uninit_use_in_call: Using uninitialized value vol.prepath when
> calling cifs_cleanup_volume_info_contents.
>
> 1404 cifs_cleanup_volume_info_contents(&vol);
> 1405 kfree(mdata);
> 1406 kfree(rpath);
>
> The vol object is memset as a result of calling cifs_setup_volume_info()
> but this is too late for the earlier jumps to the error cleanup path at
> label out. I did think about putting this back but it adds an
> unnecessary memset, a better fix may be return immediately or to exit
> via the kfree(mdata) at the end of the function.
>
> Not sure what is best, so I'm flagging this up as an issue that needs
> fixing.
>
> Colin
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-01-16 22:06 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-16 19:10 cifs: Avoid doing network I/O while holding cache lock Colin Ian King
2020-01-16 22:06 ` Steve French
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).