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: Sat, 26 Apr 2008 00:03:02 -0700 (PDT) From: David Miller <davem@...emloft.net> To: sam@...nborg.org Cc: wg@...ndegger.com, oliver.hartkopp@...kswagen.de, socketcan-core@...ts.berlios.de, netdev@...r.kernel.org, urs.thuermann@...kswagen.de, xemul@...nvz.org Subject: Re: [PATCH][CAN]: Fix copy_from_user() results interpretation. From: Sam Ravnborg <sam@...nborg.org> Date: Sat, 26 Apr 2008 08:40:07 +0200 > On Sat, Apr 26, 2008 at 08:19:31AM +0200, Wolfgang Grandegger wrote: > > What about removing the assignment "err =" in that case. > Preferred. > > See sample patch below (made on top of -linus > so it does likely not apply but made it only to > see the difference anyway). I want to make one comment, directed at Wolfgang. You are absolutely wrong, Wolfgang, in saying that Pavel's original patch isn't easier to review than just changing this code to go: if (copy_*_user()) return -EFAULT; In fact, recoding things like this is an immense extra hardship on a reviewer. I'll explain why. If I see a patch that changes: err = SOMETHING; break; into: err = SOMETHING_ELSE; break; I know, WITH JUST READING THE PATCH, exactly what the side effects of this change are. I DO NOT need to bring the code into my editor and validate side effects to the surrounding code. I know that the assignment to 'err' is being changed, and that's it. Whereas if you change: err = SOMETHING; break; into: if (SOMETHING) return -SOME_ERROR; break; I now have to bring the code into an editor and make sure that the control flow change doesn't break things. For example, maybe the exit of the switch statement was important, to make sure cleanup code runs at the end of the function to release locks, free allocated memory, etc. With Pavel's patch it is not necessary to make such validations so it's INFINITELY easier to validate. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists