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.0708211148040.6982@pademelon.sonytel.be>
Date:	Tue, 21 Aug 2007 11:52:42 +0200 (CEST)
From:	Geert Uytterhoeven <Geert.Uytterhoeven@...ycom.com>
To:	Satyam Sharma <satyam@...radead.org>
cc:	NeilBrown <neilb@...e.de>,
	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 Tue, 21 Aug 2007, Satyam Sharma wrote:
> On Tue, 21 Aug 2007, Geert Uytterhoeven wrote:
> > On Tue, 21 Aug 2007, Satyam Sharma wrote:
> > > 
> > > This is turning rather funny :-)
> > > 
> > > * Why the printk() conversion specifier must be "%llu"?
> > > 
> > > When reusing parts of this code (such as this debug message) for another
> > > 32-bit driver (we certainly seem to care about this, as per your reply),
> > > the largest size of sector_t can be "unsigned long long", thereby causing
> > > truncated output, and the following warning:
> > > 
> > > warning: format ???lu???expects type ???ong unsigned int??? but argument 2 has
> > > type ???ector_t?
> > 
> > You will get a warning, so you will know.
> 
> Ah. So you'd much rather prefer that code in drivers/ make assumptions:
> sizeof(long) == sizeof(long long), and, sizeof(long) == sizeof(sector_t),
> hmmm?

If it's code for a PS3-specific driver that cannot work in 32-bit mode, yes.

> > > Therefore, let us not depend on the fact that this driver is being used
> > > only for 64-bit platforms as of now (especially since this is in drivers/
> > > and not in arch/) and rather make the printk() specifier as "%llu".
> > > 
> > > [ Sadly, I had to repeat most of my previous mail, which Jan Engelhardt
> > > appears to have snipped. ]
> > 
> > [ and you dropped the cell mailing list from the CC list ]
> 
> I don't enjoy getting "this is a subscriber-only list" notifications,
> thank you.

IC.

> > > * Why we require an explicit (unsigned long long) cast?
> > > 
> > > Having made the above change (and say we don't have an explicit cast
> > > there), that line now becomes:
> > > 
> > > 	printk("... %llu\n", ..., iter.bio->bi_sector);
> > > 
> > > Now if we build this code with CONFIG_LBD=n, sector_t becomes just an
> > > "unsigned long" (whereas printk specifier is the larger "%llu") thereby
> > > causing:
> > > 
> > > warning: format ???llu???expects type ???ong long unsigned int??? but argument
> > > 2 has type ???ector_t?
> > > 
> > > Therefore, let us shut this up with an explicit (unsigned long long) cast,
> > > as is done in most other existing code in the kernel where we want to get
> > > bi_sector printed out, which would correctly convert the value to the
> > > larger integer type (even negative ones, due to sign-extension) and print
> > > it out.
> > > 
> > > > On the other hand, adding a cast may hide bugs:
> > > >   - cast to unsigned long long: When sector_t is changed to an even larger
>           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > >     type, it will be silently truncated as well.
>           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > 
> > > IMHO, this is not a valid enough reason, given the above (BTW if/when that
> > > happens, we'll have to update the printk conversion specifer as well).
> > > 
> > > Personally, I'd say code that assumes "sizeof(unsigned long long) ==
> > > sizeof(unsigned long)", and also that assumes "sizeof(unsigned long) ==
> > > sizeof(sector_t)" -- both assumptions _are_ made by the above code --
> > > is not good taste, even if both may be true for PS3 platform. So unless
> > > we decide "nobody except that platform would ever build this driver
> > > anyway", I'd suggest to make the printk specifier as "%llu" and use an
> > > explicit (unsigned long long) cast. Therefore (on top of this series):
> > 
> > Adding such a cast makes it impossible for the compiler to detect (and warn us)
> > if something has changed.
> 
> Change as in increase in size, right? Did you even read the mail? The only

Yes. Yes.

> argument you had given against the cast to (unsigned long long) was what
> has been underlined above, which was bogus, considering we would have to
> change the "%lu" specifier in the printk() also, when that happens
> (otherwise suffer silent truncation).

Not silent truncation. Without a cast, the compiler will warn.

> But looks like you /are/ deciding "nobody except PS3 platform would ever
> build this driver anyway" ... so okay, the current code, will all it's
> non-portable assumptions, is okay.

OK, thx.
The driver uses the PS3-specific hypervisor interface, which is 64-bit.

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