linux-kernel - Re: [PATCH 5/9] bio-integrity: fold bio_integrity_enabled to bio_integrity

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: <20170406114349.GC9312@infradead.org>
Date:   Thu, 6 Apr 2017 04:43:49 -0700
From:   Christoph Hellwig <hch@...radead.org>
To:     Dmitry Monakhov <dmonakhov@...nvz.org>
Cc:     linux-kernel@...r.kernel.org, linux-block@...r.kernel.org,
        martin.petersen@...cle.com
Subject: Re: [PATCH 5/9] bio-integrity: fold bio_integrity_enabled to
 bio_integrity_prep

> --- a/Documentation/block/data-integrity.txt
> +++ b/Documentation/block/data-integrity.txt
> @@ -202,9 +202,6 @@ will require extra work due to the application tag.
>        added.  It is up to the caller to ensure that the bio does not
>        change while I/O is in progress.
>  
> -      bio_integrity_prep() should only be called if
> -      bio_integrity_enabled() returned 1.
> -

This should grow a blurb that bio_integrity_prep will complete the bio
if an error occurs.

>  /**
> - * bio_integrity_prep - Prepare bio for integrity I/O
> + * bio_integrity_setup - Prepare bio for integrity I/O
>   * @bio:	bio to prepare
>   *
>   * Description: Allocates a buffer for integrity metadata, maps the
> @@ -269,7 +231,7 @@ static int bio_integrity_process(struct bio *bio,
>   * block device's integrity function.  In the READ case, the buffer
>   * will be prepared for DMA and a suitable end_io handler set up.
>   */
> -int bio_integrity_prep(struct bio *bio)
> +static int bio_integrity_setup(struct bio *bio)

Instead of renaming the function I'd suggest simply merging the two,
the end result will be smaller and more readable.

> +	if (bio_data_dir(bio) == READ && bi->profile->verify_fn != NULL &&
> +	    (bi->flags & BLK_INTEGRITY_VERIFY))
> +		goto need_prep;
> +
> +	if (bio_data_dir(bio) == WRITE && bi->profile->generate_fn != NULL &&
> +	    (bi->flags & BLK_INTEGRITY_GENERATE))
> +		goto need_prep;

I'd invert the conditions to avoid the goto:

	if (bio_data_dir(bio) == READ) {
		if (!bi->profile->verify_fn ||
		    !(bi->flags & BLK_INTEGRITY_VERIFY))
			return 0;
	} else {
		if (!bi->profile->generate_fn ||
		    !(bi->flags & BLK_INTEGRITY_GENERATE))
			return 0;
	}