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]

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

Hi, Dan.

2014-05-27 19:20 GMT+09:00 Dan Carpenter <dan.carpenter@...cle.com>:
> The "brd->nasync = 0;" was wrong, yes, but my main complaint was that
> you are writing complicated error handling.  This v2 patch makes the
> error handling even more complicated.  If dgap_tty_init() fails it
> should free the things it allocates itself, instead of the caller
> handling errors for it.
Yes, I knew dgap_tty_init() do only allocate some memory for channel.
But if one of function in dgap_firmware_load() is failed, it seems to
need all of resources in board.
Calling functions before calling dgap_tty_init() has memory
allocations and registration of tty related so when dgap_tty_init()
failed, all of resource in a board need to free and unregister.

And also, in dgap_cleanup_board() will free channels, so I called
dgap_cleanup_board() in err_cleanup label.

I understood your explanation but I didn't get why dgap_tty_uninit()
is not needed. Because If dgap_tty_init() is failed, and then
dgap_firmware_load() returns failure. That means it need to cleanup
previous allocation or registration before dgap_tty_init() is called.
example, dgap_tty_register() function.

If I'm wrong, please let me know.
Thanks.

regards,
Daeseok Youn.
>
> It's not actually that hard.  The only error handling we need to do in
> dgap_tty_init() is if the kzalloc() fails:
>
>   1374          /*
>   1375           * Allocate channel memory that might not have been allocated
>   1376           * when the driver was first loaded.
>   1377           */
>   1378          for (i = 0; i < brd->nasync; i++) {
>   1379                  if (!brd->channels[i]) {
>   1380                          brd->channels[i] =
>   1381                                  kzalloc(sizeof(struct channel_t), GFP_KERNEL);
>   1382                          if (!brd->channels[i])
>   1383                                  return -ENOMEM;
>
> Instead of returning directly here we should free the previous
> allocations.
>
>   1384                  }
>   1385          }
>
> The code is confusing because which ones did we allocate and which ones
> were already non-NULL at the start of the function?  In other words
> the "if (!brd->channels[i]) {" test?
>
> The answer is that the comment and the test seem to be wrong they were
> all NULL at the start of the function.  Just add a:
>
> free_chan:
>         while (--i >= 0) {
>                 kfree(brd->channels[i]);
>                 brd->channels[i] = NULL;
>         }
>         return ret;
>
> Actually, for these I would introduce an "int chan" variable just for
> that loop instead of "i" which we re-use.
>
> So then we remove the call to dgap_tty_uninit() from
> dgap_firmware_load().
>
> 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/