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
| ||
|
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
Powered by blists - more mailing lists