* [bug report] iwlwifi: integrate with iwlmei
@ 2021-11-30 6:20 Dan Carpenter
2021-11-30 8:09 ` Grumbach, Emmanuel
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2021-11-30 6:20 UTC (permalink / raw)
To: emmanuel.grumbach; +Cc: linux-wireless
Hello Emmanuel Grumbach,
The patch 6d19a5eba5cd: "iwlwifi: integrate with iwlmei" from Nov 12,
2021, leads to the following Smatch static checker warning:
drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c:2640 iwl_mvm_start_ap_ibss()
error: NULL dereference inside function '__iwl_mvm_mac_set_key()'
drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c
2559 static int iwl_mvm_start_ap_ibss(struct ieee80211_hw *hw,
2560 struct ieee80211_vif *vif)
2561 {
2562 struct iwl_mvm *mvm = IWL_MAC80211_GET_MVM(hw);
2563 struct iwl_mvm_vif *mvmvif = iwl_mvm_vif_from_mac80211(vif);
2564 int ret, i;
2565
2566 mutex_lock(&mvm->mutex);
2567
2568 /* Send the beacon template */
2569 ret = iwl_mvm_mac_ctxt_beacon_changed(mvm, vif);
2570 if (ret)
2571 goto out_unlock;
2572
2573 /*
2574 * Re-calculate the tsf id, as the leader-follower relations depend on
2575 * the beacon interval, which was not known when the AP interface
2576 * was added.
2577 */
2578 if (vif->type == NL80211_IFTYPE_AP)
2579 iwl_mvm_mac_ctxt_recalc_tsf_id(mvm, vif);
2580
2581 mvmvif->ap_assoc_sta_count = 0;
2582
2583 /* Add the mac context */
2584 ret = iwl_mvm_mac_ctxt_add(mvm, vif);
2585 if (ret)
2586 goto out_unlock;
2587
2588 /* Perform the binding */
2589 ret = iwl_mvm_binding_add_vif(mvm, vif);
2590 if (ret)
2591 goto out_remove;
2592
2593 /*
2594 * This is not very nice, but the simplest:
2595 * For older FWs adding the mcast sta before the bcast station may
2596 * cause assert 0x2b00.
2597 * This is fixed in later FW so make the order of removal depend on
2598 * the TLV
2599 */
2600 if (fw_has_api(&mvm->fw->ucode_capa, IWL_UCODE_TLV_API_STA_TYPE)) {
2601 ret = iwl_mvm_add_mcast_sta(mvm, vif);
2602 if (ret)
2603 goto out_unbind;
2604 /*
2605 * Send the bcast station. At this stage the TBTT and DTIM time
2606 * events are added and applied to the scheduler
2607 */
2608 ret = iwl_mvm_send_add_bcast_sta(mvm, vif);
2609 if (ret) {
2610 iwl_mvm_rm_mcast_sta(mvm, vif);
2611 goto out_unbind;
2612 }
2613 } else {
2614 /*
2615 * Send the bcast station. At this stage the TBTT and DTIM time
2616 * events are added and applied to the scheduler
2617 */
2618 ret = iwl_mvm_send_add_bcast_sta(mvm, vif);
2619 if (ret)
2620 goto out_unbind;
2621 ret = iwl_mvm_add_mcast_sta(mvm, vif);
2622 if (ret) {
2623 iwl_mvm_send_rm_bcast_sta(mvm, vif);
2624 goto out_unbind;
2625 }
2626 }
2627
2628 /* must be set before quota calculations */
2629 mvmvif->ap_ibss_active = true;
2630
2631 /* send all the early keys to the device now */
2632 for (i = 0; i < ARRAY_SIZE(mvmvif->ap_early_keys); i++) {
2633 struct ieee80211_key_conf *key = mvmvif->ap_early_keys[i];
2634
2635 if (!key)
2636 continue;
2637
2638 mvmvif->ap_early_keys[i] = NULL;
2639
--> 2640 ret = __iwl_mvm_mac_set_key(hw, SET_KEY, vif, NULL, key);
^^^^
This passes a NULL "sta" and now it will always crash. (Possibly it
used to sometimes crash before your patch but the static checker does
not mind about that. :P).
2641 if (ret)
2642 goto out_quota_failed;
2643 }
2644
2645 if (vif->type == NL80211_IFTYPE_AP && !vif->p2p) {
2646 iwl_mvm_vif_set_low_latency(mvmvif, true,
2647 LOW_LATENCY_VIF_TYPE);
2648 iwl_mvm_send_low_latency_cmd(mvm, true, mvmvif->id);
2649 }
2650
2651 /* power updated needs to be done before quotas */
2652 iwl_mvm_power_update_mac(mvm);
2653
2654 ret = iwl_mvm_update_quotas(mvm, false, NULL);
2655 if (ret)
2656 goto out_quota_failed;
2657
2658 /* Need to update the P2P Device MAC (only GO, IBSS is single vif) */
2659 if (vif->p2p && mvm->p2p_device_vif)
2660 iwl_mvm_mac_ctxt_changed(mvm, mvm->p2p_device_vif, false, NULL);
2661
2662 iwl_mvm_bt_coex_vif_change(mvm);
2663
2664 /* we don't support TDLS during DCM */
2665 if (iwl_mvm_phy_ctx_count(mvm) > 1)
2666 iwl_mvm_teardown_tdls_peers(mvm);
2667
2668 iwl_mvm_ftm_restart_responder(mvm, vif);
2669
2670 goto out_unlock;
2671
2672 out_quota_failed:
2673 iwl_mvm_power_update_mac(mvm);
2674 mvmvif->ap_ibss_active = false;
2675 iwl_mvm_send_rm_bcast_sta(mvm, vif);
2676 iwl_mvm_rm_mcast_sta(mvm, vif);
2677 out_unbind:
2678 iwl_mvm_binding_remove_vif(mvm, vif);
2679 out_remove:
2680 iwl_mvm_mac_ctxt_remove(mvm, vif);
2681 out_unlock:
2682 mutex_unlock(&mvm->mutex);
2683 return ret;
2684 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [bug report] iwlwifi: integrate with iwlmei
2021-11-30 6:20 [bug report] iwlwifi: integrate with iwlmei Dan Carpenter
@ 2021-11-30 8:09 ` Grumbach, Emmanuel
2021-11-30 9:07 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Grumbach, Emmanuel @ 2021-11-30 8:09 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-wireless
Hi Dan,
> Hello Emmanuel Grumbach,
>
> The patch 6d19a5eba5cd: "iwlwifi: integrate with iwlmei" from Nov 12, 2021,
> leads to the following Smatch static checker warning:
>
> drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c:2640
> iwl_mvm_start_ap_ibss()
> error: NULL dereference inside function
> '__iwl_mvm_mac_set_key()'
Where in __iwl_mvm_mac_set_key() ?
This function should be able to cope with with a NULL sta I think.
I don't really see how this could be related to my patch since iwlmei is not related to AP mode at all.
I also moved to that commit, but the line numbers don't match, so I am a bit confused.
>
> drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c
> 2559 static int iwl_mvm_start_ap_ibss(struct ieee80211_hw *hw,
> 2560 struct ieee80211_vif *vif)
> 2561 {
> 2562 struct iwl_mvm *mvm = IWL_MAC80211_GET_MVM(hw);
> 2563 struct iwl_mvm_vif *mvmvif = iwl_mvm_vif_from_mac80211(vif);
> 2564 int ret, i;
> 2565
> 2566 mutex_lock(&mvm->mutex);
> 2567
> 2568 /* Send the beacon template */
> 2569 ret = iwl_mvm_mac_ctxt_beacon_changed(mvm, vif);
> 2570 if (ret)
> 2571 goto out_unlock;
> 2572
> 2573 /*
> 2574 * Re-calculate the tsf id, as the leader-follower relations depend
> on
> 2575 * the beacon interval, which was not known when the AP
> interface
> 2576 * was added.
> 2577 */
> 2578 if (vif->type == NL80211_IFTYPE_AP)
> 2579 iwl_mvm_mac_ctxt_recalc_tsf_id(mvm, vif);
> 2580
> 2581 mvmvif->ap_assoc_sta_count = 0;
> 2582
> 2583 /* Add the mac context */
> 2584 ret = iwl_mvm_mac_ctxt_add(mvm, vif);
> 2585 if (ret)
> 2586 goto out_unlock;
> 2587
> 2588 /* Perform the binding */
> 2589 ret = iwl_mvm_binding_add_vif(mvm, vif);
> 2590 if (ret)
> 2591 goto out_remove;
> 2592
> 2593 /*
> 2594 * This is not very nice, but the simplest:
> 2595 * For older FWs adding the mcast sta before the bcast station
> may
> 2596 * cause assert 0x2b00.
> 2597 * This is fixed in later FW so make the order of removal depend
> on
> 2598 * the TLV
> 2599 */
> 2600 if (fw_has_api(&mvm->fw->ucode_capa,
> IWL_UCODE_TLV_API_STA_TYPE)) {
> 2601 ret = iwl_mvm_add_mcast_sta(mvm, vif);
> 2602 if (ret)
> 2603 goto out_unbind;
> 2604 /*
> 2605 * Send the bcast station. At this stage the TBTT and DTIM
> time
> 2606 * events are added and applied to the scheduler
> 2607 */
> 2608 ret = iwl_mvm_send_add_bcast_sta(mvm, vif);
> 2609 if (ret) {
> 2610 iwl_mvm_rm_mcast_sta(mvm, vif);
> 2611 goto out_unbind;
> 2612 }
> 2613 } else {
> 2614 /*
> 2615 * Send the bcast station. At this stage the TBTT and DTIM
> time
> 2616 * events are added and applied to the scheduler
> 2617 */
> 2618 ret = iwl_mvm_send_add_bcast_sta(mvm, vif);
> 2619 if (ret)
> 2620 goto out_unbind;
> 2621 ret = iwl_mvm_add_mcast_sta(mvm, vif);
> 2622 if (ret) {
> 2623 iwl_mvm_send_rm_bcast_sta(mvm, vif);
> 2624 goto out_unbind;
> 2625 }
> 2626 }
> 2627
> 2628 /* must be set before quota calculations */
> 2629 mvmvif->ap_ibss_active = true;
> 2630
> 2631 /* send all the early keys to the device now */
> 2632 for (i = 0; i < ARRAY_SIZE(mvmvif->ap_early_keys); i++) {
> 2633 struct ieee80211_key_conf *key = mvmvif->ap_early_keys[i];
> 2634
> 2635 if (!key)
> 2636 continue;
> 2637
> 2638 mvmvif->ap_early_keys[i] = NULL;
> 2639
> --> 2640 ret = __iwl_mvm_mac_set_key(hw, SET_KEY, vif, NULL,
> key);
> ^^^^ This passes a NULL "sta" and now it
> will always crash. (Possibly it used to sometimes crash before your patch but
> the static checker does not mind about that. :P).
This existed long before my patch
>
> 2641 if (ret)
> 2642 goto out_quota_failed;
> 2643 }
> 2644
> 2645 if (vif->type == NL80211_IFTYPE_AP && !vif->p2p) {
> 2646 iwl_mvm_vif_set_low_latency(mvmvif, true,
> 2647 LOW_LATENCY_VIF_TYPE);
> 2648 iwl_mvm_send_low_latency_cmd(mvm, true, mvmvif->id);
> 2649 }
> 2650
> 2651 /* power updated needs to be done before quotas */
> 2652 iwl_mvm_power_update_mac(mvm);
> 2653
> 2654 ret = iwl_mvm_update_quotas(mvm, false, NULL);
> 2655 if (ret)
> 2656 goto out_quota_failed;
> 2657
> 2658 /* Need to update the P2P Device MAC (only GO, IBSS is single
> vif) */
> 2659 if (vif->p2p && mvm->p2p_device_vif)
> 2660 iwl_mvm_mac_ctxt_changed(mvm, mvm->p2p_device_vif,
> false, NULL);
> 2661
> 2662 iwl_mvm_bt_coex_vif_change(mvm);
> 2663
> 2664 /* we don't support TDLS during DCM */
> 2665 if (iwl_mvm_phy_ctx_count(mvm) > 1)
> 2666 iwl_mvm_teardown_tdls_peers(mvm);
> 2667
> 2668 iwl_mvm_ftm_restart_responder(mvm, vif);
> 2669
> 2670 goto out_unlock;
> 2671
> 2672 out_quota_failed:
> 2673 iwl_mvm_power_update_mac(mvm);
> 2674 mvmvif->ap_ibss_active = false;
> 2675 iwl_mvm_send_rm_bcast_sta(mvm, vif);
> 2676 iwl_mvm_rm_mcast_sta(mvm, vif);
> 2677 out_unbind:
> 2678 iwl_mvm_binding_remove_vif(mvm, vif);
> 2679 out_remove:
> 2680 iwl_mvm_mac_ctxt_remove(mvm, vif);
> 2681 out_unlock:
> 2682 mutex_unlock(&mvm->mutex);
> 2683 return ret;
> 2684 }
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] iwlwifi: integrate with iwlmei
2021-11-30 8:09 ` Grumbach, Emmanuel
@ 2021-11-30 9:07 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2021-11-30 9:07 UTC (permalink / raw)
To: Grumbach, Emmanuel; +Cc: linux-wireless
On Tue, Nov 30, 2021 at 08:09:48AM +0000, Grumbach, Emmanuel wrote:
> Hi Dan,
>
> > Hello Emmanuel Grumbach,
> >
> > The patch 6d19a5eba5cd: "iwlwifi: integrate with iwlmei" from Nov 12, 2021,
> > leads to the following Smatch static checker warning:
> >
> > drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c:2640
> > iwl_mvm_start_ap_ibss()
> > error: NULL dereference inside function
> > '__iwl_mvm_mac_set_key()'
>
> Where in __iwl_mvm_mac_set_key() ?
> This function should be able to cope with with a NULL sta I think.
> I don't really see how this could be related to my patch since iwlmei is not related to AP mode at all.
>
> I also moved to that commit, but the line numbers don't match, so I am a bit confused.
>
I'm on yesterday's linux-next. The patch moves the mvmsta assignment to
the start of the function.
mvmsta = iwl_mvm_sta_from_mac80211(sta);
^^^^
It introduces a couple other Smatch warnings as well.
drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c:3643 __iwl_mvm_mac_set_key() warn: variable dereferenced before check 'sta' (see line 3594)
drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c:3773 __iwl_mvm_mac_set_key() warn: variable dereferenced before check 'sta' (see line 3594)
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [bug report] iwlwifi: integrate with iwlmei
2021-11-30 7:32 Dan Carpenter
@ 2021-11-30 8:17 ` Grumbach, Emmanuel
0 siblings, 0 replies; 5+ messages in thread
From: Grumbach, Emmanuel @ 2021-11-30 8:17 UTC (permalink / raw)
To: Dan Carpenter, Stephen Rothwell, Luca Coelho; +Cc: linux-wireless
Hi Dan,
>
> Hello Emmanuel Grumbach,
>
> The patch 6d19a5eba5cd: "iwlwifi: integrate with iwlmei" from Nov 12, 2021,
> leads to the following Smatch static checker warning:
>
> drivers/net/wireless/intel/iwlwifi/mvm/ops.c:740
> iwl_mvm_start_get_nvm()
> warn: inconsistent returns '&mvm->hw->wiphy->mtx'.
>
> drivers/net/wireless/intel/iwlwifi/mvm/ops.c
> 684 static int iwl_mvm_start_get_nvm(struct iwl_mvm *mvm)
> 685 {
> 686 struct iwl_trans *trans = mvm->trans;
> 687 int ret;
> 688
> 689 if (trans->csme_own) {
> 690 if (WARN(!mvm->mei_registered,
> 691 "csme is owner, but we aren't registered to iwlmei\n"))
> 692 goto get_nvm_from_fw;
> 693
> 694 mvm->mei_nvm_data = iwl_mei_get_nvm();
> 695 if (mvm->mei_nvm_data) {
> 696 /*
> 697 * mvm->mei_nvm_data is set and because of that,
> 698 * we'll load the NVM from the FW when we'll get
> 699 * ownership.
> 700 */
> 701 mvm->nvm_data =
> 702 iwl_parse_mei_nvm_data(trans, trans->cfg,
> 703 mvm->mei_nvm_data, mvm->fw);
> 704 return 0;
> 705 }
> 706
> 707 IWL_ERR(mvm,
> 708 "Got a NULL NVM from CSME, trying to get it from the
> device\n");
> 709 }
> 710
> 711 get_nvm_from_fw:
> 712 rtnl_lock();
> 713 wiphy_lock(mvm->hw->wiphy);
> 714 mutex_lock(&mvm->mutex);
>
> This code takes three lines. I'm looking at linux-next next-20211129, so it's a
> little bit different. The original patch is buggy but it's made worse by a merge
> issue.
>
>
> 715
> 716 ret = iwl_trans_start_hw(mvm->trans);
> 717 if (ret) {
> 718 mutex_unlock(&mvm->mutex);
> 719 return ret;
>
> This only drops one lock before returning. It should probably be a goto
> unlock; and we add an unlock at the end of the function. I would send a
> patch for that but it gets a bit confusing because of the merge.
> Emmanuel, could you take a look at this?
Luca has a patch in the pipe to fix this. Clearly, we had merge issues here.
Luca, it looks like the commit:
"iwlwifi: mvm: unlock RTNL when starting HW fails" is the one we need.
What happened here is that, internally, we had two features that were implemented
in a certain order (iwlmei before the load of the regdomain in INIT) and upstream
they were merged the other way around. This caused merge issues that weren't handled
properly apparently.
Luca, can you take a look?
>
> 720 }
> 721
> 722 ret = iwl_run_init_mvm_ucode(mvm);
> 723 if (ret && ret != -ERFKILL)
> 724 iwl_fw_dbg_error_collect(&mvm->fwrt,
> FW_DBG_TRIGGER_DRIVER);
> 725 if (!ret && iwl_mvm_is_lar_supported(mvm)) {
> 726 mvm->hw->wiphy->regulatory_flags |=
> REGULATORY_WIPHY_SELF_MANAGED;
> 727 ret = iwl_mvm_init_mcc(mvm);
> 728 }
> 729
> 730 if (!iwlmvm_mod_params.init_dbg || !ret)
> 731 iwl_mvm_stop_device(mvm);
> 732
> 733 mutex_unlock(&mvm->mutex);
> 734 wiphy_unlock(mvm->hw->wiphy);
> 735 rtnl_unlock();
> 736
> 737 if (ret)
> 738 IWL_ERR(mvm, "Failed to run INIT ucode: %d\n", ret);
> 739
> --> 740 return ret;
> 741 }
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* [bug report] iwlwifi: integrate with iwlmei
@ 2021-11-30 7:32 Dan Carpenter
2021-11-30 8:17 ` Grumbach, Emmanuel
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2021-11-30 7:32 UTC (permalink / raw)
To: emmanuel.grumbach, Stephen Rothwell; +Cc: linux-wireless
Hello Emmanuel Grumbach,
The patch 6d19a5eba5cd: "iwlwifi: integrate with iwlmei" from Nov 12,
2021, leads to the following Smatch static checker warning:
drivers/net/wireless/intel/iwlwifi/mvm/ops.c:740 iwl_mvm_start_get_nvm()
warn: inconsistent returns '&mvm->hw->wiphy->mtx'.
drivers/net/wireless/intel/iwlwifi/mvm/ops.c
684 static int iwl_mvm_start_get_nvm(struct iwl_mvm *mvm)
685 {
686 struct iwl_trans *trans = mvm->trans;
687 int ret;
688
689 if (trans->csme_own) {
690 if (WARN(!mvm->mei_registered,
691 "csme is owner, but we aren't registered to iwlmei\n"))
692 goto get_nvm_from_fw;
693
694 mvm->mei_nvm_data = iwl_mei_get_nvm();
695 if (mvm->mei_nvm_data) {
696 /*
697 * mvm->mei_nvm_data is set and because of that,
698 * we'll load the NVM from the FW when we'll get
699 * ownership.
700 */
701 mvm->nvm_data =
702 iwl_parse_mei_nvm_data(trans, trans->cfg,
703 mvm->mei_nvm_data, mvm->fw);
704 return 0;
705 }
706
707 IWL_ERR(mvm,
708 "Got a NULL NVM from CSME, trying to get it from the device\n");
709 }
710
711 get_nvm_from_fw:
712 rtnl_lock();
713 wiphy_lock(mvm->hw->wiphy);
714 mutex_lock(&mvm->mutex);
This code takes three lines. I'm looking at linux-next next-20211129,
so it's a little bit different. The original patch is buggy but it's
made worse by a merge issue.
715
716 ret = iwl_trans_start_hw(mvm->trans);
717 if (ret) {
718 mutex_unlock(&mvm->mutex);
719 return ret;
This only drops one lock before returning. It should probably be a
goto unlock; and we add an unlock at the end of the function. I would
send a patch for that but it gets a bit confusing because of the merge.
Emmanuel, could you take a look at this?
720 }
721
722 ret = iwl_run_init_mvm_ucode(mvm);
723 if (ret && ret != -ERFKILL)
724 iwl_fw_dbg_error_collect(&mvm->fwrt, FW_DBG_TRIGGER_DRIVER);
725 if (!ret && iwl_mvm_is_lar_supported(mvm)) {
726 mvm->hw->wiphy->regulatory_flags |= REGULATORY_WIPHY_SELF_MANAGED;
727 ret = iwl_mvm_init_mcc(mvm);
728 }
729
730 if (!iwlmvm_mod_params.init_dbg || !ret)
731 iwl_mvm_stop_device(mvm);
732
733 mutex_unlock(&mvm->mutex);
734 wiphy_unlock(mvm->hw->wiphy);
735 rtnl_unlock();
736
737 if (ret)
738 IWL_ERR(mvm, "Failed to run INIT ucode: %d\n", ret);
739
--> 740 return ret;
741 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-11-30 9:08 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-30 6:20 [bug report] iwlwifi: integrate with iwlmei Dan Carpenter
2021-11-30 8:09 ` Grumbach, Emmanuel
2021-11-30 9:07 ` Dan Carpenter
2021-11-30 7:32 Dan Carpenter
2021-11-30 8:17 ` Grumbach, Emmanuel
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.