Re: [RESEND PATCH v1 1/8] firmware_loader: Clear data and size in fw_free_paged_buf

[Tom Rix]

On 3/28/22 11:09 AM, Russ Weight wrote:
> Hi Tom,
>
> On 3/28/22 06:27, Tom Rix wrote:
> > On 3/23/22 4:33 PM, Russ Weight wrote:
> > > The fw_free_paged_buf() function resets the paged buffer information in
> > > the fw_priv data structure. Additionally, clear the data and size members
> > > of fw_priv in order to facilitate the reuse of fw_priv. This is being
> > > done in preparation for enabling userspace to initiate multiple firmware
> > > uploads using this sysfs interface.
> > >
> > > Signed-off-by: Russ Weight <russell.h.weight@xxxxxxxxx>
> > > ---
> > > v1:
> > >     - No change from RFC patch
> > > ---
> > >    drivers/base/firmware_loader/main.c | 2 ++
> > >    1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> > > index 94d1789a233e..2cc11d93753a 100644
> > > --- a/drivers/base/firmware_loader/main.c
> > > +++ b/drivers/base/firmware_loader/main.c
> > > @@ -253,6 +253,8 @@ void fw_free_paged_buf(struct fw_priv *fw_priv)
> > Why isn't a vfree needed or realloc done?

I am looking at the use of this function in __free_fw_priv

      if (fw_is_paged_buf(fw_priv))
          fw_free_paged_buf(fw_priv);
      else if (!fw_priv->allocated_size)
          vfree(fw_priv->data);

Where it seems like there is another way to set data, so it needs 
another way to unset.

The vfree here looks suspect because the pointer comes in from 
request_firmware_info_buf with a hope that it was allocated by vmalloc.

Tom

> The free and realloc support was present prior to my changes. The page
> buffer support was designed such that if a firmware write was cancelled, the
> existing fw_priv structure could be re-used for another write in the context
> of the same firmware upload. However, there was no prior case for completing
> a write and then reusing the fw_priv structure for subsequent firmware writes;
> fw_priv previously had a one-time use. The changes I have made are to enable
> the re-use of the fw_priv structure.
>
> Initially, fw_priv->data is NULL. The "realloc" functionality happens during
> the write of the data binary attribute here:
>
> https://github.com/torvalds/linux/blob/ae085d7f9365de7da27ab5c0d16b12d51ea7fca9/drivers/base/firmware_loader/fallback.c#L426
>
> The fw_priv->data pointer remains NULL until all data is written and the
> user writes '0' to the loading attribute. The fw_priv->data pointer is set in
> fw_map_paged_buf() which is called here:
>
> https://github.com/torvalds/linux/blob/ae085d7f9365de7da27ab5c0d16b12d51ea7fca9/drivers/base/firmware_loader/fallback.c#L274
>
> In the unmodified code, the fw_priv->data pointer is never cleared. My changes
> reset the pointer to NULL after the memory is released so that the fw_priv can
> be resused.
>
> The new firmware-upload happens in the context of a kernel worker thread and the work
> function is fw_upload_main(). At the end of fw_upload_main(), fw_free_paged_buf()
> is called to do the free. This is the function that is being modified by the lines
> below. This function calls "__free_page(fw_priv->pages[i])" in a loop to free the
> memory pages. It also calls "vunmap(fw_priv->data)" to free the virtual mapping.
> You can see the unmodified implementation of this function here:
>
> https://github.com/torvalds/linux/blob/ae085d7f9365de7da27ab5c0d16b12d51ea7fca9/drivers/base/firmware_loader/main.c#L241
>
> - Russ
>
> > Tom
> >
> > >        fw_priv->pages = NULL;
> > >        fw_priv->page_array_size = 0;
> > >        fw_priv->nr_pages = 0;
> > > +    fw_priv->data = NULL;
> > > +    fw_priv->size = 0;
> > >    }
> > >      int fw_grow_paged_buf(struct fw_priv *fw_priv, int pages_needed)