linux-kernel - Re: [PATCH v2] NVMe: silence GCC warning on 32 bit

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: <alpine.LRH.2.03.1403050803090.2885@AMR>
Date:	Wed, 5 Mar 2014 08:09:34 -0700 (MST)
From:	Keith Busch <keith.busch@...el.com>
To:	Paul Bolle <pebolle@...cali.nl>
cc:	Matthew Wilcox <willy@...ux.intel.com>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	Keith Busch <keith.busch@...el.com>,
	linux-nvme@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] NVMe: silence GCC warning on 32 bit

Looks good to me. This won't apply in linux-nvme yet and it may be a
little while before it does, so this might be considered to go upstream
through a different tree if you want this in sooner.

On Tue, 4 Mar 2014, Paul Bolle wrote:
> Building nvme-core.o on 32 bit x86 triggers a rather impressive set of
> GCC warnings:
>    In file included from drivers/block/nvme-core.c:20:0:
>    drivers/block/nvme-core.c: In function 'nvme_submit_bio_queue':
>    include/linux/bio.h:154:55: warning: 'bvprv.bv_offset' may be used uninitialized in this function [-Wmaybe-uninitialized]
>     #define bvec_to_phys(bv) (page_to_phys((bv)->bv_page) + (unsigned long) (bv)->bv_offset)
>                                                           ^
>    drivers/block/nvme-core.c:498:23: note: 'bvprv.bv_offset' was declared here
>      struct bio_vec bvec, bvprv;
>                           ^
>    In file included from drivers/block/nvme-core.c:20:0:
>    include/linux/bio.h:154:55: warning: 'bvprv.bv_len' may be used uninitialized in this function [-Wmaybe-uninitialized]
>     #define bvec_to_phys(bv) (page_to_phys((bv)->bv_page) + (unsigned long) (bv)->bv_offset)
>                                                           ^
>    drivers/block/nvme-core.c:498:23: note: 'bvprv.bv_len' was declared here
>      struct bio_vec bvec, bvprv;
>                           ^
>    In file included from [...]/arch/x86/include/asm/page.h:70:0,
>                     from [...]/arch/x86/include/asm/processor.h:17,
>                     from [...]/arch/x86/include/asm/atomic.h:6,
>                     from include/linux/atomic.h:4,
>                     from include/linux/mutex.h:18,
>                     from include/linux/kernfs.h:13,
>                     from include/linux/sysfs.h:15,
>                     from include/linux/kobject.h:21,
>                     from include/linux/pci.h:28,
>                     from include/linux/nvme.h:23,
>                     from drivers/block/nvme-core.c:19:
>    include/asm-generic/memory_model.h:31:53: warning: 'bvprv.bv_page' may be used uninitialized in this function [-Wmaybe-uninitialized]
>     #define __page_to_pfn(page) ((unsigned long)((page) - mem_map) + \
>                                                         ^
>    drivers/block/nvme-core.c:498:23: note: 'bvprv.bv_page' was declared here
>      struct bio_vec bvec, bvprv;
>                           ^
>
> These are false positives. Apparently GCC can't determine that bvprv
> will only be used if first is false and has, therefore, been
> initialized. It turned out hard to reorganize the code to help GCC
> understand the flow of this code. So take the easy way out and
> initialize bvprv to { NULL }.
>
> And, since we're touching this code, make first a bool.
>
> Signed-off-by: Paul Bolle <pebolle@...cali.nl>
> ---
> v2: redone, as required by Keith's review. Note that initializing bvprv
> to { NULL } is already done twice in block/blk-merge.c, which inspired
> me to take the easy way out here.
>
> Also note that it's actually not clear to me why these warnings only
> trigger on 32 bit. I guess there's some int/long conversion lurking
> somewhere. I haven't found it.
>
> bvprv? Would that mean bio_vec private? But it looks like some temporary
> variable, so something like tmp may make more sense. Anyhow, still
> compile tested only (on 32 and 64 bit x86).

bvprv == bio_vec previous

> drivers/block/nvme-core.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
> index 51824d1..60f98be 100644
> --- a/drivers/block/nvme-core.c
> +++ b/drivers/block/nvme-core.c
> @@ -495,11 +495,11 @@ static int nvme_split_and_submit(struct bio *bio, struct nvme_queue *nvmeq,
> static int nvme_map_bio(struct nvme_queue *nvmeq, struct nvme_iod *iod,
> 		struct bio *bio, enum dma_data_direction dma_dir, int psegs)
> {
> -	struct bio_vec bvec, bvprv;
> +	struct bio_vec bvec, bvprv = { NULL };
> 	struct bvec_iter iter;
> 	struct scatterlist *sg = NULL;
> 	int length = 0, nsegs = 0, split_len = bio->bi_iter.bi_size;
> -	int first = 1;
> +	bool first = true;
>
> 	if (nvmeq->dev->stripe_size)
> 		split_len = nvmeq->dev->stripe_size -
> @@ -525,7 +525,7 @@ static int nvme_map_bio(struct nvme_queue *nvmeq, struct nvme_iod *iod,
> 			return nvme_split_and_submit(bio, nvmeq, split_len);
> 		length += bvec.bv_len;
> 		bvprv = bvec;
> -		first = 0;
> +		first = false;
> 	}
> 	iod->nents = nsegs;
> 	sg_mark_end(sg);
> -- 
> 1.8.5.3
--
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/