[Intel-gfx] [PATCH] [i915] avoid infinite retries in GuC/HuC loading

[John Harrison]

On 3/12/2023 12:56, Alexandre Oliva wrote:
> If two or more suitable entries with the same filename are found in
> __uc_fw_auto_select's fw_blobs, and that filename fails to load in the
> first attempt and in the retry, when __uc_fw_auto_select is called for
> the third time, the coincidence of strings will cause it to clear
> file_selected.path at the first hit, so it will return the second hit
> over and over again, indefinitely.
>
> Of course this doesn't occur with the pristine blob lists, but a
> modified version could run into this, e.g., patching in a duplicate
> entry, or (as in our case) disarming blob loading by remapping their
> names to "/*(DEBLOBBED)*/", given a toolchain that unifies identical
> string literals.
Not sure what you mean by disarming?

I think what you are saying is that you made a change similar to this?
     #define __MAKE_UC_FW_PATH_MMP(prefix_, name_, major_, minor_, 
patch_) "i915/invalid_file_name.bin"

So all entries in the table have the exact same filename. And with the 
toolchain unification comment, that means not just a matching string but 
the same string pointer. Thus, the search code is getting confused.

I'm not sure that is really a valid use case that the driver code should 
be expected to support. I'm not even sure what the purpose of your 
testing is? Even without the infinite loop, the driver is not going to 
load because you have removed the firmware files?

However, I think you are saying that the problem would also exist if 
there was some kind of genuine duplication in the table? Can you give an 
example of a genuine use case problem? If the same string is used for 
different platforms, I believe that should be fine. E.g. there are 
already a bunch of different platforms that all use the same TGL 
firmware file. Even with the string unification, that should not be an 
issue because the search is within a platform only. So there can only be 
a problem if a single platform specifies the same filename multiple 
times? Which would be a bug in the table because why? It would be 
redundant entries that have no purpose.

Note that I'm not saying we don't want to take your change. But I would 
like to understand if there is a genuine issue that maybe needs a better 
fix. E.g. should the table verification code be enhanced to just reject 
the table entirely if there are such errors present.

Also, is this string unification thing a part of the current gcc 
toolchain? Or are you saying that is a new feature that is not generally 
available yet? Or maybe only exists in some non-gcc toolchain?

Thanks,
John.


>
> Of course I'm ready to carry a patchlet to avoid this problem
> triggered by our (GNU Linux-libre's) intentional changes, but I
> figured you might be interested in fail-safing it even in accidental
> backporting circumstances.  I realize it's not entirely foolproof: if
> the same string appears in two entries separated by a different one,
> the infinite loop might still occur.  Catching that even more unlikely
> situation seemed too expensive.
>
> Link: https://www.fsfla.org/pipermail/linux-libre/2023-March/003506.html
> Cc: intel-gfx at lists.freedesktop.org
> Cc: stable at vger.kernel.org # 6.[12].x
> Signed-off-by: Alexandre Oliva <lxoliva at fsfla.org>
> ---
>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> index 9d6f571097e6..2b7564a3ed82 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -259,7 +259,10 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
>   				uc_fw->file_selected.path = NULL;
>   
>   			continue;
> -		}
> +		} else if (uc_fw->file_wanted.path == blob->path)
> +			/* Avoid retrying forever when neighbor
> +			   entries point to the same path.  */
> +			continue;
>   
>   		uc_fw->file_selected.path = blob->path;
>   		uc_fw->file_wanted.path = blob->path;