| Message ID | 20220411154210.1870008-1-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7635a1ad8d92dcc8247b53f949e37795154b5b6f |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | iwlwifi: iwl-dbg: Use del_timer_sync() before freeing | expand |
On Mon, 2022-04-11 at 08:42 -0700, Guenter Roeck wrote: > In Chrome OS, a large number of crashes is observed due to corrupted > timer > lists. Steven Rostedt pointed out that this usually happens when a > timer > is freed while still active, and that the problem is often triggered > by code calling del_timer() instead of del_timer_sync() just before > freeing. > > Steven also identified the iwlwifi driver as one of the possible > culprits > since it does exactly that. > > Reported-by: Steven Rostedt <rostedt@goodmis.org> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Johannes Berg <johannes.berg@intel.com> > Cc: Gregory Greenman <gregory.greenman@intel.com> > Fixes: 60e8abd9d3e91 ("iwlwifi: dbg_ini: add periodic trigger new API > support") > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > v1 (from RFC): > Removed Shahar S Matityahu from Cc: and added Gregory Greenman. > No functional change. > > I thought about the need to add a mutex to protect the timer list, > but > I convinced myself that it is not necessary because the code adding > the timer list and the code removing it should never be never > executed > in parallel. > > drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c > b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c > index 866a33f49915..3237d4b528b5 100644 > --- a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c > +++ b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c > @@ -371,7 +371,7 @@ void iwl_dbg_tlv_del_timers(struct iwl_trans > *trans) > struct iwl_dbg_tlv_timer_node *node, *tmp; > > list_for_each_entry_safe(node, tmp, timer_list, list) { > - del_timer(&node->timer); > + del_timer_sync(&node->timer); > list_del(&node->list); > kfree(node); > } Hi Kalle, Can you please pick it up to wireless-drivers for the next rc? It is an important fix. Luca has already assigned it to you in patchwork. Thanks! Acked-by: Gregory Greenman <gregory.greenman@intel.com>
On Wed, Apr 13, 2022 at 11:56 AM Greenman, Gregory <gregory.greenman@intel.com> wrote: > > > On Mon, 2022-04-11 at 08:42 -0700, Guenter Roeck wrote: > > In Chrome OS, a large number of crashes is observed due to corrupted > > timer > > lists. Steven Rostedt pointed out that this usually happens when a > > timer > > is freed while still active, and that the problem is often triggered > > by code calling del_timer() instead of del_timer_sync() just before > > freeing. > > > > Steven also identified the iwlwifi driver as one of the possible > > culprits > > since it does exactly that. > > > > Reported-by: Steven Rostedt <rostedt@goodmis.org> > > Cc: Steven Rostedt <rostedt@goodmis.org> > > Cc: Johannes Berg <johannes.berg@intel.com> > > Cc: Gregory Greenman <gregory.greenman@intel.com> > > Fixes: 60e8abd9d3e91 ("iwlwifi: dbg_ini: add periodic trigger new API > > support") > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > > --- > > v1 (from RFC): > > Removed Shahar S Matityahu from Cc: and added Gregory Greenman. > > No functional change. > > > > I thought about the need to add a mutex to protect the timer list, > > but > > I convinced myself that it is not necessary because the code adding > > the timer list and the code removing it should never be never > > executed > > in parallel. > > > > drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c > > b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c > > index 866a33f49915..3237d4b528b5 100644 > > --- a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c > > +++ b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c > > @@ -371,7 +371,7 @@ void iwl_dbg_tlv_del_timers(struct iwl_trans > > *trans) > > struct iwl_dbg_tlv_timer_node *node, *tmp; > > > > list_for_each_entry_safe(node, tmp, timer_list, list) { > > - del_timer(&node->timer); > > + del_timer_sync(&node->timer); > > list_del(&node->list); > > kfree(node); > > } > > Hi Kalle, > > Can you please pick it up to wireless-drivers for the next rc? > It is an important fix. > Luca has already assigned it to you in patchwork. > > Thanks! > > Acked-by: Gregory Greenman <gregory.greenman@intel.com> I have tested this on top of Linux v5.17.3-rc1. Feel free to add my... Tested-by: Sedat Dilek <sedat.dilek@gmail.com> # Linux v5.17.3-rc1 and Debian LLVM-14 - Sedat -
Guenter Roeck <linux@roeck-us.net> wrote: > In Chrome OS, a large number of crashes is observed due to corrupted timer > lists. Steven Rostedt pointed out that this usually happens when a timer > is freed while still active, and that the problem is often triggered > by code calling del_timer() instead of del_timer_sync() just before > freeing. > > Steven also identified the iwlwifi driver as one of the possible culprits > since it does exactly that. > > Reported-by: Steven Rostedt <rostedt@goodmis.org> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Johannes Berg <johannes.berg@intel.com> > Cc: Gregory Greenman <gregory.greenman@intel.com> > Fixes: 60e8abd9d3e91 ("iwlwifi: dbg_ini: add periodic trigger new API support") > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > Acked-by: Gregory Greenman <gregory.greenman@intel.com> > Tested-by: Sedat Dilek <sedat.dilek@gmail.com> # Linux v5.17.3-rc1 and Debian LLVM-14 Patch applied to wireless.git, thanks. 7635a1ad8d92 iwlwifi: iwl-dbg: Use del_timer_sync() before freeing
Sedat Dilek <sedat.dilek@gmail.com> writes: > On Wed, Apr 13, 2022 at 11:56 AM Greenman, Gregory > <gregory.greenman@intel.com> wrote: >> >> >> On Mon, 2022-04-11 at 08:42 -0700, Guenter Roeck wrote: >> > In Chrome OS, a large number of crashes is observed due to corrupted >> > timer >> > lists. Steven Rostedt pointed out that this usually happens when a >> > timer >> > is freed while still active, and that the problem is often triggered >> > by code calling del_timer() instead of del_timer_sync() just before >> > freeing. >> > >> > Steven also identified the iwlwifi driver as one of the possible >> > culprits >> > since it does exactly that. >> > >> > Reported-by: Steven Rostedt <rostedt@goodmis.org> >> > Cc: Steven Rostedt <rostedt@goodmis.org> >> > Cc: Johannes Berg <johannes.berg@intel.com> >> > Cc: Gregory Greenman <gregory.greenman@intel.com> >> > Fixes: 60e8abd9d3e91 ("iwlwifi: dbg_ini: add periodic trigger new API >> > support") >> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> >> > --- >> > v1 (from RFC): >> > Removed Shahar S Matityahu from Cc: and added Gregory Greenman. >> > No functional change. >> > >> > I thought about the need to add a mutex to protect the timer list, >> > but >> > I convinced myself that it is not necessary because the code adding >> > the timer list and the code removing it should never be never >> > executed >> > in parallel. >> > >> > drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c >> > b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c >> > index 866a33f49915..3237d4b528b5 100644 >> > --- a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c >> > +++ b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c >> > @@ -371,7 +371,7 @@ void iwl_dbg_tlv_del_timers(struct iwl_trans >> > *trans) >> > struct iwl_dbg_tlv_timer_node *node, *tmp; >> > >> > list_for_each_entry_safe(node, tmp, timer_list, list) { >> > - del_timer(&node->timer); >> > + del_timer_sync(&node->timer); >> > list_del(&node->list); >> > kfree(node); >> > } >> >> Hi Kalle, >> >> Can you please pick it up to wireless-drivers for the next rc? >> It is an important fix. >> Luca has already assigned it to you in patchwork. >> >> Thanks! >> >> Acked-by: Gregory Greenman <gregory.greenman@intel.com> > > I have tested this on top of Linux v5.17.3-rc1. > > Feel free to add my... > > Tested-by: Sedat Dilek <sedat.dilek@gmail.com> # Linux v5.17.3-rc1 and > Debian LLVM-14 Please keep the Tested-by in one line, otherwise patchwork cannot parse it correctly. I fixed this manually during commit.
diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c index 866a33f49915..3237d4b528b5 100644 --- a/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c +++ b/drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c @@ -371,7 +371,7 @@ void iwl_dbg_tlv_del_timers(struct iwl_trans *trans) struct iwl_dbg_tlv_timer_node *node, *tmp; list_for_each_entry_safe(node, tmp, timer_list, list) { - del_timer(&node->timer); + del_timer_sync(&node->timer); list_del(&node->list); kfree(node); }
In Chrome OS, a large number of crashes is observed due to corrupted timer lists. Steven Rostedt pointed out that this usually happens when a timer is freed while still active, and that the problem is often triggered by code calling del_timer() instead of del_timer_sync() just before freeing. Steven also identified the iwlwifi driver as one of the possible culprits since it does exactly that. Reported-by: Steven Rostedt <rostedt@goodmis.org> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Johannes Berg <johannes.berg@intel.com> Cc: Gregory Greenman <gregory.greenman@intel.com> Fixes: 60e8abd9d3e91 ("iwlwifi: dbg_ini: add periodic trigger new API support") Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- v1 (from RFC): Removed Shahar S Matityahu from Cc: and added Gregory Greenman. No functional change. I thought about the need to add a mutex to protect the timer list, but I convinced myself that it is not necessary because the code adding the timer list and the code removing it should never be never executed in parallel. drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)