From: Joel Granados <j.granados@samsung.com>
To: Dan Carpenter <dan.carpenter@linaro.org>
Cc: <oe-kbuild@lists.linux.dev>, <lkp@intel.com>,
<oe-kbuild-all@lists.linux.dev>
Subject: Re: [joelgranados:jag/sysctl-testing-for-0day 2/6] drivers/parport/procfs.c:502 parport_proc_register() warn: possible memory leak of 't'
Date: Mon, 15 May 2023 20:59:17 +0200 [thread overview]
Message-ID: <20230515185917.sdxy2mszhmjyngn3@localhost> (raw)
In-Reply-To: <819e68aa-8c11-4f4d-9dcb-d20ef136fe0f@kili.mountain>
[-- Attachment #1: Type: text/plain, Size: 7652 bytes --]
On Mon, May 15, 2023 at 10:34:44AM +0300, Dan Carpenter wrote:
> tree: https://protect2.fireeye.com/v1/url?k=77a81f96-28332689-77a994d9-000babdfecba-20da5463c1bf339f&q=1&e=47a4ce4b-b7ad-4d3e-ba9e-cfb53e82e0b3&u=https%3A%2F%2Fgithub.com%2FJoelgranados%2Flinux.git jag/sysctl-testing-for-0day
> head: 947f41127456cd102688bcddc3002d03111668c1
> commit: 3756e44223c2862f95de9d4280b44987e0b3aafc [2/6] parport: Remove register_sysctl_table from parport_proc_register
> config: x86_64-randconfig-m001
> compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <error27@gmail.com>
> | Link: https://lore.kernel.org/r/202305150948.pHgIh7Ql-lkp@intel.com/
>
> smatch warnings:
> drivers/parport/procfs.c:502 parport_proc_register() warn: possible memory leak of 't'
> drivers/parport/procfs.c:509 parport_proc_register() warn: missing error code 'err'
>
> vim +/t +502 drivers/parport/procfs.c
>
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 468 int parport_proc_register(struct parport *port)
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 469 {
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 470 struct parport_sysctl_table *t;
> 3756e44223c286 Joel Granados 2023-05-12 471 char *tmp_dir_path;
> 3756e44223c286 Joel Granados 2023-05-12 472 size_t tmp_path_len, port_name_len;
> 3756e44223c286 Joel Granados 2023-05-12 473 int i, err = 0;
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 474
> 2451a84839187c Silviu-Mihai Popescu 2013-03-11 475 t = kmemdup(&parport_sysctl_template, sizeof(*t), GFP_KERNEL);
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 476 if (t == NULL)
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 477 return -ENOMEM;
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 478
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 479 t->device_dir[0].extra1 = port;
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 480
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 481 t->vars[0].data = &port->spintime;
> 3756e44223c286 Joel Granados 2023-05-12 482 for (i = 0; i < 5; i++) {
> 3756e44223c286 Joel Granados 2023-05-12 483 t->vars[i].extra1 = port;
> 3756e44223c286 Joel Granados 2023-05-12 484 t->vars[5 + i].extra2 = &port->probe_info[i];
> 3756e44223c286 Joel Granados 2023-05-12 485 }
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 486
> 3756e44223c286 Joel Granados 2023-05-12 487 port_name_len = strnlen(port->name, PARPORT_NAME_MAX_LEN);
> 3756e44223c286 Joel Granados 2023-05-12 488 /*
> 3756e44223c286 Joel Granados 2023-05-12 489 * Allocate a buffer for two paths: dev/parport/PORT and dev/parport/PORT/devices.
> 3756e44223c286 Joel Granados 2023-05-12 490 * We calculate for the second as that will give us enough for the first.
> 3756e44223c286 Joel Granados 2023-05-12 491 */
> 3756e44223c286 Joel Granados 2023-05-12 492 tmp_path_len = PARPORT_BASE_DEVICES_PATH_SIZE + port_name_len;
> 3756e44223c286 Joel Granados 2023-05-12 493 tmp_dir_path = kmalloc(tmp_path_len, GFP_KERNEL);
> 3756e44223c286 Joel Granados 2023-05-12 494 if (!tmp_dir_path) {
> 3756e44223c286 Joel Granados 2023-05-12 495 err = -ENOMEM;
> 3756e44223c286 Joel Granados 2023-05-12 496 goto exit_free_t;
> 3756e44223c286 Joel Granados 2023-05-12 497 }
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 498
> 3756e44223c286 Joel Granados 2023-05-12 499 if (tmp_path_len
> 3756e44223c286 Joel Granados 2023-05-12 500 <= snprintf(tmp_dir_path, tmp_path_len, "dev/parport/%s/devices", port->name)) {
> 3756e44223c286 Joel Granados 2023-05-12 501 kfree(tmp_dir_path);
> 3756e44223c286 Joel Granados 2023-05-12 @502 return -ENOENT;
>
> Should be goto exit_free_t;
oops. my bad. that should have been goto exit_free_t
>
> 3756e44223c286 Joel Granados 2023-05-12 503 }
> 3756e44223c286 Joel Granados 2023-05-12 504 if (register_sysctl(tmp_dir_path, t->device_dir) == NULL)
> 3756e44223c286 Joel Granados 2023-05-12 505 /*
> 3756e44223c286 Joel Granados 2023-05-12 506 * We keep the original behavior of parport where failure to register
> 3756e44223c286 Joel Granados 2023-05-12 507 * does not return error.
> 3756e44223c286 Joel Granados 2023-05-12 508 */
> 3756e44223c286 Joel Granados 2023-05-12 @509 goto exit_free_tmp_dir_path;
>
> Smatch wants a error code for this error path.
>
> The comments say that we are doing the wrong thing deliberately to
> maintain backwards compatibility and avoid breaking stuff. Fortunately,
> (LOL), no one is checking the returns from parport_proc_register() so we
> are free to return a correct error code without any worry about breakage.
Sigh. Indeed, the only place were this function is called is from
parport/share.c. Should we just change the function return value to
void?
>
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 510
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 511
> 3756e44223c286 Joel Granados 2023-05-12 512 tmp_path_len = PARPORT_BASE_PATH_SIZE + port_name_len;
> 3756e44223c286 Joel Granados 2023-05-12 513 if (tmp_path_len
> 3756e44223c286 Joel Granados 2023-05-12 514 <= snprintf(tmp_dir_path, tmp_path_len, "dev/parport/%s", port->name)) {
> 3756e44223c286 Joel Granados 2023-05-12 515 err = -ENOENT;
> 3756e44223c286 Joel Granados 2023-05-12 516 goto exit_free_tmp_dir_path;
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 517 }
> 3756e44223c286 Joel Granados 2023-05-12 518
> 3756e44223c286 Joel Granados 2023-05-12 519 t->sysctl_header = register_sysctl(tmp_dir_path, t->vars);
> 3756e44223c286 Joel Granados 2023-05-12 520 if (t->sysctl_header == NULL)
> 3756e44223c286 Joel Granados 2023-05-12 521 goto exit_free_tmp_dir_path;
>
> Same here. Return an error code.
Ack
>
> 3756e44223c286 Joel Granados 2023-05-12 522
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 523 port->sysctl_table = t;
> 3756e44223c286 Joel Granados 2023-05-12 524
> 3756e44223c286 Joel Granados 2023-05-12 525 kfree(tmp_dir_path);
> ^1da177e4c3f41 Linus Torvalds 2005-04-16 526 return 0;
> 3756e44223c286 Joel Granados 2023-05-12 527
> 3756e44223c286 Joel Granados 2023-05-12 528 exit_free_tmp_dir_path:
> 3756e44223c286 Joel Granados 2023-05-12 529 kfree(tmp_dir_path);
> 3756e44223c286 Joel Granados 2023-05-12 530 exit_free_t:
> 3756e44223c286 Joel Granados 2023-05-12 531 kfree(t);
> 3756e44223c286 Joel Granados 2023-05-12 532 t = NULL;
>
> t is a local variable. No point in setting it to NULL. (This was
> copied from the original code, I know, but since you're re-writing stuff
> you may as well delete the nonsense bits).
I was also wondering why it was like this. Will be changed in V2.
>
> 3756e44223c286 Joel Granados 2023-05-12 533 return err;
> 3756e44223c286 Joel Granados 2023-05-12 534
>
> --
> 0-DAY CI Kernel Test Service
> https://protect2.fireeye.com/v1/url?k=7917a300-268c9a1f-7916284f-000babdfecba-d19874df90955c77&q=1&e=47a4ce4b-b7ad-4d3e-ba9e-cfb53e82e0b3&u=https%3A%2F%2Fgithub.com%2Fintel%2Flkp-tests
--
Joel Granados
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
next prev parent reply other threads:[~2023-05-15 19:32 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20230515073454eucas1p13d3f249160454d835f3cefce3fa7a0eb@eucas1p1.samsung.com>
2023-05-15 7:34 ` [joelgranados:jag/sysctl-testing-for-0day 2/6] drivers/parport/procfs.c:502 parport_proc_register() warn: possible memory leak of 't' Dan Carpenter
2023-05-15 18:59 ` Joel Granados [this message]
2023-05-16 7:05 ` Dan Carpenter
2023-05-16 14:05 ` Joel Granados
2023-05-15 1:38 kernel test robot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230515185917.sdxy2mszhmjyngn3@localhost \
--to=j.granados@samsung.com \
--cc=dan.carpenter@linaro.org \
--cc=lkp@intel.com \
--cc=oe-kbuild-all@lists.linux.dev \
--cc=oe-kbuild@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.