linux-kernel - Re: [PATCH 002 of 6] Introduce rq_for_each_segment replacing rq_for_each

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: <Pine.LNX.4.62.0708201313260.6197@pademelon.sonytel.be>
Date:	Mon, 20 Aug 2007 13:21:09 +0200 (CEST)
From:	Geert Uytterhoeven <Geert.Uytterhoeven@...ycom.com>
To:	NeilBrown <neilb@...e.de>, Satyam Sharma <satyam@...radead.org>
cc:	Jan Engelhardt <jengelh@...putergmbh.de>,
	Jens Axboe <jens.axboe@...cle.com>,
	Linux Kernel Development <linux-kernel@...r.kernel.org>,
	Tejun Heo <htejun@...il.com>,
	Cell Broadband Engine OSS Development 
	<cbe-oss-dev@...abs.org>
Subject: Re: [PATCH 002 of 6] Introduce rq_for_each_segment replacing
 rq_for_each_bio

On Sat, 18 Aug 2007, Satyam Sharma wrote:
> On Sat, 18 Aug 2007, Jan Engelhardt wrote:
> > On Aug 18 2007 20:07, Satyam Sharma wrote:
> > >On Fri, 17 Aug 2007, Geert Uytterhoeven wrote:
> > >> On Thu, 16 Aug 2007, NeilBrown wrote:
> > >> [...]
> > >> >  		dev_dbg(&dev->sbd.core,
> > >> >  			"%s:%u: bio %u: %u segs %u sectors from %lu\n",
> > >> > -			__func__, __LINE__, i, bio_segments(bio),
> > >> > -			bio_sectors(bio), sector);
> > >> > -		bio_for_each_segment(bvec, bio, j) {
> > >> > +			__func__, __LINE__, i, bio_segments(iter.bio),
> > >> > +			bio_sectors(iter.bio),
> > >> > +			(unsigned long)iter.bio->bi_sector);
> > >>                         ^^^^^^^^^^^^^^^
> > >> Superfluous cast: PS3 is 64-bit only, and casts are evil.
> > 
> > bi_sector is sector_t. The cast is ok, because printf will warn, and rightfully
> > so since sector_t may just change its shape underneath. It would not be so much
> > of a problem if printf() was not a varargs function, but it is, and hence,
> > passing an object bigger than the format specifier can make problems.
> 
> Oh yeah, that's why the _cast_ _is_ needed in the first place, by the way.
> I was mentioning why the cast itself should be (unsigned long long) otoh.

On 64-bit platforms, sector_t (which is either u64 or unsigned long, depending
on CONFIG_LBD) is unsigned long, so there's no need for a cast. Hence there's
no compiler warning to be silenced by adding the cast.

On the other hand, adding a cast may hide bugs:
  - cast to unsigned long: When reusing parts of this code for a new 32-bit
    driver, the sector_t will be silently truncated,
  - cast to unsigned long long: When sector_t is changed to an even larger
    type, it will be silently truncated as well.
Without the cast, these would cause compiler warnings.

BTW, the resulting code didn't even compile, because of 3 bugs:

| linux-2.6/drivers/block/ps3disk.c: In function ‘ps3disk_scatter_gather’:
| linux-2.6/drivers/block/ps3disk.c:115: error: ‘bio’ undeclared (first use in this function)
| linux-2.6/drivers/block/ps3disk.c:115: error: (Each undeclared identifier is reported only once
| linux-2.6/drivers/block/ps3disk.c:115: error: for each function it appears in.)
| linux-2.6/drivers/block/ps3disk.c:115: error: ‘j’ undeclared (first use in this function)
| linux-2.6/drivers/block/ps3disk.c:116: error: implicit declaration of function ‘bio_kunmap_bvec’
| make[3]: *** [drivers/block/ps3disk.o] Error 1

With kind regards,
 
Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
 
Phone:    +32 (0)2 700 8453	
Fax:      +32 (0)2 700 8622	
E-mail:   Geert.Uytterhoeven@...ycom.com	
Internet: http://www.sony-europe.com/
 	
Sony Network and Software Technology Center Europe	
A division of Sony Service Centre (Europe) N.V.	
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium	
VAT BE 0413.825.160 · RPR Brussels	
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619