From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:47061 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752495AbcBDU0Z (ORCPT ); Thu, 4 Feb 2016 15:26:25 -0500 Date: Thu, 4 Feb 2016 21:26:22 +0100 From: "Luis R. Rodriguez" To: Kees Cook Cc: Mimi Zohar , linux-security-module , Kexec Mailing List , linux-modules@vger.kernel.org, "linux-fsdevel@vger.kernel.org" , David Howells , David Woodhouse , Dmitry Torokhov , Dmitry Kasatkin , Eric Biederman , Rusty Russell , "Luis R. Rodriguez" Subject: Re: [PATCH v3 06/22] firmware: fold successful fw read early Message-ID: <20160204202622.GH12481@wotan.suse.de> References: <1454526390-19792-1-git-send-email-zohar@linux.vnet.ibm.com> <1454526390-19792-7-git-send-email-zohar@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: Sender: owner-linux-modules@vger.kernel.org List-ID: On Thu, Feb 04, 2016 at 09:36:50AM -0800, Kees Cook wrote: > On Wed, Feb 3, 2016 at 11:06 AM, Mimi Zohar wrote: > > From: David Howells > > > > We'll be folding in some more checks on fw_read_file_contents(), > > this will make the success case easier to follow. > > > > Reviewed-by: Josh Boyer > > Signed-off-by: David Howells > > Signed-off-by: Luis R. Rodriguez > > Signed-off-by: Mimi Zohar > > --- > > drivers/base/firmware_class.c | 16 +++++++--------- > > 1 file changed, 7 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c > > index fb64814..c658cec 100644 > > --- a/drivers/base/firmware_class.c > > +++ b/drivers/base/firmware_class.c > > @@ -361,20 +361,18 @@ static int fw_get_filesystem_firmware(struct device *device, > > continue; > > rc = fw_read_file_contents(file, buf); > > fput(file); > > - if (rc) > > + if (rc == 0) { > > + dev_dbg(device, "direct-loading %s\n", > > + buf->fw_id); > > + fw_finish_direct_load(device, buf); > > + goto out; > > + } else > > dev_warn(device, "loading %s failed with error %d\n", > > path, rc); > > - else > > - break; > > } > > +out: > > __putname(path); > > > > - if (!rc) { > > - dev_dbg(device, "direct-loading %s\n", > > - buf->fw_id); > > - fw_finish_direct_load(device, buf); > > - } > > - > > return rc; > > } > > Looking at this code, why does this use "break": > > len = snprintf(path, PATH_MAX, "%s/%s", > fw_path[i], buf->fw_id); > if (len >= PATH_MAX) { > rc = -ENAMETOOLONG; > break; > } > > Shouldn't that emit a warning, set rc, and continue? Yes but this is a separate piece of code, and that's a functional change but I welcome the change for sure. This could be done separately. > Regardless, I think this is more readable. Adding an "out" target at > the end of a for loop seems weird, given "break" existing. :) Good call, goto mixed with while loops can be get iffy. I'm fine with this change as a replacement. To be fair we should also note both patches (the one submitted and yours below) also make a small functional change so that the loop continues over all other possible directories in the fw_path in case of failure with the first directory used as a base. If we wanted to be pedantic and careful we could split the functional change to not enable to continue in case of failure onto the next directory, and instead require that as a separate patch. I'll leave it to Mimi to decide. > > rc = fw_read_file_contents(file, buf); > fput(file); > - if (rc) > + if (rc) { > dev_warn(device, "loading %s failed with error %d\n", > path, rc); > + continue; > + } > + dev_dbg(device, "direct-loading %s\n", buf->fw_id); > + fw_finish_direct_load(device, buf); > - else > - break; > + break; > } > __putname(path); > - > - if (!rc) { > - dev_dbg(device, "direct-loading %s\n", > - buf->fw_id); > - fw_finish_direct_load(device, buf); > - } > - > return rc; > } Can you provided a Signed-offy-by? If so consider my Acked-by. Luis From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mx2.suse.de ([195.135.220.15]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1aRQUH-00084o-7c for kexec@lists.infradead.org; Thu, 04 Feb 2016 20:26:46 +0000 Date: Thu, 4 Feb 2016 21:26:22 +0100 From: "Luis R. Rodriguez" Subject: Re: [PATCH v3 06/22] firmware: fold successful fw read early Message-ID: <20160204202622.GH12481@wotan.suse.de> References: <1454526390-19792-1-git-send-email-zohar@linux.vnet.ibm.com> <1454526390-19792-7-git-send-email-zohar@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "kexec" Errors-To: kexec-bounces+dwmw2=infradead.org@lists.infradead.org To: Kees Cook Cc: Rusty Russell , "linux-fsdevel@vger.kernel.org" , David Woodhouse , Dmitry Torokhov , Kexec Mailing List , "Luis R. Rodriguez" , David Howells , linux-security-module , Eric Biederman , Dmitry Kasatkin , Mimi Zohar , linux-modules@vger.kernel.org On Thu, Feb 04, 2016 at 09:36:50AM -0800, Kees Cook wrote: > On Wed, Feb 3, 2016 at 11:06 AM, Mimi Zohar wrote: > > From: David Howells > > > > We'll be folding in some more checks on fw_read_file_contents(), > > this will make the success case easier to follow. > > > > Reviewed-by: Josh Boyer > > Signed-off-by: David Howells > > Signed-off-by: Luis R. Rodriguez > > Signed-off-by: Mimi Zohar > > --- > > drivers/base/firmware_class.c | 16 +++++++--------- > > 1 file changed, 7 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c > > index fb64814..c658cec 100644 > > --- a/drivers/base/firmware_class.c > > +++ b/drivers/base/firmware_class.c > > @@ -361,20 +361,18 @@ static int fw_get_filesystem_firmware(struct device *device, > > continue; > > rc = fw_read_file_contents(file, buf); > > fput(file); > > - if (rc) > > + if (rc == 0) { > > + dev_dbg(device, "direct-loading %s\n", > > + buf->fw_id); > > + fw_finish_direct_load(device, buf); > > + goto out; > > + } else > > dev_warn(device, "loading %s failed with error %d\n", > > path, rc); > > - else > > - break; > > } > > +out: > > __putname(path); > > > > - if (!rc) { > > - dev_dbg(device, "direct-loading %s\n", > > - buf->fw_id); > > - fw_finish_direct_load(device, buf); > > - } > > - > > return rc; > > } > > Looking at this code, why does this use "break": > > len = snprintf(path, PATH_MAX, "%s/%s", > fw_path[i], buf->fw_id); > if (len >= PATH_MAX) { > rc = -ENAMETOOLONG; > break; > } > > Shouldn't that emit a warning, set rc, and continue? Yes but this is a separate piece of code, and that's a functional change but I welcome the change for sure. This could be done separately. > Regardless, I think this is more readable. Adding an "out" target at > the end of a for loop seems weird, given "break" existing. :) Good call, goto mixed with while loops can be get iffy. I'm fine with this change as a replacement. To be fair we should also note both patches (the one submitted and yours below) also make a small functional change so that the loop continues over all other possible directories in the fw_path in case of failure with the first directory used as a base. If we wanted to be pedantic and careful we could split the functional change to not enable to continue in case of failure onto the next directory, and instead require that as a separate patch. I'll leave it to Mimi to decide. > > rc = fw_read_file_contents(file, buf); > fput(file); > - if (rc) > + if (rc) { > dev_warn(device, "loading %s failed with error %d\n", > path, rc); > + continue; > + } > + dev_dbg(device, "direct-loading %s\n", buf->fw_id); > + fw_finish_direct_load(device, buf); > - else > - break; > + break; > } > __putname(path); > - > - if (!rc) { > - dev_dbg(device, "direct-loading %s\n", > - buf->fw_id); > - fw_finish_direct_load(device, buf); > - } > - > return rc; > } Can you provided a Signed-offy-by? If so consider my Acked-by. Luis _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec