| 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
| ||
|
Message-Id: <20090708062729.E32231B00E2@f41.poczta.interia.pl> Date: 08 Jul 2009 08:27:28 +0200 From: krzysztof.h1@...zta.fm To: Linus Torvalds <torvalds@...ux-foundation.org>, Krzysztof Helt <krzysztof.h1@...zta.fm>, Linux-fbdev-devel <linux-fbdev-devel@...ts.sourceforge.net>, linux-kernel@...r.kernel.org, akpm@...ux-foundation.org, a.p.zijlstra@...llo.nl, rjw@...k.pl, stable@...nel.org Subject: Re: Re: matroxfb: fix regression with uninitalized fb_info->mm_lock mutex (second head) Linus Torvalds napisał(a): > > > On Tue, 7 Jul 2009, Krzysztof Helt wrote: > > > > Remove redundant locking by the mm_lock mutex before a second head of > matrox > > framebuffer is registered. > > Why do you write misleading commentary like this. > I have not intention to mislead someone. English is not my primary language and this can be a source of this unfortunate wording. > > +/* > > + * This function is called before the register_framebuffer so > > + * no locking is needed. > > + */ > > Or this? > > It's not about "needed". The locking is not only not needed, it would be > BUGGY. > > And it's not "redundant". That implies that it's done somewhere else. It's > > more than "not needed" - it would be actively buggy to lock things there. > > I really don't like how you're approaching this. You're ignoring the real > > issues I ask you, you're writing misleading comments and commit messages, > > and the end result is fragile code. I still don't understand why you > insist on initializing those things late, which is the primary problem > here. > The mm_lock mutex is used only inside the fb_mmap() function and driver's specific code. The fb_mmap() cannot be called before a framebuffer is registered. That's why it is enough to initialize the mutex just before registering the framebuffer (or inside the register_framebuffer()). A current approach allows for statically declared fb_info structures (you consider it wrong). It is easy to find affected drivers: "grep -rl mm_lock drivers/video/*". I went too far with some drivers and added mm_lock mutex to functions which are called only from a probing function. They do not need any locking yet. This is similar as marking them as __init or __devinit which makes incorrect calling them after driver initialization. Some of these functions are so short they can be unrolled into the probing functions. Does it make removal of the unneeded mutexing correct then? All other drivers just have, let's say, inefficient code - calling a function which is called just a moment later or set values which are not used before set again later (i810fb, matroxfb, sisfb). The mm_lock patch converted this inefficient code into broken code. I understand you choose different approach. In order to keep this inefficient code in place you decided to change some correctly working legacy drivers and disallow statically declared fb_info structures. I see I cannot convince you it is worse solution so I am not trying any more. However, you can still count my patches as small code improvements. My fault is that I haven't tested the patch on a hardware and driver with added mm_lock mutex and broken by this (like sisfb or matroxfb). I have not known I broke them. Regards, Krzysztof ---------------------------------------------------------------------- Teraz AC/OC + ubezpieczenie kosztow leczenia za granica http://link.interia.pl/f2230 -- 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/
Powered by blists - more mailing lists