linux-kernel - Re: [PATCH V2] staging: dgap: implement proper error handling in dgap_firmware

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]

Message-ID: <20140528070233.GP17724@mwanda>
Date:	Wed, 28 May 2014 10:02:33 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	DaeSeok Youn <daeseok.youn@...il.com>
Cc:	devel <devel@...verdev.osuosl.org>,
	Lidza Louina <lidza.louina@...il.com>,
	driverdev-devel@...uxdriverproject.org,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Greg KH <gregkh@...uxfoundation.org>,
	Mark Hounschell <markh@...pro.net>
Subject: Re: [PATCH V2] staging: dgap: implement proper error handling in
 dgap_firmware_load()

We are talking about different things I think.  What I'm saying is that
there is the normal way to do error handling in the kernel.  That's with
a series of labels like this:

	...
	return 0;

err_free_ttys:
	free_ttys();
err_free_channels:
	free_channels();
err_free_brd:
	free_brd();

	return ret;

In this code there are no if statements unless absolutely needed because
of an matching if statement in the allocation code.  The label names
describe what happens at the label.  It is in reverse order from the way
the variables were allocated.

The other thing is that at the end of dgap_tty_register() we have
unwinding like this.

  1304  unregister_serial_drv:
  1305          tty_unregister_driver(brd->serial_driver);
  1306  free_print_drv:
  1307          put_tty_driver(brd->print_driver);
  1308  free_serial_drv:
  1309          put_tty_driver(brd->serial_driver);

We can add a tty_unregister_driver(brd->print_driver) and create a
dgap_tty_unregister().

static void dgap_tty_unregister(struct board_t *brd)
{
	tty_unregister_driver(brd->print_driver);
	tty_unregister_driver(brd->serial_driver);
	put_tty_driver(brd->print_driver);
	put_tty_driver(brd->serial_driver);
}

Very simple and nice.

If you have one label it makes the code too complicated and it causes
bugs.  We call them "one err bugs" because there is one error label.

In your patch it has:
+       dgap_tty_uninit(brd, false);

But it should only be "false" if dgap_tty_init() failed.  If
dgap_tty_register_ports() fails then it should be "true".  Another
problem is that as you say, the earlier function are allocating
resources like dgap_tty_register() but only the last two function calls
have a "goto err_cleanup;" so the error handling is incomplete.

To be honest, I think once dgap the code is cleaned up this error
handling will be easy to write.  We shouldn't have things like:
brd->dgap_major_serial_registered = TRUE; because we will know which
order things were registered and unregister them in the reverse order.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/