From: "Luis R. Rodriguez" <[email protected]>
Ming, Greg,
this patch set consists of a few small code shifts which would make
it easier to add extensible firmware API code, and later firmware
signing support. This patch set is being sent out separately as it
does not contain any controversial changes. It should also help
with readibility of the code.
I'll be Cc'ing linux-doc, linux-security-module, and keyring folks as the
next patch sets would start slowly diving into the topic of firmware signing
and extending documentation, and those patches will depend on this set.
There is a superfluous else branch on patch #3, its not needed because of
the goto statement but we leave that in place to make patch #4 easier to
read.
David Howells (2):
firmware: fold successful fw read early
firmware: generalize reading file contents as a helper
Luis R. Rodriguez (2):
firmware: generalize "firmware" as "system data" helpers
firmware: move completing fw into a helper
drivers/base/firmware_class.c | 94 ++++++++++++++++++++++++++-----------------
1 file changed, 57 insertions(+), 37 deletions(-)
--
2.3.2.209.gd67f9d5.dirty
From: "Luis R. Rodriguez" <[email protected]>
Historically firmware_class code was added to help
get device driver firmware binaries but these days
request_firmware*() helpers are being repurposed for
general system data needed by the kernel.
Annotate this before we extend firmare_class more,
as this is expected. We want to generalize the code
as much as possible.
Cc: Rusty Russell <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: David Howells <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Casey Schaufler <[email protected]>
Cc: Ming Lei <[email protected]>
Cc: Takashi Iwai <[email protected]>
Cc: Vojtěch Pavlík <[email protected]>
Cc: Kyle McMartin <[email protected]>
Cc: Matthew Garrett <[email protected]>
Cc: [email protected]
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/base/firmware_class.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 894bda114224..f97e76cca069 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -353,15 +353,15 @@ static int fw_get_filesystem_firmware(struct device *device,
rc = fw_read_file_contents(file, buf);
fput(file);
if (rc)
- dev_warn(device, "firmware, attempted to load %s, but failed with error %d\n",
- path, rc);
+ dev_warn(device, "system data, attempted to load %s, but failed with error %d\n",
+ path, rc);
else
break;
}
__putname(path);
if (!rc) {
- dev_dbg(device, "firmware: direct-loading firmware %s\n",
+ dev_dbg(device, "system data: direct-loading firmware %s\n",
buf->fw_id);
mutex_lock(&fw_lock);
set_bit(FW_STATUS_DONE, &buf->status);
@@ -1051,7 +1051,7 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
}
if (fw_get_builtin_firmware(firmware, name)) {
- dev_dbg(device, "firmware: using built-in firmware %s\n", name);
+ dev_dbg(device, "system data: using built-in system data%s\n", name);
return 0; /* assigned */
}
--
2.3.2.209.gd67f9d5.dirty
From: "Luis R. Rodriguez" <[email protected]>
This will be re-used later through a new extensible interface.
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/base/firmware_class.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index f97e76cca069..9ee334c1b872 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -322,6 +322,15 @@ fail:
return rc;
}
+static void fw_finish_direct_load(struct device *device,
+ struct firmware_buf *buf)
+{
+ mutex_lock(&fw_lock);
+ set_bit(FW_STATUS_DONE, &buf->status);
+ complete_all(&buf->completion);
+ mutex_unlock(&fw_lock);
+}
+
static int fw_get_filesystem_firmware(struct device *device,
struct firmware_buf *buf)
{
@@ -363,10 +372,7 @@ static int fw_get_filesystem_firmware(struct device *device,
if (!rc) {
dev_dbg(device, "system data: direct-loading firmware %s\n",
buf->fw_id);
- mutex_lock(&fw_lock);
- set_bit(FW_STATUS_DONE, &buf->status);
- complete_all(&buf->completion);
- mutex_unlock(&fw_lock);
+ fw_finish_direct_load(device, buf);
}
return rc;
--
2.3.2.209.gd67f9d5.dirty
From: David Howells <[email protected]>
We'll be folding in some more checks on fw_read_file_contents(),
this will make the success case easier to follow.
Signed-off-by: David Howells <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
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 9ee334c1b872..736fb952b75b 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, "system data: direct-loading firmware %s\n",
+ buf->fw_id);
+ fw_finish_direct_load(device, buf);
+ goto out;
+ } else
dev_warn(device, "system data, attempted to load %s, but failed with error %d\n",
path, rc);
- else
- break;
}
+out:
__putname(path);
- if (!rc) {
- dev_dbg(device, "system data: direct-loading firmware %s\n",
- buf->fw_id);
- fw_finish_direct_load(device, buf);
- }
-
return rc;
}
--
2.3.2.209.gd67f9d5.dirty
From: David Howells <[email protected]>
We'll want to reuse this same code later in order to
read two separate types of file contents. This generalizes
fw_read_file() for reading a file rebrands it as fw_read_file().
This caller lets us pegs arbitrary data onto the target
buffer and size if the file is found.
While at it this cleans up the exit paths on fw_read_file().
Signed-off-by: David Howells <[email protected]>
Signed-off-by: Luis R. Rodriguez <[email protected]>
---
drivers/base/firmware_class.c | 62 +++++++++++++++++++++++++++----------------
1 file changed, 39 insertions(+), 23 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 736fb952b75b..dcc7036b4ad2 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -291,34 +291,51 @@ static const char * const fw_path[] = {
module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");
-static int fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf)
+/*
+ * Read the contents of a file.
+ */
+static int fw_read_file(const char *path, void **_buf, size_t *_size)
{
- int size;
+ struct file *file;
+ size_t size;
char *buf;
int rc;
+ file = filp_open(path, O_RDONLY, 0);
+ if (IS_ERR(file))
+ return PTR_ERR(file);
+
+ rc = -EINVAL;
if (!S_ISREG(file_inode(file)->i_mode))
- return -EINVAL;
+ goto err_file;
size = i_size_read(file_inode(file));
if (size <= 0)
- return -EINVAL;
+ goto err_file;
+ rc = -ENOMEM;
buf = vmalloc(size);
if (!buf)
- return -ENOMEM;
+ goto err_file;
+
rc = kernel_read(file, 0, buf, size);
+ if (rc < 0)
+ goto err_buf;
if (rc != size) {
- if (rc > 0)
- rc = -EIO;
- goto fail;
+ rc = -EIO;
+ goto err_buf;
}
+
rc = security_kernel_fw_from_file(file, buf, size);
if (rc)
- goto fail;
- fw_buf->data = buf;
- fw_buf->size = size;
+ goto err_buf;
+
+ *_buf = buf;
+ *_size = size;
return 0;
-fail:
+
+err_buf:
vfree(buf);
+err_file:
+ fput(file);
return rc;
}
@@ -332,19 +349,21 @@ static void fw_finish_direct_load(struct device *device,
}
static int fw_get_filesystem_firmware(struct device *device,
- struct firmware_buf *buf)
+ struct firmware_buf *buf)
{
int i, len;
int rc = -ENOENT;
- char *path;
+ char *path = NULL;
path = __getname();
if (!path)
return -ENOMEM;
+ /*
+ * Try each possible firmware blob in turn till one doesn't produce
+ * ENOENT.
+ */
for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
- struct file *file;
-
/* skip the unset customized path */
if (!fw_path[i][0])
continue;
@@ -356,23 +375,20 @@ static int fw_get_filesystem_firmware(struct device *device,
break;
}
- file = filp_open(path, O_RDONLY, 0);
- if (IS_ERR(file))
- continue;
- rc = fw_read_file_contents(file, buf);
- fput(file);
+ rc = fw_read_file(path, &buf->data, &buf->size);
if (rc == 0) {
dev_dbg(device, "system data: direct-loading firmware %s\n",
buf->fw_id);
fw_finish_direct_load(device, buf);
goto out;
- } else
+ } else if (rc != -ENOENT) {
dev_warn(device, "system data, attempted to load %s, but failed with error %d\n",
path, rc);
+ goto out;
+ }
}
out:
__putname(path);
-
return rc;
}
--
2.3.2.209.gd67f9d5.dirty
On Tue, Aug 4, 2015 at 6:00 PM, Luis R. Rodriguez
<[email protected]> wrote:
> From: David Howells <[email protected]>
>
> We'll be folding in some more checks on fw_read_file_contents(),
> this will make the success case easier to follow.
>
> Signed-off-by: David Howells <[email protected]>
> Signed-off-by: Luis R. Rodriguez <[email protected]>
> ---
> 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 9ee334c1b872..736fb952b75b 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, "system data: direct-loading firmware %s\n",
> + buf->fw_id);
> + fw_finish_direct_load(device, buf);
> + goto out;
'break' should be enough, and the following 'out' label can be saved too.
> + } else
> dev_warn(device, "system data, attempted to load %s, but failed with error %d\n",
> path, rc);
> - else
> - break;
> }
> +out:
> __putname(path);
>
> - if (!rc) {
> - dev_dbg(device, "system data: direct-loading firmware %s\n",
> - buf->fw_id);
> - fw_finish_direct_load(device, buf);
> - }
> -
> return rc;
> }
>
> --
> 2.3.2.209.gd67f9d5.dirty
>
On Sun, Aug 9, 2015 at 6:29 AM, Ming Lei <[email protected]> wrote:
> On Tue, Aug 4, 2015 at 6:00 PM, Luis R. Rodriguez
> <[email protected]> wrote:
>> From: David Howells <[email protected]>
>>
>> We'll be folding in some more checks on fw_read_file_contents(),
>> this will make the success case easier to follow.
>>
>> Signed-off-by: David Howells <[email protected]>
>> Signed-off-by: Luis R. Rodriguez <[email protected]>
>> ---
>> 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 9ee334c1b872..736fb952b75b 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, "system data: direct-loading firmware %s\n",
>> + buf->fw_id);
>> + fw_finish_direct_load(device, buf);
>> + goto out;
>
> 'break' should be enough, and the following 'out' label can be saved too.
This is true but I left it as it makes the next patch easier to read and follow.
>> + } else
>> dev_warn(device, "system data, attempted to load %s, but failed with error %d\n",
>> path, rc);
>> - else
>> - break;
>> }
>> +out:
>> __putname(path);
Luis
On Tue, Aug 04, 2015 at 03:00:00PM -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <[email protected]>
>
> Ming, Greg,
>
> this patch set consists of a few small code shifts which would make
> it easier to add extensible firmware API code, and later firmware
> signing support. This patch set is being sent out separately as it
> does not contain any controversial changes. It should also help
> with readibility of the code.
>
> I'll be Cc'ing linux-doc, linux-security-module, and keyring folks as the
> next patch sets would start slowly diving into the topic of firmware signing
> and extending documentation, and those patches will depend on this set.
>
> There is a superfluous else branch on patch #3, its not needed because of
> the goto statement but we leave that in place to make patch #4 easier to
> read.
>
> David Howells (2):
> firmware: fold successful fw read early
> firmware: generalize reading file contents as a helper
>
> Luis R. Rodriguez (2):
> firmware: generalize "firmware" as "system data" helpers
> firmware: move completing fw into a helper
Ming, Greg,
Please let me know if there are any issues with this series.
Luis
On Fri, Aug 21, 2015 at 11:23:03PM +0200, Luis R. Rodriguez wrote:
> On Tue, Aug 04, 2015 at 03:00:00PM -0700, Luis R. Rodriguez wrote:
> > From: "Luis R. Rodriguez" <[email protected]>
> >
> > Ming, Greg,
> >
> > this patch set consists of a few small code shifts which would make
> > it easier to add extensible firmware API code, and later firmware
> > signing support. This patch set is being sent out separately as it
> > does not contain any controversial changes. It should also help
> > with readibility of the code.
> >
> > I'll be Cc'ing linux-doc, linux-security-module, and keyring folks as the
> > next patch sets would start slowly diving into the topic of firmware signing
> > and extending documentation, and those patches will depend on this set.
> >
> > There is a superfluous else branch on patch #3, its not needed because of
> > the goto statement but we leave that in place to make patch #4 easier to
> > read.
> >
> > David Howells (2):
> > firmware: fold successful fw read early
> > firmware: generalize reading file contents as a helper
> >
> > Luis R. Rodriguez (2):
> > firmware: generalize "firmware" as "system data" helpers
> > firmware: move completing fw into a helper
>
> Ming, Greg,
>
> Please let me know if there are any issues with this series.
It's too late for 4.3 at the moment, and my first impulse is you are
just painting the bikeshed by changing some printk names, so I don't
like that, but I'll review it in full after 4.3-rc1 is out, can't do
anything until then.
thanks,
greg k-h
On Fri, Aug 21, 2015 at 10:38:24PM -0700, Greg KH wrote:
> On Fri, Aug 21, 2015 at 11:23:03PM +0200, Luis R. Rodriguez wrote:
> > On Tue, Aug 04, 2015 at 03:00:00PM -0700, Luis R. Rodriguez wrote:
> > > From: "Luis R. Rodriguez" <[email protected]>
> > >
> > > Ming, Greg,
> > >
> > > this patch set consists of a few small code shifts which would make
> > > it easier to add extensible firmware API code, and later firmware
> > > signing support. This patch set is being sent out separately as it
> > > does not contain any controversial changes. It should also help
> > > with readibility of the code.
> > >
> > > I'll be Cc'ing linux-doc, linux-security-module, and keyring folks as the
> > > next patch sets would start slowly diving into the topic of firmware signing
> > > and extending documentation, and those patches will depend on this set.
> > >
> > > There is a superfluous else branch on patch #3, its not needed because of
> > > the goto statement but we leave that in place to make patch #4 easier to
> > > read.
> > >
> > > David Howells (2):
> > > firmware: fold successful fw read early
> > > firmware: generalize reading file contents as a helper
> > >
> > > Luis R. Rodriguez (2):
> > > firmware: generalize "firmware" as "system data" helpers
> > > firmware: move completing fw into a helper
> >
> > Ming, Greg,
> >
> > Please let me know if there are any issues with this series.
>
> It's too late for 4.3 at the moment, and my first impulse is you are
> just painting the bikeshed by changing some printk names, so I don't
> like that,
This series doesn't address the actual sysdata helper changes which make the
firmware API easly extensible, it just paves the path for it, so because of
that what you describe is correct but only in lack of sight of the other RFC
patch I posted [0].
[0] http://[email protected]
> but I'll review it in full after 4.3-rc1 is out, can't do
> anything until then.
Sure.
Luis
On Thu, Oct 8, 2015 at 1:16 PM, Josh Boyer <[email protected]> wrote:
> On Tue, Oct 6, 2015 at 5:08 AM, Greg KH <[email protected]> wrote:
>> Just responding to one thing at the moment:
>>
>> On Mon, Oct 05, 2015 at 11:22:22PM +0200, Luis R. Rodriguez wrote:
>>> * we should phase out the usermode helper from firmware_class long term
>>
>> You can "phase out", but you can not delete it as it's a user/kernel api
>> that we have to support for forever, sorry.
>
> Assuming that dell-rbu is the only in-tree legitimate user of the
> userhelper code, I'm curious if the code itself could simply move into
> that driver. It might help prevent the spread of reliance on an API
> we don't want to see grow in usage. We'd probably need to evaluate if
> the two new users could migrate off that.
Greg pointed out Daniel might have some uses for this. More on this later.
>> Also, for some devices / use cases, the usermode helper is the solution
>> (think async loading of firmware when the host wants to do it.)
>
> Are any of those use cases in the kernel today, aside from dell-rbu?
> Would Luis' async mode to system data suffice?
We'll have to see based on Daniel's feedback (Daniel, please respond
to the other thread I'll Cc you on).
Luis