linux-kernel - Re: [PATCH 3/3] staging: dgap: use dgap_release_remap() in dgap_cleanup

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:	Thu, 25 Sep 2014 10:50:33 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	DaeSeok Youn <daeseok.youn@...il.com>
Cc:	Lidza Louina <lidza.louina@...il.com>,
	Mark Hounschell <markh@...pro.net>,
	devel <devel@...verdev.osuosl.org>,
	Greg KH <gregkh@...uxfoundation.org>,
	driverdev-devel@...uxdriverproject.org,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] staging: dgap: use dgap_release_remap() in
 dgap_cleanup_board()

On Thu, Sep 25, 2014 at 09:26:47AM +0900, DaeSeok Youn wrote:
> Hi,
> 
> 2014-09-24 18:45 GMT+09:00 Dan Carpenter <dan.carpenter@...cle.com>:
> > On Tue, Sep 23, 2014 at 09:22:36AM +0900, Daeseok Youn wrote:
> >>  static void dgap_release_remap(struct board_t *brd)
> >>  {
> >> -     release_mem_region(brd->membase, 0x200000);
> >> -     release_mem_region(brd->membase + PCI_IO_OFFSET, 0x200000);
> >> -     iounmap(brd->re_map_membase);
> >> -     iounmap(brd->re_map_port);
> >> +     if (brd->re_map_membase) {
> >
> > I hate this if conditions.  Let's not complicate dgap_release_remap(),
> > let's fix the callers instead.  Especially dgap_init_module() is a
> > totally sucky function with bad unwinding.
> ok. then,
>    if (brd->re_map_membase && brd->re_map_port)
>         dgap_release_remap(brd);
> 
> right?

What I'm saying is there should be no if statement because we should
only call this function after the memory has been remapped so there
should be no if conditions in this function at all.

The problem is that dgap_init_module() doesn't unwind, it just tries to
free every variable without considering whether or not it was allocated.
It's the "One Err" approach to error handling where you just have one
error label that calls free on every single thing in the world.  It
*should* look more like this:

static int dgap_init_module(void)
{
        int rc;

        pr_info("%s, Digi International Part Number %s\n", DG_NAME, DG_PART);

        rc = dgap_start();
        if (rc)
                return rc;

        rc = pci_register_driver(&dgap_driver);
        if (rc)
                goto err_stop;

        rc = dgap_create_driver_sysfiles(&dgap_driver);
        if (rc)
                goto err_unregister;

        dgap_driver_state = DRIVER_READY;

        return 0;

err_unregister:   // <- note that the label names reflect where the label
                  //    is not where the goto is.  dgap_start() uses
                  //    wrong/confusing label names.
	pci_unregister_driver(&dgap_driver);
err_stop:
	dgap_stop();  // <- mirror image of dgap_start();

        return rc;
}

dgap_stop() is easy to write because it's based on the error handling
in dgap_start().

static void dgap_start(void)
{
        spin_lock_irqsave(&dgap_poll_lock, lock_flags);
        dgap_poll_stop = 1;
        spin_unlock_irqrestore(&dgap_poll_lock, lock_flags);

	del_timer_sync(&dgap_poll_timer);

	device_destroy(dgap_class, MKDEV(DIGI_DGAP_MAJOR, 0));
        class_destroy(dgap_class);
        unregister_chrdev(DIGI_DGAP_MAJOR, "dgap");
}

The existing dgap_init_module() "free every single thing in the entire
universe" approach is buggy because it tries to delete the sysfs files
before they have been allocated.  This is called a "One Err Bug" because
having just one error label makes the code too complicated for anyone to
understand.  You have to add all kinds of if conditions everywhere and
it's a mess.

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/