| Message ID | 1679858.MgakDZE2Vr@debian64 (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Fri, 2014-08-22 at 22:38 +0200, Christian Lamparter wrote: > On Friday, August 22, 2014 10:14:31 PM Andreea-Cristina Bernat wrote: > > The carl9170_op_ampdu_action() function is used only by the mac80211 > > framework. Since the mac80211 already takes care of checks and > > properly serializing calls to the driver's function there is no > > need for the driver to do the same thing. > > > > Signed-off-by: Andreea-Cristina Bernat <bernat.ada@gmail.com> > > --- > > Changes in v2: > > - Change subject line from > > "carl9170: Replace rcu_dereference() with rcu_access_pointer()" > > to > > "carl9170: Remove redundant protection check" > > - Update the commit message according to the modifications > > - Delete the lines of interest at the suggestion and explanations of > > Christian Lamparter <chunkeey@googlemail.com> > > > > drivers/net/wireless/ath/carl9170/main.c | 6 ------ > > 1 file changed, 6 deletions(-) > > > > diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c > > index 12018ff..6758b9a 100644 > > --- a/drivers/net/wireless/ath/carl9170/main.c > > +++ b/drivers/net/wireless/ath/carl9170/main.c > > @@ -1430,12 +1430,6 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw, > > if (!sta_info->ht_sta) > > return -EOPNOTSUPP; > > > > - rcu_read_lock(); > > - if (rcu_access_pointer(sta_info->agg[tid])) { > > - rcu_read_unlock(); > > - return -EBUSY; > > - } > > - > > tid_info = kzalloc(sizeof(struct carl9170_sta_tid), > > GFP_ATOMIC); > > if (!tid_info) { > > > > sparse [0] hit a bug when testing the patch: > > drivers/net/wireless/ath/carl9170/main.c:1440:17: > warning: context imbalance in 'carl9170_op_ampdu_action' - unexpected unlock > > This warning is caused by the remaining, stray rcu_read protection > in the code below (Sorry! Guess RCU needed a bit more explanation > in the previous post. If you are looking for *pointers*, there are > excellent resources available in Documentation/RCU/ [1]). > > I've attached a full patch (see below) with all changes so far and > tested if the device/driver still behaves ;-). This patch applies > cleanly on top of wireless-testing. > > @John, > can you please take it? > > --- > From: Andreea-Cristina Bernat <bernat.ada@gmail.com> > > The carl9170_op_ampdu_action() function is used only by the mac80211 > framework. Since the mac80211 already takes care of checks and > properly serializing calls to the driver's function there is no > need for the driver to do the same thing. > > Signed-off-by: Andreea-Cristina Bernat <bernat.ada@gmail.com> > [chunkeey@googlemail.com: remove two stray rcu_read_unlock()] > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com> > --- > diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c > index f8ded84..ef5b6dc 100644 > --- a/drivers/net/wireless/ath/carl9170/main.c > +++ b/drivers/net/wireless/ath/carl9170/main.c > @@ -1430,18 +1430,10 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw, > if (!sta_info->ht_sta) > return -EOPNOTSUPP; > > - rcu_read_lock(); > - if (rcu_dereference(sta_info->agg[tid])) { > - rcu_read_unlock(); > - return -EBUSY; > - } > - > tid_info = kzalloc(sizeof(struct carl9170_sta_tid), > GFP_ATOMIC); > - if (!tid_info) { > - rcu_read_unlock(); > + if (!tid_info) > return -ENOMEM; > - } > > tid_info->hsn = tid_info->bsn = tid_info->snx = (*ssn); > tid_info->state = CARL9170_TID_STATE_PROGRESS; > @@ -1460,7 +1452,6 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw, > list_add_tail_rcu(&tid_info->list, &ar->tx_ampdu_list); > rcu_assign_pointer(sta_info->agg[tid], tid_info); > spin_unlock_bh(&ar->tx_ampdu_list_lock); > - rcu_read_unlock(); > > ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid); > break; > --- > > Regards > Christian > > [0] <http://kernelnewbies.org/Sparse> > [1] <https://www.kernel.org/doc/Documentation/RCU/whatisRCU.txt> > Sorry but this patch is not complete. You need to somehow return -EBUSY if sta_info->agg[tid] is set. The test has to be done inside the spin_lock_bh(&ar->tx_ampdu_list_lock) / spin_unlock_bh(&ar->tx_ampdu_list_lock); region. You are correct the RCU bits were wrong in this context, but we still have to avoid leaks. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, August 22, 2014 02:08:52 PM Eric Dumazet wrote: > On Fri, 2014-08-22 at 22:38 +0200, Christian Lamparter wrote: > [...] > > From: Andreea-Cristina Bernat <bernat.ada@gmail.com> > > > > The carl9170_op_ampdu_action() function is used only by the mac80211 > > framework. Since the mac80211 already takes care of checks and > > properly serializing calls to the driver's function there is no > > need for the driver to do the same thing. > > > > --- > > @@ -1430,18 +1430,10 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw, > > if (!sta_info->ht_sta) > > return -EOPNOTSUPP; > > > > - rcu_read_lock(); > > - if (rcu_dereference(sta_info->agg[tid])) { > > - rcu_read_unlock(); > > - return -EBUSY; > > - } > > - > > [...] > > --- > > Sorry but this patch is not complete. > > You need to somehow return -EBUSY if sta_info->agg[tid] is set. The sta_info->agg[tid] check is not needed (for reference, see [0]). (There is already a check in mac80211 which prevents the leak of sta_info->agg[tid] [1]). Regards Christian [0] <https://lkml.org/lkml/2014/8/20/725> [1] <http://lxr.free-electrons.com/source/net/mac80211/agg-tx.c#L583> -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2014-08-22 at 23:53 +0200, Christian Lamparter wrote: > The sta_info->agg[tid] check is not needed (for reference, see [0]). > (There is already a check in mac80211 which prevents the leak of > sta_info->agg[tid] [1]). > > Regards > Christian > > [0] <https://lkml.org/lkml/2014/8/20/725> > [1] <http://lxr.free-electrons.com/source/net/mac80211/agg-tx.c#L583> > Hmpfff... this code is quite confusing. RCU is used both in tricky way (carl9170_ampdu_gc() is an example) and a talisman (the part you remove) Why is rcu_assign_pointer(sta_info->agg[tid], tid_info); done inside the spinlock protected region, I don't know. If this code relies on external protection, a comment would help its comprehension for sure. For example, you could add a BUG_ON(rcu_access_pointer(sta_info->agg[tid])); so that we are sure requirements are not changed in the callers one day. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, August 22, 2014 04:23:19 PM Eric Dumazet wrote: > On Fri, 2014-08-22 at 23:53 +0200, Christian Lamparter wrote: > > > The sta_info->agg[tid] check is not needed (for reference, see [0]). > > (There is already a check in mac80211 which prevents the leak of > > sta_info->agg[tid] [1]). > > > > Regards > > Christian > > > > [0] <https://lkml.org/lkml/2014/8/20/725> > > [1] <http://lxr.free-electrons.com/source/net/mac80211/agg-tx.c#L583> > > > > Hmpfff... this code is quite confusing. That's true. Furthermore, parts of the logic are also embedded in the mac80211-stack and above. So, it's very hard to see the whole big picture, just by looking at the driver code. > RCU is used both in tricky way (carl9170_ampdu_gc() is an example) > and a talisman (the part you remove) I know that game ;-). But fair enough: if you have concerns about the complexity of the code in question: I'm willing to help you and explain the quirks in detail if necessary. I think this is a valuable addition, since "external consultants" are hard to come by. > Why is rcu_assign_pointer(sta_info->agg[tid], tid_info); > done inside the spinlock protected region, I don't know. The pointer in sta_info->agg[tid] is used exclusively by the tx.c code... It is queried only if an outgoing frame has the IEEE80211_TX_CTL_AMPDU flag set. But for this flag to be set, the aggregation session has to be operational. This requires two calls to ampdu_action [0]. (first with: IEEE80211_AMPDU_TX_START and later with: IEEE80211_AMPDU_TX_OPERATIONAL). => If you want to make a patch to move this rcu_assign_pointer(...) after the spin_unlock_bh() - Then: Yes, GO FOR IT! > If this code relies on external protection, a comment would help its > comprehension for sure. > > For example, you could add a > BUG_ON(rcu_access_pointer(sta_info->agg[tid])); > so that we are sure requirements are not changed > in the callers one day. Maybe, but then: Is a "specific driver" the right place for this? Other drivers may also depend on ampdu_action not changing. As for the logic: The AMPDU handshake itself is part of the 802.11 spec. If you are interested you can get 802.11-2012 [1] and look into Section 9.21 "Block Acknowledgment". It contains a message sequence chart and details about the setup and tear down procedures for aggregation session [which is at the heart of the ampdu_action callback issue]. Note: mac80211 has a "software simulator" mac80211_hwsim [2]. It can be (and is) used to test most of the mac80211 functionality. So what do you think? Regards Christian [0] <https://www.kernel.org/doc/htmldocs/80211/aggregation.html> [1] <http://standards.ieee.org/findstds/standard/802.11-2012.html> [2] <http://wireless.kernel.org/en/users/Drivers/mac80211_hwsim> -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c index f8ded84..ef5b6dc 100644 --- a/drivers/net/wireless/ath/carl9170/main.c +++ b/drivers/net/wireless/ath/carl9170/main.c @@ -1430,18 +1430,10 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw, if (!sta_info->ht_sta) return -EOPNOTSUPP; - rcu_read_lock(); - if (rcu_dereference(sta_info->agg[tid])) { - rcu_read_unlock(); - return -EBUSY; - } - tid_info = kzalloc(sizeof(struct carl9170_sta_tid), GFP_ATOMIC); - if (!tid_info) { - rcu_read_unlock(); + if (!tid_info) return -ENOMEM; - } tid_info->hsn = tid_info->bsn = tid_info->snx = (*ssn); tid_info->state = CARL9170_TID_STATE_PROGRESS; @@ -1460,7 +1452,6 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw, list_add_tail_rcu(&tid_info->list, &ar->tx_ampdu_list); rcu_assign_pointer(sta_info->agg[tid], tid_info); spin_unlock_bh(&ar->tx_ampdu_list_lock); - rcu_read_unlock(); ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid); break;