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: <CAHb8M2ABVCaoNR0m5mLmWv-ei60PDRu462QTqDgEJwAf63oteg@mail.gmail.com>
Date:	Wed, 28 May 2014 18:29:38 +0900
From:	DaeSeok Youn <daeseok.youn@...il.com>
To:	Dan Carpenter <dan.carpenter@...cle.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()

Hi, Dan

Please look at below my line comment.

2014-05-28 16:02 GMT+09:00 Dan Carpenter <dan.carpenter@...cle.com>:
> 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
Yes, you're right. There were no error handle for tty_port_register_device() and
dgap_create_tty_sysfs() in dgap_tty_register_ports(). I didn't catch it. :-(
It need to add error handlers for them, right?

> 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.
So remove "goto" in dgap_firmware_load() and add error handler in
dgap_tty_init() and dgap_tty_register_ports(), right?

I have a question of this. In case of this, how to complete the error handling?
I want to cleanup allocated resources from dgap_tty_register() when
dgap_tty_init() is failed.
(and also in case of failure of dgap_tty_register_ports())
I think it can be handled as your previous e-mail.
It can have a label name of "free_chan" in dgap_tty_init() but it also have
a cleanup for allocated resource from earlier functions like
dgap_tty_register().

Can I call some functions for cleanup allocated resource from the
earlier function in dgap_tty_init() with goto label?

>
> 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.
OK. I will look at the code with your comment.

Really thanks for kind explanation.
regards,
Daeseok Youn
>
> 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/