From patchwork Fri May 3 16:11:46 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christian Lamparter X-Patchwork-Id: 2518431 Return-Path: X-Original-To: patchwork-linux-wireless@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id 48501DF2E5 for ; Fri, 3 May 2013 16:11:58 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933312Ab3ECQL4 (ORCPT ); Fri, 3 May 2013 12:11:56 -0400 Received: from mail-bk0-f43.google.com ([209.85.214.43]:52838 "EHLO mail-bk0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932913Ab3ECQLz (ORCPT ); Fri, 3 May 2013 12:11:55 -0400 Received: by mail-bk0-f43.google.com with SMTP id jm19so799022bkc.30 for ; Fri, 03 May 2013 09:11:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=googlemail.com; s=20120113; h=x-received:from:to:subject:date:user-agent:cc:references :in-reply-to:mime-version:content-type:content-transfer-encoding :message-id; bh=rRtreNE+Z+hN4ebGD7B16mqamVeJYEoqo5Ly8piJQbo=; b=bj5TDxwV+MZsvNO0kF44qLKvVH0/EyEDOvGrJrj7v/eKX6sKVcRUilkHH4b7+tjkbe wA9OOSNtHo0pxEpt4faMXqZvQ9GXYwR+xS+R0pgqc1fEf91PJeSf18RMbFFf9kBEUHEx Syv6yYuvKMGmJiDb0D+df2e6cvv/bm4yQrFI5THjlWdDYwF+Wl45SkwCrJ6WLZ8+Y+RB njaNiCsrnP41IBoZsNBON6mc3i+m7hkwgGQJ3Ge5Ea22VZ4w2LwFJk9ztz79MBAhIA5k P9EJR9PYGpX9DxlcyeeEP6JEwUsg7fPvfpYMF7v+BDTFvVH+f+azK17ljwQcm6A0evsV GXLw== X-Received: by 10.204.232.8 with SMTP id js8mr3849377bkb.109.1367597514418; Fri, 03 May 2013 09:11:54 -0700 (PDT) Received: from blech.mobile (f053211191.adsl.alicedsl.de. [78.53.211.191]) by mx.google.com with ESMTPSA id tc9sm2562744bkb.18.2013.05.03.09.11.52 for (version=TLSv1.2 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Fri, 03 May 2013 09:11:53 -0700 (PDT) Received: from localhost (localhost [127.0.0.1]) by blech.mobile (Postfix) with ESMTP id 64B3010032D; Fri, 3 May 2013 18:11:54 +0200 (CEST) Received: from blech.mobile ([127.0.0.1]) by localhost (localhost [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id At01RK5FtHeC; Fri, 3 May 2013 18:11:47 +0200 (CEST) Received: from blech.localnet (localhost [127.0.0.1]) by blech.mobile (Postfix) with ESMTP id 16BD610027A; Fri, 3 May 2013 18:11:47 +0200 (CEST) From: Christian Lamparter To: Felix Fietkau Subject: Re: [PATCH 3.10] mac80211: fix spurious RCU warning and update documentation Date: Fri, 3 May 2013 18:11:46 +0200 User-Agent: KMail/1.13.7 (Linux/3.9.0-wl+; KDE/4.8.4; x86_64; ; ) Cc: linux-wireless@vger.kernel.org References: <1367568063-24859-1-git-send-email-nbd@openwrt.org> In-Reply-To: <1367568063-24859-1-git-send-email-nbd@openwrt.org> MIME-Version: 1.0 Message-Id: <201305031811.46945.chunkeey@googlemail.com> Sender: linux-wireless-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org On Friday, May 03, 2013 10:01:03 AM Felix Fietkau wrote: > Document rx vs tx status concurrency requirements. > > Signed-off-by: Felix Fietkau Thanks. As you know, I was playing with this before. I wrote a patch, but I didn't have the change to test it yet properly. If you find anything useful in this (now no longer needed) take it, if not ignore it :-D. (i.e: the rcu_dereference with sta->hnext == NULL instead of true). Regards, Christian --- Subject: [RFT] mac80211: more rate control api work In a discussion of my previous patch [1]: "mac80211: fix spurious use of rcu_dereference" Johannes discovered that the band-aid fix proposed in the mail [adding rcu_read_(un)lock] was inadequate: >+ rcu_read_lock(); > old = rcu_dereference(sta->rates); > rcu_assign_pointer(sta->rates, new); > if (old) > kfree_rcu(old); >+ rcu_read_unlock(); "The problem here is that even the rcu_read_lock() around here that's actually there in most cases *isn't* what's protecting this code. What's protecting this assignment is the fact that we require drivers to not call ieee80211_tx_status() concurrently. If this wasn't the case, then calling the function could cause double-free or so by having two CPUs read the old pointer and both call kfree_rcu() on it." [2] This patch moves the rcu-protected rates pointer into mac80211's private station structure. This prevents drivers from modifying the pointer (in an unsafe way). If a driver wants to access the rates table, it should use the rate control api function: ieee80211_get_tx_rates Furthermore, it also adds a lock in rate_control_set_rates to avoid double freeing old rates if the function is called concurrently. At last, the patch fixes suspicious RCU usage in rate_control_set_rates during initialization by adding an exception for stations which are not yet added to the station hash table. [1] Message-Id: <201304230258.08359.chunkeey@googlemail.com> [2] Message-Id: <1366802638.21854.11.camel@jlt4.sipsolutions.net> --- include/net/mac80211.h | 2 -- net/mac80211/rate.c | 20 ++++++++++++++++---- net/mac80211/sta_info.h | 2 ++ net/mac80211/tx.c | 2 +- 4 files changed, 19 insertions(+), 7 deletions(-) diff --git a/include/net/mac80211.h b/include/net/mac80211.h index 04c2d46..4457ea2 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -1276,7 +1276,6 @@ struct ieee80211_sta_rates { * notifications and capabilities. The value is only valid after * the station moves to associated state. * @smps_mode: current SMPS mode (off, static or dynamic) - * @tx_rates: rate control selection table */ struct ieee80211_sta { u32 supp_rates[IEEE80211_NUM_BANDS]; @@ -1290,7 +1289,6 @@ struct ieee80211_sta { u8 rx_nss; enum ieee80211_sta_rx_bandwidth bandwidth; enum ieee80211_smps_mode smps_mode; - struct ieee80211_sta_rates __rcu *rates; /* must be last */ u8 drv_priv[0] __aligned(sizeof(void *)); diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c index 0d51877..a049ff7 100644 --- a/net/mac80211/rate.c +++ b/net/mac80211/rate.c @@ -530,7 +530,7 @@ static void rate_fixup_ratelist(struct ieee80211_vif *vif, } -static void rate_control_fill_sta_table(struct ieee80211_sta *sta, +static void rate_control_fill_sta_table(struct ieee80211_sta *pubsta, struct ieee80211_tx_info *info, struct ieee80211_tx_rate *rates, int max_rates) @@ -538,8 +538,11 @@ static void rate_control_fill_sta_table(struct ieee80211_sta *sta, struct ieee80211_sta_rates *ratetbl = NULL; int i; - if (sta && !info->control.skip_table) + if (pubsta && !info->control.skip_table) { + struct sta_info *sta = container_of(pubsta, struct sta_info, + sta); ratetbl = rcu_dereference(sta->rates); + } /* Fill remaining rate slots with data from the sta rate table. */ max_rates = min_t(int, max_rates, IEEE80211_TX_RATE_TABLE_SIZE); @@ -688,9 +691,18 @@ int rate_control_set_rates(struct ieee80211_hw *hw, struct ieee80211_sta *pubsta, struct ieee80211_sta_rates *rates) { - struct ieee80211_sta_rates *old = rcu_dereference(pubsta->rates); + struct sta_info *sta; + struct ieee80211_sta_rates *old; + + sta = container_of(pubsta, struct sta_info, sta); + + spin_lock_bh(&sta->lock); + old = rcu_dereference_check(sta->rates, + rcu_access_pointer(sta->hnext) != NULL); + + rcu_assign_pointer(sta->rates, rates); + spin_unlock_bh(&sta->lock); - rcu_assign_pointer(pubsta->rates, rates); if (old) kfree_rcu(old, rcu_head); diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h index adc3004..29169f2 100644 --- a/net/mac80211/sta_info.h +++ b/net/mac80211/sta_info.h @@ -234,6 +234,7 @@ struct sta_ampdu_mlme { * @gtk: group keys negotiated with this station, if any * @rate_ctrl: rate control algorithm reference * @rate_ctrl_priv: rate control private per-STA pointer + * @rates: rate control selection table * @last_tx_rate: rate used for last transmit, to report to userspace as * "the" transmit rate * @last_rx_rate_idx: rx status rate index of the last data packet @@ -309,6 +310,7 @@ struct sta_info { struct ieee80211_key __rcu *ptk; struct rate_control_ref *rate_ctrl; void *rate_ctrl_priv; + struct ieee80211_sta_rates __rcu *rates; spinlock_t lock; struct work_struct drv_unblock_wk; diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index 4a5fbf8..fdf5bc4 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -693,7 +693,7 @@ ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx) rate_control_get_rate(tx->sdata, tx->sta, &txrc); if (tx->sta && !info->control.skip_table) - ratetbl = rcu_dereference(tx->sta->sta.rates); + ratetbl = rcu_dereference(tx->sta->rates); if (unlikely(info->control.rates[0].idx < 0)) { if (ratetbl) {