linux-kernel - Re: [PATCH v2] serial: sh-sci: Missing uart_unregister_driver() on error in sci_probe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 11 Mar 2019 15:46:16 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Mao Wenan <maowenan@...wei.com>
Cc:     gregkh@...uxfoundation.org, jslaby@...e.com,
        linux-serial@...r.kernel.org, kernel-janitors@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] serial: sh-sci: Missing uart_unregister_driver() on
 error in sci_probe_single()

On Mon, Mar 11, 2019 at 05:51:15PM +0800, Mao Wenan wrote:
> Add the missing uart_unregister_driver() before return
> from sci_probe_single() in the error handling case.
> 
> Signed-off-by: Mao Wenan <maowenan@...wei.com>
> ---

Sorry, I didn't really look at the code when I saw the v1 patch.

There are other error paths, but actually the whole approach is wrong.
Please, read my google plus post about error handling:

https://plus.google.com/u/0/106378716002406849458/posts/1Ud9JbaYnPr

But then the other rule I didn't mention in that post which applies
here is that the error handling should "mirror" the allocation code
so if you have:

	if (foo) {
		ret = allocate_one();
		if (ret)
			return ret;
	}
	ret = allocate_two();
	if (ret)
		goto free_one;

The error handling should mirror the "if (foo) " condition.  Like this:

free_one:
	if (foo)
		free_one();

Even if you can do extra analysis and find that the "if (foo) " can
be removed, you should leave there, because the mirroring helps human
readers.

In this case, the code is doing:

drivers/tty/serial/sh-sci.c
  3259                  return -EBUSY;
  3260  
  3261          mutex_lock(&sci_uart_registration_lock);
  3262          if (!sci_uart_driver.state) {
                    ^^^^^^^^^^^^^^^^^^^^^^
  3263                  ret = uart_register_driver(&sci_uart_driver);
  3264                  if (ret) {
  3265                          mutex_unlock(&sci_uart_registration_lock);
  3266                          return ret;
  3267                  }
  3268          }
  3269          mutex_unlock(&sci_uart_registration_lock);
  3270  

We would have to mirror the "if (!sci_uart_driver.state) {" code.

But actually, we can't.

The first driver to hit this code is supposed to load the
sci_uart_driver.  We can't know if we are the last driver to stop using
the sci_uart_driver so we can't know if we can free it.  This looks like
a very ugly hack to me.  It should probably be using ref counters.

regards,
an carpenter