[v5,0/7] firmware: add partial read support in request_firmware_into_buf
mbox series

Message ID 20200508002739.19360-1-scott.branden@broadcom.com
Headers show
Series
  • firmware: add partial read support in request_firmware_into_buf
Related show

Message

Scott Branden May 8, 2020, 12:27 a.m. UTC
Please consider this version series ready for upstream acceptance.

This patch series adds partial read support in request_firmware_into_buf.
In order to accept the enhanced API it has been requested that kernel
selftests and upstreamed driver utilize the API enhancement and so
are included in this patch series.

Also in this patch series is the addition of a new Broadcom VK driver
utilizing the new request_firmware_into_buf enhanced API.

Changes from v4:
 - handle reset issues if card crashes
 - allow driver to have min required msix
 - add card utilization information
Changes from v3:
 - fix sparse warnings
 - fix printf format specifiers for size_t
 - fix 32-bit cross-compiling reports 32-bit shifts
 - use readl/writel,_relaxed to access pci ioremap memory,
  removed memory barriers and volatile keyword with such change
 - driver optimizations for interrupt/poll functionalities
Changes from v2:
 - remove unnecessary code and mutex locks in lib/test_firmware.c
 - remove VK_IOCTL_ACCESS_BAR support from driver and use pci sysfs instead
 - remove bitfields
 - remove Kconfig default m
 - adjust formatting and some naming based on feedback
 - fix error handling conditions
 - use appropriate return codes
 - use memcpy_toio instead of direct access to PCIE bar

Scott Branden (7):
  fs: introduce kernel_pread_file* support
  firmware: add offset to request_firmware_into_buf
  test_firmware: add partial read support for request_firmware_into_buf
  firmware: test partial file reads of request_firmware_into_buf
  bcm-vk: add bcm_vk UAPI
  misc: bcm-vk: add Broadcom VK driver
  MAINTAINERS: bcm-vk: add maintainer for Broadcom VK Driver

 MAINTAINERS                                   |    7 +
 drivers/base/firmware_loader/firmware.h       |    5 +
 drivers/base/firmware_loader/main.c           |   52 +-
 drivers/misc/Kconfig                          |    1 +
 drivers/misc/Makefile                         |    1 +
 drivers/misc/bcm-vk/Kconfig                   |   29 +
 drivers/misc/bcm-vk/Makefile                  |   12 +
 drivers/misc/bcm-vk/bcm_vk.h                  |  435 +++++
 drivers/misc/bcm-vk/bcm_vk_dev.c              | 1256 +++++++++++++++
 drivers/misc/bcm-vk/bcm_vk_legacy.c           |   89 +
 drivers/misc/bcm-vk/bcm_vk_msg.c              | 1425 +++++++++++++++++
 drivers/misc/bcm-vk/bcm_vk_msg.h              |  196 +++
 drivers/misc/bcm-vk/bcm_vk_sg.c               |  271 ++++
 drivers/misc/bcm-vk/bcm_vk_sg.h               |   60 +
 drivers/misc/bcm-vk/bcm_vk_tty.c              |  352 ++++
 drivers/soc/qcom/mdt_loader.c                 |    7 +-
 fs/exec.c                                     |   96 +-
 include/linux/firmware.h                      |    8 +-
 include/linux/fs.h                            |   20 +
 include/uapi/linux/misc/bcm_vk.h              |   99 ++
 lib/test_firmware.c                           |  144 +-
 .../selftests/firmware/fw_filesystem.sh       |   80 +
 22 files changed, 4596 insertions(+), 49 deletions(-)
 create mode 100644 drivers/misc/bcm-vk/Kconfig
 create mode 100644 drivers/misc/bcm-vk/Makefile
 create mode 100644 drivers/misc/bcm-vk/bcm_vk.h
 create mode 100644 drivers/misc/bcm-vk/bcm_vk_dev.c
 create mode 100644 drivers/misc/bcm-vk/bcm_vk_legacy.c
 create mode 100644 drivers/misc/bcm-vk/bcm_vk_msg.c
 create mode 100644 drivers/misc/bcm-vk/bcm_vk_msg.h
 create mode 100644 drivers/misc/bcm-vk/bcm_vk_sg.c
 create mode 100644 drivers/misc/bcm-vk/bcm_vk_sg.h
 create mode 100644 drivers/misc/bcm-vk/bcm_vk_tty.c
 create mode 100644 include/uapi/linux/misc/bcm_vk.h

Comments

Luis Chamberlain May 13, 2020, 12:38 a.m. UTC | #1
On Thu, May 07, 2020 at 05:27:38PM -0700, Scott Branden wrote:
> +#if defined(CONFIG_REQ_FW_INTO_BUF_PRIV)
> +
> +#define KERNEL_PREAD_FLAG_PART	0x0001 /* Allow reading part of file */
> +#define REQUEST_FIRMWARE_INTO_BUF request_firmware_into_buf_priv
> +int request_firmware_into_buf_priv(const struct firmware **firmware_p,
> +				   const char *name, struct device *device,
> +				   void *buf, size_t size,
> +				   size_t offset, unsigned int pread_flags);
> +
> +#else
> +
> +#define REQUEST_FIRMWARE_INTO_BUF request_firmware_into_buf
> +
> +#endif
> +
> +#endif

Please clean this up, the code must reflect only the code upstream. No
config stuff like this should be used on the driver. I had to stop my
review here.

  Luis
Scott Branden May 13, 2020, 6:31 a.m. UTC | #2
Hi Luis,

On 2020-05-12 5:38 p.m., Luis Chamberlain wrote:
> On Thu, May 07, 2020 at 05:27:38PM -0700, Scott Branden wrote:
>> +#if defined(CONFIG_REQ_FW_INTO_BUF_PRIV)
>> +
>> +#define KERNEL_PREAD_FLAG_PART	0x0001 /* Allow reading part of file */
>> +#define REQUEST_FIRMWARE_INTO_BUF request_firmware_into_buf_priv
>> +int request_firmware_into_buf_priv(const struct firmware **firmware_p,
>> +				   const char *name, struct device *device,
>> +				   void *buf, size_t size,
>> +				   size_t offset, unsigned int pread_flags);
>> +
>> +#else
>> +
>> +#define REQUEST_FIRMWARE_INTO_BUF request_firmware_into_buf
>> +
>> +#endif
>> +
>> +#endif
> Please clean this up, the code must reflect only the code upstream. No
> config stuff like this should be used on the driver. I had to stop my
> review here.
The CONFIG_ prefix shouldn't have been there as there is no Kconfig 
option to select this.
Would like to just change it to a normal define without CONFIG_ prefix 
instead?
This code is here to allow a limited version of the driver to run on 
older kernels which do not have the necessary partial file read support.
By having it in the upstream codebase we don't need to maintain an 
internal version of the driver.  User can take the upstream kernel 
module and compile it against an old version of the kernel by via the 
define.
>    Luis
Regards,
Scott
Greg Kroah-Hartman May 13, 2020, 6:50 a.m. UTC | #3
On Tue, May 12, 2020 at 11:31:28PM -0700, Scott Branden wrote:
> Hi Luis,
> 
> On 2020-05-12 5:38 p.m., Luis Chamberlain wrote:
> > On Thu, May 07, 2020 at 05:27:38PM -0700, Scott Branden wrote:
> > > +#if defined(CONFIG_REQ_FW_INTO_BUF_PRIV)
> > > +
> > > +#define KERNEL_PREAD_FLAG_PART	0x0001 /* Allow reading part of file */
> > > +#define REQUEST_FIRMWARE_INTO_BUF request_firmware_into_buf_priv
> > > +int request_firmware_into_buf_priv(const struct firmware **firmware_p,
> > > +				   const char *name, struct device *device,
> > > +				   void *buf, size_t size,
> > > +				   size_t offset, unsigned int pread_flags);
> > > +
> > > +#else
> > > +
> > > +#define REQUEST_FIRMWARE_INTO_BUF request_firmware_into_buf
> > > +
> > > +#endif
> > > +
> > > +#endif
> > Please clean this up, the code must reflect only the code upstream. No
> > config stuff like this should be used on the driver. I had to stop my
> > review here.
> The CONFIG_ prefix shouldn't have been there as there is no Kconfig option
> to select this.
> Would like to just change it to a normal define without CONFIG_ prefix
> instead?
> This code is here to allow a limited version of the driver to run on older
> kernels which do not have the necessary partial file read support.
> By having it in the upstream codebase we don't need to maintain an internal
> version of the driver.  User can take the upstream kernel module and compile
> it against an old version of the kernel by via the define.

That's not how kernel drivers in the tree work, sorry.  They do not
contain "older kernel support" in them, they work as a whole with the
rest of the kernel they ship with only.

Otherwise all drivers would be a total mess over time, can you imagine
doing this for the next 20+ years?  Not maintainable.

thanks,

greg k-h
Luis Chamberlain May 13, 2020, 12:30 p.m. UTC | #4
On Wed, May 13, 2020 at 08:50:46AM +0200, Greg Kroah-Hartman wrote:
> On Tue, May 12, 2020 at 11:31:28PM -0700, Scott Branden wrote:
> > Hi Luis,
> > 
> > On 2020-05-12 5:38 p.m., Luis Chamberlain wrote:
> > > On Thu, May 07, 2020 at 05:27:38PM -0700, Scott Branden wrote:
> > > > +#if defined(CONFIG_REQ_FW_INTO_BUF_PRIV)
> > > > +
> > > > +#define KERNEL_PREAD_FLAG_PART	0x0001 /* Allow reading part of file */
> > > > +#define REQUEST_FIRMWARE_INTO_BUF request_firmware_into_buf_priv
> > > > +int request_firmware_into_buf_priv(const struct firmware **firmware_p,
> > > > +				   const char *name, struct device *device,
> > > > +				   void *buf, size_t size,
> > > > +				   size_t offset, unsigned int pread_flags);
> > > > +
> > > > +#else
> > > > +
> > > > +#define REQUEST_FIRMWARE_INTO_BUF request_firmware_into_buf
> > > > +
> > > > +#endif
> > > > +
> > > > +#endif
> > > Please clean this up, the code must reflect only the code upstream. No
> > > config stuff like this should be used on the driver. I had to stop my
> > > review here.
> > The CONFIG_ prefix shouldn't have been there as there is no Kconfig option
> > to select this.
> > Would like to just change it to a normal define without CONFIG_ prefix
> > instead?
> > This code is here to allow a limited version of the driver to run on older
> > kernels which do not have the necessary partial file read support.
> > By having it in the upstream codebase we don't need to maintain an internal
> > version of the driver.  User can take the upstream kernel module and compile
> > it against an old version of the kernel by via the define.
> 
> That's not how kernel drivers in the tree work, sorry.  They do not
> contain "older kernel support" in them, they work as a whole with the
> rest of the kernel they ship with only.
> 
> Otherwise all drivers would be a total mess over time, can you imagine
> doing this for the next 20+ years?  Not maintainable.

Scott, now imagine the amount of cleanup you'd need to do to your driver
to get it to a state where it doesn't depend on any old kernel. That's
the exact shape of the driver we want.

To backport, you can look into the backports project which strives to
backport drivers automatically [0] to older kernels.

[0] https://backports.wiki.kernel.org/index.php/Main_Page

  Luis
Mimi Zohar May 13, 2020, 4:23 p.m. UTC | #5
Hi Scott,

On Thu, 2020-05-07 at 17:27 -0700, Scott Branden wrote:
> Please consider this version series ready for upstream acceptance.
> 
> This patch series adds partial read support in request_firmware_into_buf.
> In order to accept the enhanced API it has been requested that kernel
> selftests and upstreamed driver utilize the API enhancement and so
> are included in this patch series.
> 
> Also in this patch series is the addition of a new Broadcom VK driver
> utilizing the new request_firmware_into_buf enhanced API.

Up to now, the firmware blob was read into memory allowing IMA to
verify the file signature.  With this change, ima_post_read_file()
will not be able to verify the file signature.

(I don't think any of the other LSMs are on this hook, but you might
want to Cc the LSM or integrity mailing list.)

Mimi
Scott Branden May 13, 2020, 6:39 p.m. UTC | #6
Thanks for link.

On 2020-05-13 5:30 a.m., Luis Chamberlain wrote:
> On Wed, May 13, 2020 at 08:50:46AM +0200, Greg Kroah-Hartman wrote:
>> On Tue, May 12, 2020 at 11:31:28PM -0700, Scott Branden wrote:
>>
>> That's not how kernel drivers in the tree work, sorry.  They do not
>> contain "older kernel support" in them, they work as a whole with the
>> rest of the kernel they ship with only.
>>
>> Otherwise all drivers would be a total mess over time, can you imagine
>> doing this for the next 20+ years?  Not maintainable.
> Scott, now imagine the amount of cleanup you'd need to do to your driver
> to get it to a state where it doesn't depend on any old kernel. That's
> the exact shape of the driver we want.
>
> To backport, you can look into the backports project which strives to
> backport drivers automatically [0] to older kernels.
>
> [0] https://backports.wiki.kernel.org/index.php/Main_Page
>
>    Luis
Will drop legacy support from patch and look closer at this.

Thanks,
  Scott
Luis Chamberlain May 15, 2020, 8:47 p.m. UTC | #7
On Wed, May 13, 2020 at 12:23:59PM -0400, Mimi Zohar wrote:
> Hi Scott,
> 
> On Thu, 2020-05-07 at 17:27 -0700, Scott Branden wrote:
> > Please consider this version series ready for upstream acceptance.
> > 
> > This patch series adds partial read support in request_firmware_into_buf.
> > In order to accept the enhanced API it has been requested that kernel
> > selftests and upstreamed driver utilize the API enhancement and so
> > are included in this patch series.
> > 
> > Also in this patch series is the addition of a new Broadcom VK driver
> > utilizing the new request_firmware_into_buf enhanced API.
> 
> Up to now, the firmware blob was read into memory allowing IMA to
> verify the file signature.  With this change, ima_post_read_file()
> will not be able to verify the file signature.
> 
> (I don't think any of the other LSMs are on this hook, but you might
> want to Cc the LSM or integrity mailing list.)

Scott, so it sounds we need a resolution for pread for IMA for file
signature verification. It seems that can be addressed though. Feel
free to submit the u32 flag changes which you picked up on though in
the meantime.

  Luis
Scott Branden May 15, 2020, 11:28 p.m. UTC | #8
Hi Luis,

On 2020-05-15 1:47 p.m., Luis Chamberlain wrote:
> On Wed, May 13, 2020 at 12:23:59PM -0400, Mimi Zohar wrote:
>> Hi Scott,
>>
>> On Thu, 2020-05-07 at 17:27 -0700, Scott Branden wrote:
>>> Please consider this version series ready for upstream acceptance.
>>>
>>> This patch series adds partial read support in request_firmware_into_buf.
>>> In order to accept the enhanced API it has been requested that kernel
>>> selftests and upstreamed driver utilize the API enhancement and so
>>> are included in this patch series.
>>>
>>> Also in this patch series is the addition of a new Broadcom VK driver
>>> utilizing the new request_firmware_into_buf enhanced API.
>> Up to now, the firmware blob was read into memory allowing IMA to
>> verify the file signature.  With this change, ima_post_read_file()
>> will not be able to verify the file signature.
>>
>> (I don't think any of the other LSMs are on this hook, but you might
>> want to Cc the LSM or integrity mailing list.)
> Scott, so it sounds we need a resolution for pread for IMA for file
> signature verification. It seems that can be addressed though. Feel
> free to submit the u32 flag changes which you picked up on though in
> the meantime.
Sure, I can submit a new patch to change the existing enum to
a u32.  For the new pread flags I am adding I could also leave as
a u32 or change from a u32 to an enum since there is currently only
one flag in use.  Then, in the future if another flag was added we would 
need
to change it back to a u32.

Please let me know what you prefer for the pread_flags.
>
>    Luis
Thanks,
  Scott
Luis Chamberlain May 16, 2020, 1:05 a.m. UTC | #9
On Fri, May 15, 2020 at 04:28:08PM -0700, Scott Branden wrote:
> Hi Luis,
> 
> On 2020-05-15 1:47 p.m., Luis Chamberlain wrote:
> > On Wed, May 13, 2020 at 12:23:59PM -0400, Mimi Zohar wrote:
> > > Hi Scott,
> > > 
> > > On Thu, 2020-05-07 at 17:27 -0700, Scott Branden wrote:
> > > > Please consider this version series ready for upstream acceptance.
> > > > 
> > > > This patch series adds partial read support in request_firmware_into_buf.
> > > > In order to accept the enhanced API it has been requested that kernel
> > > > selftests and upstreamed driver utilize the API enhancement and so
> > > > are included in this patch series.
> > > > 
> > > > Also in this patch series is the addition of a new Broadcom VK driver
> > > > utilizing the new request_firmware_into_buf enhanced API.
> > > Up to now, the firmware blob was read into memory allowing IMA to
> > > verify the file signature.  With this change, ima_post_read_file()
> > > will not be able to verify the file signature.
> > > 
> > > (I don't think any of the other LSMs are on this hook, but you might
> > > want to Cc the LSM or integrity mailing list.)
> > Scott, so it sounds we need a resolution for pread for IMA for file
> > signature verification. It seems that can be addressed though. Feel
> > free to submit the u32 flag changes which you picked up on though in
> > the meantime.
> Sure, I can submit a new patch to change the existing enum to
> a u32. 

Great thanks!

> For the new pread flags I am adding I could also leave as
> a u32 or change from a u32 to an enum since there is currently only
> one flag in use.  Then, in the future if another flag was added we would
> need
> to change it back to a u32.

Yes that approach works well, enum for now.


  Luis