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: <20140527102002.GI17724@mwanda>
Date:	Tue, 27 May 2014 13:20:02 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Daeseok Youn <daeseok.youn@...il.com>
Cc:	lidza.louina@...il.com, gregkh@...uxfoundation.org,
	markh@...pro.net, driverdev-devel@...uxdriverproject.org,
	devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2] staging: dgap: implement proper error handling in
 dgap_firmware_load()

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.

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/