linux-kernel - Re: [DRIVER SUBMISSION] DRBD wants to go mainline

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:	Mon, 23 Jul 2007 22:59:25 +0200
From:	"Jesper Juhl" <jesper.juhl@...il.com>
To:	"Jens Axboe" <axboe@...nel.dk>, "Andrew Morton" <akpm@...l.org>,
	lkml <linux-kernel@...r.kernel.org>, drbd-user@...ts.linbit.com
Subject: Re: [DRIVER SUBMISSION] DRBD wants to go mainline

On 21/07/07, Lars Ellenberg <lars@...bit.com> wrote:
>
>         DRBD wants to go mainline.
>         please have a look at the "for-linus" branch of
>         git://git.drbd.org/home/git/linux-drbd.git.
>
A few more comments.

In drbd_main.c::after_state_ch() there's this code :

...
                if ( mdev->p_uuid ) {
                        kfree(mdev->p_uuid);
                        mdev->p_uuid = NULL;
                }
...

Since kfree(NULL) if a noop, that should just become :

...
                kfree(mdev->p_uuid);
                mdev->p_uuid = NULL;
...


Same for these bits in drbd_main.c::drbd_cleanup()

...
                        if(mdev->app_reads_hash) {
                                kfree(mdev->app_reads_hash);
                                mdev->app_reads_hash = NULL;
                        }
                        if ( mdev->p_uuid ) {
                                kfree(mdev->p_uuid);
                                mdev->p_uuid = NULL;
                        }
...

They should just become

...
                        kfree(mdev->app_reads_hash);
                        mdev->app_reads_hash = NULL;
                        kfree(mdev->p_uuid);
                        mdev->p_uuid = NULL;
...

And then there's drbd_main.c::drbd_new_device() :

...
                if(mdev->app_reads_hash) kfree(mdev->app_reads_hash);
...

That should just be

...
                kfree(mdev->app_reads_hash);
...

Btw; You shouldn't put multiple statements on the same line. That is
seen all over the place - like

        mdev = kzalloc(sizeof(drbd_dev),GFP_KERNEL);
        if(!mdev) goto Enomem;

That should be

        mdev = kzalloc(sizeof(drbd_dev),GFP_KERNEL);
        if(!mdev)
                goto Enomem;

There are lots of other examples.


Here are a few more pointless NULL checks before kfree() :

drbd_nl.c::drbd_nl_net_conf() :
...
        if(new_tl_hash) {
                if (mdev->tl_hash) kfree(mdev->tl_hash);
                mdev->tl_hash_s = mdev->net_conf->max_epoch_size/8;
                mdev->tl_hash = new_tl_hash;
        }

        if(new_ee_hash) {
                if (mdev->ee_hash) kfree(mdev->ee_hash);
                mdev->ee_hash_s = mdev->net_conf->max_buffers/8;
                mdev->ee_hash = new_ee_hash;
        }

        if ( mdev->cram_hmac_tfm ) {
                crypto_free_hash(mdev->cram_hmac_tfm);
        }
        mdev->cram_hmac_tfm = tfm;
...

should become :

...
        if(new_tl_hash) {
                kfree(mdev->tl_hash);
                mdev->tl_hash_s = mdev->net_conf->max_epoch_size/8;
                mdev->tl_hash = new_tl_hash;
        }

        if(new_ee_hash) {
                kfree(mdev->ee_hash);
                mdev->ee_hash_s = mdev->net_conf->max_buffers/8;
                mdev->ee_hash = new_ee_hash;
        }

        crypto_free_hash(mdev->cram_hmac_tfm);
        mdev->cram_hmac_tfm = tfm;
...

still in drbd_nl.c::drbd_nl_net_conf() :

...
  fail:
        if (tfm) crypto_free_hash(tfm);
        if (new_tl_hash) kfree(new_tl_hash);
        if (new_ee_hash) kfree(new_ee_hash);
        if (new_conf) kfree(new_conf);
...

should become

...
  fail:
        crypto_free_hash(tfm);
        kfree(new_tl_hash);
        kfree(new_ee_hash);
        kfree(new_conf);
...

drbd_nl.c::ensure_mdev() :

...
                        if(mdev->app_reads_hash) kfree(mdev->app_reads_hash);
..

becomes

...
                        kfree(mdev->app_reads_hash);
...


In drbd_receiver.c::receive_uuids() we find this :

...
        if ( mdev->p_uuid ) kfree(mdev->p_uuid);
...

which should be just

...
        kfree(mdev->p_uuid);
...

There is also drbd_receiver.c::drbd_disconnect() - where

...
        if ( mdev->p_uuid ) {
                kfree(mdev->p_uuid);
                mdev->p_uuid = NULL;
        }
...

needs to become just

...
        kfree(mdev->p_uuid);
        mdev->p_uuid = NULL;
...


And in drbd_receiver.c::drbd_do_auth()

...
 fail:
        if(peers_ch) kfree(peers_ch);
        if(response) kfree(response);
        if(right_response) kfree(right_response);

...

needs to be turned into

...
 fail:
        kfree(peers_ch);
        kfree(response);
        kfree(right_response);
...


drbd_req.c::drbd_make_request_common() also needs to get rid of

...
        if (b) kfree(b); /* if someone else has beaten us to it... */
...
  fail_and_free_req:
        if (b) kfree(b);
...


and just turn it into

...
        kfree(b); /* if someone else has beaten us to it... */
...
  fail_and_free_req:
        kfree(b);
...


-- 
Jesper Juhl <jesper.juhl@...il.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html
-
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/