| Message ID | 1431455457-25322-2-git-send-email-mcgrof@do-not-panic.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
On Tue, May 12, 2015 at 11:30 AM, Luis R. Rodriguez <mcgrof@do-not-panic.com> wrote: > + > + path = __getname(); > + if (unlikely(!path)) > + return PTR_ERR(path); This makes no sense. PTR_ERR() on NULL is an insane operation. It's a very non-intuitive and misleading way of writing "0". So not only is that "return 0;", that's not likely what you want _anyway_. If you intended to return an error, you should just have done so, eg if (unlikely(!path)) return -ENOMEM; which actually does something sane, and is more readable. PTR_ERR() is for when you get an error pointer, so a sequence like if (IS_ERR(ptr)) return PTR_ERR(ptr); is sensible (it checks whether the ptr has an error value in it, and then returns the integer error value of the pointer). But for a NULL pointer? No. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 12, 2015 at 01:31:58PM -0700, Linus Torvalds wrote: > On Tue, May 12, 2015 at 11:30 AM, Luis R. Rodriguez > <mcgrof@do-not-panic.com> wrote: > > + > > + path = __getname(); > > + if (unlikely(!path)) > > + return PTR_ERR(path); > > This makes no sense. > > PTR_ERR() on NULL is an insane operation. It's a very non-intuitive > and misleading way of writing "0". > > So not only is that "return 0;", that's not likely what you want _anyway_. > > If you intended to return an error, you should just have done so, eg > > if (unlikely(!path)) > return -ENOMEM; > > which actually does something sane, and is more readable. Will fix, thanks. Luis -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 12, 2015 at 11:30:53AM -0700, Luis R. Rodriguez wrote: > From: "Luis R. Rodriguez" <mcgrof@suse.com> > > The request_firmware*() APIs uses __getname() to iterate > over the list of paths possible for firmware to be found, > the code however never checked for failure on __getname(). > Although *very unlikely*, this can still happen. Add the > missing check. > > There is still no checks on the concatenation of the path > and filename passed, that requires a bit more work and > subsequent patches address this. The commit that introduced > this is abb139e7 ("firmware: teach the kernel to load > firmware files directly from the filesystem"). > > mcgrof@ergon ~/linux (git::firmware-fixes) $ git describe --contains abb139e7 > v3.7-rc1~120 > > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Ming Lei <ming.lei@canonical.com> > Cc: Rusty Russell <rusty@rustcorp.com.au> > Cc: David Howells <dhowells@redhat.com> > Cc: Kyle McMartin <kyle@kernel.org> > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com> > --- > drivers/base/firmware_class.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c > index 171841a..bc6c8e6 100644 > --- a/drivers/base/firmware_class.c > +++ b/drivers/base/firmware_class.c > @@ -322,7 +322,11 @@ static int fw_get_filesystem_firmware(struct device *device, > { > int i; > int rc = -ENOENT; > - char *path = __getname(); > + char *path; > + > + path = __getname(); > + if (unlikely(!path)) Please only use likely/unlikely on code paths that actually care about it (i.e. you can measure the difference). Otherwise it is pretty useless, and people have determined that sometimes it is slower as humans get this wrong a lot of time... thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 12, 2015 at 02:21:11PM -0700, Greg KH wrote: > On Tue, May 12, 2015 at 11:30:53AM -0700, Luis R. Rodriguez wrote: > > From: "Luis R. Rodriguez" <mcgrof@suse.com> > > > > The request_firmware*() APIs uses __getname() to iterate > > over the list of paths possible for firmware to be found, > > the code however never checked for failure on __getname(). > > Although *very unlikely*, this can still happen. Add the > > missing check. > > > > There is still no checks on the concatenation of the path > > and filename passed, that requires a bit more work and > > subsequent patches address this. The commit that introduced > > this is abb139e7 ("firmware: teach the kernel to load > > firmware files directly from the filesystem"). > > > > mcgrof@ergon ~/linux (git::firmware-fixes) $ git describe --contains abb139e7 > > v3.7-rc1~120 > > > > Cc: Linus Torvalds <torvalds@linux-foundation.org> > > Cc: Ming Lei <ming.lei@canonical.com> > > Cc: Rusty Russell <rusty@rustcorp.com.au> > > Cc: David Howells <dhowells@redhat.com> > > Cc: Kyle McMartin <kyle@kernel.org> > > Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com> > > --- > > drivers/base/firmware_class.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c > > index 171841a..bc6c8e6 100644 > > --- a/drivers/base/firmware_class.c > > +++ b/drivers/base/firmware_class.c > > @@ -322,7 +322,11 @@ static int fw_get_filesystem_firmware(struct device *device, > > { > > int i; > > int rc = -ENOENT; > > - char *path = __getname(); > > + char *path; > > + > > + path = __getname(); > > + if (unlikely(!path)) > > Please only use likely/unlikely on code paths that actually care about > it (i.e. you can measure the difference). Otherwise it is pretty > useless, and people have determined that sometimes it is slower as > humans get this wrong a lot of time... Will do , thanks. Luis -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index 171841a..bc6c8e6 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -322,7 +322,11 @@ static int fw_get_filesystem_firmware(struct device *device, { int i; int rc = -ENOENT; - char *path = __getname(); + char *path; + + path = __getname(); + if (unlikely(!path)) + return PTR_ERR(path); for (i = 0; i < ARRAY_SIZE(fw_path); i++) { struct file *file;