All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] iwlwifi: Fix a crash at loading
@ 2021-01-12 13:24 Takashi Iwai
  2021-01-12 13:24 ` [PATCH 1/2] iwlwifi: dbg: Don't touch the tlv data Takashi Iwai
  2021-01-12 13:24 ` [PATCH 2/2] iwlwifi: dbg: Mark ucode tlv data as const Takashi Iwai
  0 siblings, 2 replies; 13+ messages in thread
From: Takashi Iwai @ 2021-01-12 13:24 UTC (permalink / raw)
  To: linux-wireless; +Cc: netdev, Luca Coelho, Kalle Valo

Hi,

this is the fix for the recently introduced regression of iwlwifi
driver (since 5.10), a crash at the module loading [1][2].
The first patch actually fixes the bug, and this should go to 5.11-rc.
The second patch is an enhancement to make pointers const for safety.


Takashi

[1] https://bugzilla.suse.com/show_bug.cgi?id=1180344
[2] https://bugzilla.kernel.org/show_bug.cgi?id=210733

===

Takashi Iwai (2):
  iwlwifi: dbg: Don't touch the tlv data
  iwlwifi: dbg: Mark ucode tlv data as const

 .../net/wireless/intel/iwlwifi/iwl-dbg-tlv.c  | 57 +++++++++----------
 .../net/wireless/intel/iwlwifi/iwl-dbg-tlv.h  |  2 +-
 drivers/net/wireless/intel/iwlwifi/iwl-drv.c  |  2 +-
 3 files changed, 28 insertions(+), 33 deletions(-)

-- 
2.26.2


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/2] iwlwifi: dbg: Don't touch the tlv data
  2021-01-12 13:24 [PATCH 0/2] iwlwifi: Fix a crash at loading Takashi Iwai
@ 2021-01-12 13:24 ` Takashi Iwai
  2021-01-12 15:48   ` Kalle Valo
  2021-01-14 16:57   ` [1/2] " Kalle Valo
  2021-01-12 13:24 ` [PATCH 2/2] iwlwifi: dbg: Mark ucode tlv data as const Takashi Iwai
  1 sibling, 2 replies; 13+ messages in thread
From: Takashi Iwai @ 2021-01-12 13:24 UTC (permalink / raw)
  To: linux-wireless; +Cc: netdev, Luca Coelho, Kalle Valo

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(-)

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;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/2] iwlwifi: dbg: Mark ucode tlv data as const
  2021-01-12 13:24 [PATCH 0/2] iwlwifi: Fix a crash at loading Takashi Iwai
  2021-01-12 13:24 ` [PATCH 1/2] iwlwifi: dbg: Don't touch the tlv data Takashi Iwai
@ 2021-01-12 13:24 ` Takashi Iwai
  2021-01-12 15:50   ` Kalle Valo
  2021-02-10 11:35   ` Luca Coelho
  1 sibling, 2 replies; 13+ messages in thread
From: Takashi Iwai @ 2021-01-12 13:24 UTC (permalink / raw)
  To: linux-wireless; +Cc: netdev, Luca Coelho, Kalle Valo

The ucode TLV data may be read-only and should be treated as const
pointers, but currently a few code forcibly cast to the writable
pointer unnecessarily.  This gave developers a wrong impression as if
it can be modified, resulting in crashing regressions already a couple
of times.

This patch adds the const prefix to those cast pointers, so that such
attempt can be caught more easily in future.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 .../net/wireless/intel/iwlwifi/iwl-dbg-tlv.c  | 50 ++++++++++---------
 .../net/wireless/intel/iwlwifi/iwl-dbg-tlv.h  |  2 +-
 drivers/net/wireless/intel/iwlwifi/iwl-drv.c  |  2 +-
 3 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c
index a80a35a7740f..12c49fe8608a 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c
@@ -61,7 +61,8 @@ dbg_ver_table[IWL_DBG_TLV_TYPE_NUM] = {
 	[IWL_DBG_TLV_TYPE_TRIGGER]	= {.min_ver = 1, .max_ver = 1,},
 };
 
-static int iwl_dbg_tlv_add(struct iwl_ucode_tlv *tlv, struct list_head *list)
+static int iwl_dbg_tlv_add(const struct iwl_ucode_tlv *tlv,
+			   struct list_head *list)
 {
 	u32 len = le32_to_cpu(tlv->length);
 	struct iwl_dbg_tlv_node *node;
@@ -76,9 +77,9 @@ static int iwl_dbg_tlv_add(struct iwl_ucode_tlv *tlv, struct list_head *list)
 	return 0;
 }
 
-static bool iwl_dbg_tlv_ver_support(struct iwl_ucode_tlv *tlv)
+static bool iwl_dbg_tlv_ver_support(const struct iwl_ucode_tlv *tlv)
 {
-	struct iwl_fw_ini_header *hdr = (void *)&tlv->data[0];
+	const struct iwl_fw_ini_header *hdr = (const void *)&tlv->data[0];
 	u32 type = le32_to_cpu(tlv->type);
 	u32 tlv_idx = type - IWL_UCODE_TLV_DEBUG_BASE;
 	u32 ver = le32_to_cpu(hdr->version);
@@ -91,9 +92,9 @@ static bool iwl_dbg_tlv_ver_support(struct iwl_ucode_tlv *tlv)
 }
 
 static int iwl_dbg_tlv_alloc_debug_info(struct iwl_trans *trans,
-					struct iwl_ucode_tlv *tlv)
+					const struct iwl_ucode_tlv *tlv)
 {
-	struct iwl_fw_ini_debug_info_tlv *debug_info = (void *)tlv->data;
+	const struct iwl_fw_ini_debug_info_tlv *debug_info = (const void *)tlv->data;
 
 	if (le32_to_cpu(tlv->length) != sizeof(*debug_info))
 		return -EINVAL;
@@ -105,9 +106,9 @@ static int iwl_dbg_tlv_alloc_debug_info(struct iwl_trans *trans,
 }
 
 static int iwl_dbg_tlv_alloc_buf_alloc(struct iwl_trans *trans,
-				       struct iwl_ucode_tlv *tlv)
+				       const struct iwl_ucode_tlv *tlv)
 {
-	struct iwl_fw_ini_allocation_tlv *alloc = (void *)tlv->data;
+	const struct iwl_fw_ini_allocation_tlv *alloc = (const void *)tlv->data;
 	u32 buf_location;
 	u32 alloc_id;
 
@@ -145,9 +146,9 @@ static int iwl_dbg_tlv_alloc_buf_alloc(struct iwl_trans *trans,
 }
 
 static int iwl_dbg_tlv_alloc_hcmd(struct iwl_trans *trans,
-				  struct iwl_ucode_tlv *tlv)
+				  const struct iwl_ucode_tlv *tlv)
 {
-	struct iwl_fw_ini_hcmd_tlv *hcmd = (void *)tlv->data;
+	const struct iwl_fw_ini_hcmd_tlv *hcmd = (const void *)tlv->data;
 	u32 tp = le32_to_cpu(hcmd->time_point);
 
 	if (le32_to_cpu(tlv->length) <= sizeof(*hcmd))
@@ -169,9 +170,9 @@ static int iwl_dbg_tlv_alloc_hcmd(struct iwl_trans *trans,
 }
 
 static int iwl_dbg_tlv_alloc_region(struct iwl_trans *trans,
-				    struct iwl_ucode_tlv *tlv)
+				    const struct iwl_ucode_tlv *tlv)
 {
-	struct iwl_fw_ini_region_tlv *reg = (void *)tlv->data;
+	const struct iwl_fw_ini_region_tlv *reg = (const void *)tlv->data;
 	struct iwl_ucode_tlv **active_reg;
 	u32 id = le32_to_cpu(reg->id);
 	u32 type = le32_to_cpu(reg->type);
@@ -214,9 +215,10 @@ static int iwl_dbg_tlv_alloc_region(struct iwl_trans *trans,
 }
 
 static int iwl_dbg_tlv_alloc_trigger(struct iwl_trans *trans,
-				     struct iwl_ucode_tlv *tlv)
+				     const struct iwl_ucode_tlv *tlv)
 {
-	struct iwl_fw_ini_trigger_tlv *trig = (void *)tlv->data;
+	const struct iwl_fw_ini_trigger_tlv *trig = (const void *)tlv->data;
+	struct iwl_fw_ini_trigger_tlv *dup_trig;
 	u32 tp = le32_to_cpu(trig->time_point);
 	struct iwl_ucode_tlv *dup = NULL;
 	int ret;
@@ -237,8 +239,8 @@ static int iwl_dbg_tlv_alloc_trigger(struct iwl_trans *trans,
 				GFP_KERNEL);
 		if (!dup)
 			return -ENOMEM;
-		trig = (void *)dup->data;
-		trig->occurrences = cpu_to_le32(-1);
+		dup_trig = (void *)dup->data;
+		dup_trig->occurrences = cpu_to_le32(-1);
 		tlv = dup;
 	}
 
@@ -249,7 +251,7 @@ static int iwl_dbg_tlv_alloc_trigger(struct iwl_trans *trans,
 }
 
 static int (*dbg_tlv_alloc[])(struct iwl_trans *trans,
-			      struct iwl_ucode_tlv *tlv) = {
+			      const struct iwl_ucode_tlv *tlv) = {
 	[IWL_DBG_TLV_TYPE_DEBUG_INFO]	= iwl_dbg_tlv_alloc_debug_info,
 	[IWL_DBG_TLV_TYPE_BUF_ALLOC]	= iwl_dbg_tlv_alloc_buf_alloc,
 	[IWL_DBG_TLV_TYPE_HCMD]		= iwl_dbg_tlv_alloc_hcmd,
@@ -257,10 +259,10 @@ static int (*dbg_tlv_alloc[])(struct iwl_trans *trans,
 	[IWL_DBG_TLV_TYPE_TRIGGER]	= iwl_dbg_tlv_alloc_trigger,
 };
 
-void iwl_dbg_tlv_alloc(struct iwl_trans *trans, struct iwl_ucode_tlv *tlv,
+void iwl_dbg_tlv_alloc(struct iwl_trans *trans, const struct iwl_ucode_tlv *tlv,
 		       bool ext)
 {
-	struct iwl_fw_ini_header *hdr = (void *)&tlv->data[0];
+	const struct iwl_fw_ini_header *hdr = (const void *)&tlv->data[0];
 	u32 type = le32_to_cpu(tlv->type);
 	u32 tlv_idx = type - IWL_UCODE_TLV_DEBUG_BASE;
 	u32 domain = le32_to_cpu(hdr->domain);
@@ -396,7 +398,7 @@ void iwl_dbg_tlv_free(struct iwl_trans *trans)
 static int iwl_dbg_tlv_parse_bin(struct iwl_trans *trans, const u8 *data,
 				 size_t len)
 {
-	struct iwl_ucode_tlv *tlv;
+	const struct iwl_ucode_tlv *tlv;
 	u32 tlv_len;
 
 	while (len >= sizeof(*tlv)) {
@@ -737,12 +739,12 @@ static void iwl_dbg_tlv_set_periodic_trigs(struct iwl_fw_runtime *fwrt)
 	}
 }
 
-static bool is_trig_data_contained(struct iwl_ucode_tlv *new,
-				   struct iwl_ucode_tlv *old)
+static bool is_trig_data_contained(const struct iwl_ucode_tlv *new,
+				   const struct iwl_ucode_tlv *old)
 {
-	struct iwl_fw_ini_trigger_tlv *new_trig = (void *)new->data;
-	struct iwl_fw_ini_trigger_tlv *old_trig = (void *)old->data;
-	__le32 *new_data = new_trig->data, *old_data = old_trig->data;
+	const struct iwl_fw_ini_trigger_tlv *new_trig = (const void *)new->data;
+	const struct iwl_fw_ini_trigger_tlv *old_trig = (const void *)old->data;
+	const __le32 *new_data = new_trig->data, *old_data = old_trig->data;
 	u32 new_dwords_num = iwl_tlv_array_len(new, new_trig, data);
 	u32 old_dwords_num = iwl_tlv_array_len(old, old_trig, data);
 	int i, j;
diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.h b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.h
index 246823878281..e9f19ecbc4ee 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.h
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.h
@@ -43,7 +43,7 @@ struct iwl_fw_runtime;
 
 void iwl_dbg_tlv_load_bin(struct device *dev, struct iwl_trans *trans);
 void iwl_dbg_tlv_free(struct iwl_trans *trans);
-void iwl_dbg_tlv_alloc(struct iwl_trans *trans, struct iwl_ucode_tlv *tlv,
+void iwl_dbg_tlv_alloc(struct iwl_trans *trans, const struct iwl_ucode_tlv *tlv,
 		       bool ext);
 void iwl_dbg_tlv_init(struct iwl_trans *trans);
 void iwl_dbg_tlv_time_point(struct iwl_fw_runtime *fwrt,
diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
index d44bc61c34f5..5dcc490729b4 100644
--- a/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
+++ b/drivers/net/wireless/intel/iwlwifi/iwl-drv.c
@@ -558,7 +558,7 @@ static int iwl_parse_tlv_firmware(struct iwl_drv *drv,
 				bool *usniffer_images)
 {
 	struct iwl_tlv_ucode_header *ucode = (void *)ucode_raw->data;
-	struct iwl_ucode_tlv *tlv;
+	const struct iwl_ucode_tlv *tlv;
 	size_t len = ucode_raw->size;
 	const u8 *data;
 	u32 tlv_len;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] iwlwifi: dbg: Don't touch the tlv data
  2021-01-12 13:24 ` [PATCH 1/2] iwlwifi: dbg: Don't touch the tlv data Takashi Iwai
@ 2021-01-12 15:48   ` Kalle Valo
  2021-01-12 16:02     ` Takashi Iwai
  2021-01-14 16:57   ` [1/2] " Kalle Valo
  1 sibling, 1 reply; 13+ messages in thread
From: Kalle Valo @ 2021-01-12 15:48 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: linux-wireless, netdev, Luca Coelho

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?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] iwlwifi: dbg: Mark ucode tlv data as const
  2021-01-12 13:24 ` [PATCH 2/2] iwlwifi: dbg: Mark ucode tlv data as const Takashi Iwai
@ 2021-01-12 15:50   ` Kalle Valo
  2021-01-12 16:05     ` Takashi Iwai
  2021-02-10 11:35   ` Luca Coelho
  1 sibling, 1 reply; 13+ messages in thread
From: Kalle Valo @ 2021-01-12 15:50 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: linux-wireless, netdev, Luca Coelho

Takashi Iwai <tiwai@suse.de> writes:

> The ucode TLV data may be read-only and should be treated as const
> pointers, but currently a few code forcibly cast to the writable
> pointer unnecessarily.  This gave developers a wrong impression as if
> it can be modified, resulting in crashing regressions already a couple
> of times.
>
> This patch adds the const prefix to those cast pointers, so that such
> attempt can be caught more easily in future.
>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

So this need to go to -next, right? Does this depend on patch 1 or can
this be applied independently?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] iwlwifi: dbg: Don't touch the tlv data
  2021-01-12 15:48   ` Kalle Valo
@ 2021-01-12 16:02     ` Takashi Iwai
  2021-01-12 17:08       ` Coelho, Luciano
  0 siblings, 1 reply; 13+ messages in thread
From: Takashi Iwai @ 2021-01-12 16:02 UTC (permalink / raw)
  To: Kalle Valo; +Cc: linux-wireless, netdev, Luca Coelho

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] iwlwifi: dbg: Mark ucode tlv data as const
  2021-01-12 15:50   ` Kalle Valo
@ 2021-01-12 16:05     ` Takashi Iwai
  2021-01-12 17:13       ` Coelho, Luciano
  0 siblings, 1 reply; 13+ messages in thread
From: Takashi Iwai @ 2021-01-12 16:05 UTC (permalink / raw)
  To: Kalle Valo; +Cc: linux-wireless, netdev, Luca Coelho

On Tue, 12 Jan 2021 16:50:54 +0100,
Kalle Valo wrote:
> 
> Takashi Iwai <tiwai@suse.de> writes:
> 
> > The ucode TLV data may be read-only and should be treated as const
> > pointers, but currently a few code forcibly cast to the writable
> > pointer unnecessarily.  This gave developers a wrong impression as if
> > it can be modified, resulting in crashing regressions already a couple
> > of times.
> >
> > This patch adds the const prefix to those cast pointers, so that such
> > attempt can be caught more easily in future.
> >
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> So this need to go to -next, right?

Yes, this isn't urgently needed for 5.11.

> Does this depend on patch 1 or can
> this be applied independently?

It depends on the first patch, otherwise you'll get the warning in the
code changing the const data (it must warn -- that's the purpose of
this change :)

So, if applying to a separate branch is difficult, applying together
for 5.11 would be an option.


thanks,

Takashi

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] iwlwifi: dbg: Don't touch the tlv data
  2021-01-12 16:02     ` Takashi Iwai
@ 2021-01-12 17:08       ` Coelho, Luciano
  2021-01-14  7:14         ` Kalle Valo
  0 siblings, 1 reply; 13+ messages in thread
From: Coelho, Luciano @ 2021-01-12 17:08 UTC (permalink / raw)
  To: kvalo, tiwai; +Cc: linux-wireless, netdev

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] iwlwifi: dbg: Mark ucode tlv data as const
  2021-01-12 16:05     ` Takashi Iwai
@ 2021-01-12 17:13       ` Coelho, Luciano
  2021-01-13  6:50         ` Kalle Valo
  0 siblings, 1 reply; 13+ messages in thread
From: Coelho, Luciano @ 2021-01-12 17:13 UTC (permalink / raw)
  To: kvalo, tiwai; +Cc: linux-wireless, netdev

On Tue, 2021-01-12 at 17:05 +0100, Takashi Iwai wrote:
> On Tue, 12 Jan 2021 16:50:54 +0100,
> Kalle Valo wrote:
> > 
> > Takashi Iwai <tiwai@suse.de> writes:
> > 
> > > The ucode TLV data may be read-only and should be treated as const
> > > pointers, but currently a few code forcibly cast to the writable
> > > pointer unnecessarily.  This gave developers a wrong impression as if
> > > it can be modified, resulting in crashing regressions already a couple
> > > of times.
> > > 
> > > This patch adds the const prefix to those cast pointers, so that such
> > > attempt can be caught more easily in future.
> > > 
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > 
> > So this need to go to -next, right?
> 
> Yes, this isn't urgently needed for 5.11.

Acked-by: Luca Coelho <luciano.coelho@intel.com>


> > Does this depend on patch 1 or can
> > this be applied independently?
> 
> It depends on the first patch, otherwise you'll get the warning in the
> code changing the const data (it must warn -- that's the purpose of
> this change :)
> 
> So, if applying to a separate branch is difficult, applying together
> for 5.11 would be an option.

It doesn't matter to me how you apply it.  Applying together is
obviously going to be easier, but applying separately wouldn't be that
hard either.  You'd just have to track when 1/2 went into net-next
before applying this one.  Kalle's call.

--
Cheers,
Luca.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] iwlwifi: dbg: Mark ucode tlv data as const
  2021-01-12 17:13       ` Coelho, Luciano
@ 2021-01-13  6:50         ` Kalle Valo
  0 siblings, 0 replies; 13+ messages in thread
From: Kalle Valo @ 2021-01-13  6:50 UTC (permalink / raw)
  To: Coelho, Luciano; +Cc: tiwai, linux-wireless, netdev

"Coelho, Luciano" <luciano.coelho@intel.com> writes:

> On Tue, 2021-01-12 at 17:05 +0100, Takashi Iwai wrote:
>> On Tue, 12 Jan 2021 16:50:54 +0100,
>> Kalle Valo wrote:
>> > 
>> > Takashi Iwai <tiwai@suse.de> writes:
>> > 
>> > > The ucode TLV data may be read-only and should be treated as const
>> > > pointers, but currently a few code forcibly cast to the writable
>> > > pointer unnecessarily.  This gave developers a wrong impression as if
>> > > it can be modified, resulting in crashing regressions already a couple
>> > > of times.
>> > > 
>> > > This patch adds the const prefix to those cast pointers, so that such
>> > > attempt can be caught more easily in future.
>> > > 
>> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
>> > 
>> > So this need to go to -next, right?
>> 
>> Yes, this isn't urgently needed for 5.11.
>
> Acked-by: Luca Coelho <luciano.coelho@intel.com>
>
>
>> > Does this depend on patch 1 or can
>> > this be applied independently?
>> 
>> It depends on the first patch, otherwise you'll get the warning in the
>> code changing the const data (it must warn -- that's the purpose of
>> this change :)
>> 
>> So, if applying to a separate branch is difficult, applying together
>> for 5.11 would be an option.
>
> It doesn't matter to me how you apply it.  Applying together is
> obviously going to be easier, but applying separately wouldn't be that
> hard either.  You'd just have to track when 1/2 went into net-next
> before applying this one.  Kalle's call.

Ok, I'll apply this to wireless-drivers-next after wireless-drivers is
merged to -next. It might take a while.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] iwlwifi: dbg: Don't touch the tlv data
  2021-01-12 17:08       ` Coelho, Luciano
@ 2021-01-14  7:14         ` Kalle Valo
  0 siblings, 0 replies; 13+ messages in thread
From: Kalle Valo @ 2021-01-14  7:14 UTC (permalink / raw)
  To: Coelho, Luciano; +Cc: tiwai, linux-wireless, netdev

"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.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [1/2] iwlwifi: dbg: Don't touch the tlv data
  2021-01-12 13:24 ` [PATCH 1/2] iwlwifi: dbg: Don't touch the tlv data Takashi Iwai
  2021-01-12 15:48   ` Kalle Valo
@ 2021-01-14 16:57   ` Kalle Valo
  1 sibling, 0 replies; 13+ messages in thread
From: Kalle Valo @ 2021-01-14 16:57 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: linux-wireless, netdev, Luca Coelho

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

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20210112132449.22243-2-tiwai@suse.de/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] iwlwifi: dbg: Mark ucode tlv data as const
  2021-01-12 13:24 ` [PATCH 2/2] iwlwifi: dbg: Mark ucode tlv data as const Takashi Iwai
  2021-01-12 15:50   ` Kalle Valo
@ 2021-02-10 11:35   ` Luca Coelho
  1 sibling, 0 replies; 13+ messages in thread
From: Luca Coelho @ 2021-02-10 11:35 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: linux-wireless, netdev, Luca Coelho, Kalle Valo

Takashi Iwai <tiwai@suse.de> wrote:

> The ucode TLV data may be read-only and should be treated as const
> pointers, but currently a few code forcibly cast to the writable
> pointer unnecessarily.  This gave developers a wrong impression as if
> it can be modified, resulting in crashing regressions already a couple
> of times.
> 
> This patch adds the const prefix to those cast pointers, so that such
> attempt can be caught more easily in future.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> Acked-by: Luca Coelho <luciano.coelho@intel.com>

Patch applied to iwlwifi-next.git, thanks.

71b6254a6c98 iwlwifi: dbg: Mark ucode tlv data as const


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-02-10 11:37 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-12 13:24 [PATCH 0/2] iwlwifi: Fix a crash at loading Takashi Iwai
2021-01-12 13:24 ` [PATCH 1/2] iwlwifi: dbg: Don't touch the tlv data Takashi Iwai
2021-01-12 15:48   ` Kalle Valo
2021-01-12 16:02     ` Takashi Iwai
2021-01-12 17:08       ` Coelho, Luciano
2021-01-14  7:14         ` Kalle Valo
2021-01-14 16:57   ` [1/2] " Kalle Valo
2021-01-12 13:24 ` [PATCH 2/2] iwlwifi: dbg: Mark ucode tlv data as const Takashi Iwai
2021-01-12 15:50   ` Kalle Valo
2021-01-12 16:05     ` Takashi Iwai
2021-01-12 17:13       ` Coelho, Luciano
2021-01-13  6:50         ` Kalle Valo
2021-02-10 11:35   ` Luca Coelho

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.