| Message ID | 20210112132449.22243-2-tiwai@suse.de (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | iwlwifi: Fix a crash at loading | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
Takashi Iwai <tiwai@suse.de> writes: > The commit ba8f6f4ae254 ("iwlwifi: dbg: add dumping special device > memory") added a termination of name string just to be sure, and this > seems causing a regression, a GPF triggered at firmware loading. > Basically we shouldn't modify the firmware data that may be provided > as read-only. > > This patch drops the code that caused the regression and keep the tlv > data as is. > > Fixes: ba8f6f4ae254 ("iwlwifi: dbg: add dumping special device memory") > BugLink: https://bugzilla.suse.com/show_bug.cgi?id=1180344 > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=210733 > Signed-off-by: Takashi Iwai <tiwai@suse.de> I'm planning to queue this to v5.11. Should I add cc stable? Luca, can I have your ack?
On Tue, 12 Jan 2021 16:48:56 +0100, Kalle Valo wrote: > > Takashi Iwai <tiwai@suse.de> writes: > > > The commit ba8f6f4ae254 ("iwlwifi: dbg: add dumping special device > > memory") added a termination of name string just to be sure, and this > > seems causing a regression, a GPF triggered at firmware loading. > > Basically we shouldn't modify the firmware data that may be provided > > as read-only. > > > > This patch drops the code that caused the regression and keep the tlv > > data as is. > > > > Fixes: ba8f6f4ae254 ("iwlwifi: dbg: add dumping special device memory") > > BugLink: https://bugzilla.suse.com/show_bug.cgi?id=1180344 > > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=210733 > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > I'm planning to queue this to v5.11. Should I add cc stable? Yes, it hits 5.10.y. > Luca, can I have your ack? It'd be great if this fix goes in quickly. BTW, I thought network people don't want to have Cc-to-stable in the patch, so I didn't put it by myself. Is this rule still valid? thanks, Takashi
On Tue, 2021-01-12 at 17:02 +0100, Takashi Iwai wrote: > On Tue, 12 Jan 2021 16:48:56 +0100, > Kalle Valo wrote: > > > > Takashi Iwai <tiwai@suse.de> writes: > > > > > The commit ba8f6f4ae254 ("iwlwifi: dbg: add dumping special device > > > memory") added a termination of name string just to be sure, and this > > > seems causing a regression, a GPF triggered at firmware loading. > > > Basically we shouldn't modify the firmware data that may be provided > > > as read-only. > > > > > > This patch drops the code that caused the regression and keep the tlv > > > data as is. > > > > > > Fixes: ba8f6f4ae254 ("iwlwifi: dbg: add dumping special device memory") > > > BugLink: https://bugzilla.suse.com/show_bug.cgi?id=1180344 > > > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=210733 > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > > > I'm planning to queue this to v5.11. Should I add cc stable? > > Yes, it hits 5.10.y. > > > Luca, can I have your ack? > > It'd be great if this fix goes in quickly. Thanks for the fix! Acked-by: Luca Coelho <luciano.coelho@intel.com> > BTW, I thought network people don't want to have Cc-to-stable in the > patch, so I didn't put it by myself. Is this rule still valid? In the wireless side of network, we've always used Cc stable when needed, but the Fixes tag itself will almost always trigger the stable people to take it anyway. -- Cheers, Luca.
"Coelho, Luciano" <luciano.coelho@intel.com> writes: >> BTW, I thought network people don't want to have Cc-to-stable in the >> patch, so I didn't put it by myself. Is this rule still valid? > > In the wireless side of network, we've always used Cc stable when > needed Yeah, we handle stable patches differently from the main network tree. > but the Fixes tag itself will almost always trigger the stable > people to take it anyway. BTW, this is now clarified in the documentation: https://lkml.kernel.org/r/20210113163315.1331064-1-lee.jones@linaro.org So cc stable should be added even if there's already a Fixes tag.
Takashi Iwai <tiwai@suse.de> wrote: > The commit ba8f6f4ae254 ("iwlwifi: dbg: add dumping special device > memory") added a termination of name string just to be sure, and this > seems causing a regression, a GPF triggered at firmware loading. > Basically we shouldn't modify the firmware data that may be provided > as read-only. > > This patch drops the code that caused the regression and keep the tlv > data as is. > > Fixes: ba8f6f4ae254 ("iwlwifi: dbg: add dumping special device memory") > BugLink: https://bugzilla.suse.com/show_bug.cgi?id=1180344 > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=210733 > Cc: stable@vger.kernel.org > Signed-off-by: Takashi Iwai <tiwai@suse.de> > Acked-by: Luca Coelho <luciano.coelho@intel.com> Patch applied to wireless-drivers.git, thanks. a6616bc9a0af iwlwifi: dbg: Don't touch the tlv data
diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c index a654147d3cd6..a80a35a7740f 100644 --- a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c +++ b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c @@ -180,13 +180,6 @@ static int iwl_dbg_tlv_alloc_region(struct iwl_trans *trans, if (le32_to_cpu(tlv->length) < sizeof(*reg)) return -EINVAL; - /* For safe using a string from FW make sure we have a - * null terminator - */ - reg->name[IWL_FW_INI_MAX_NAME - 1] = 0; - - IWL_DEBUG_FW(trans, "WRT: parsing region: %s\n", reg->name); - if (id >= IWL_FW_INI_MAX_REGION_ID) { IWL_ERR(trans, "WRT: Invalid region id %u\n", id); return -EINVAL;
The commit ba8f6f4ae254 ("iwlwifi: dbg: add dumping special device memory") added a termination of name string just to be sure, and this seems causing a regression, a GPF triggered at firmware loading. Basically we shouldn't modify the firmware data that may be provided as read-only. This patch drops the code that caused the regression and keep the tlv data as is. Fixes: ba8f6f4ae254 ("iwlwifi: dbg: add dumping special device memory") BugLink: https://bugzilla.suse.com/show_bug.cgi?id=1180344 BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=210733 Signed-off-by: Takashi Iwai <tiwai@suse.de> --- As an alternative fix, the debug print could be with the printf length specifier to limit the size to IWL_FW_INIT_MAX_NAME, as found in the bugzilla entries above, too. I chose a simpler (partial) revert here instead. drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c | 7 ------- 1 file changed, 7 deletions(-)