[PATCH 1/2] mac80211: allocate and fill tidstats only when needed

[PATCH 1/2] mac80211: allocate and fill tidstats only when needed

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

This fixes memory leaks in the case where we just have the
station info on the stack for internal usage without sending
it to cfg80211.

Fixes: 8689c051a201 ("cfg80211: dynamically allocate per-tid stats for station info")
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/cfg.c      | 4 ++--
 net/mac80211/ethtool.c  | 4 ++--
 net/mac80211/sta_info.c | 7 ++++---
 net/mac80211/sta_info.h | 3 ++-
 4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 5ce9d121af2b..bdf6fa78d0d2 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -695,7 +695,7 @@ static int ieee80211_dump_station(struct wiphy *wiphy, struct net_device *dev,
 	if (sta) {
 		ret = 0;
 		memcpy(mac, sta->sta.addr, ETH_ALEN);
-		sta_set_sinfo(sta, sinfo);
+		sta_set_sinfo(sta, sinfo, true);
 	}
 
 	mutex_unlock(&local->sta_mtx);
@@ -724,7 +724,7 @@ static int ieee80211_get_station(struct wiphy *wiphy, struct net_device *dev,
 	sta = sta_info_get_bss(sdata, mac);
 	if (sta) {
 		ret = 0;
-		sta_set_sinfo(sta, sinfo);
+		sta_set_sinfo(sta, sinfo, true);
 	}
 
 	mutex_unlock(&local->sta_mtx);
diff --git a/net/mac80211/ethtool.c b/net/mac80211/ethtool.c
index 2ba5686cbcab..690c142a7a44 100644
--- a/net/mac80211/ethtool.c
+++ b/net/mac80211/ethtool.c
@@ -108,7 +108,7 @@ static void ieee80211_get_stats(struct net_device *dev,
 			goto do_survey;
 
 		memset(&sinfo, 0, sizeof(sinfo));
-		sta_set_sinfo(sta, &sinfo);
+		sta_set_sinfo(sta, &sinfo, false);
 
 		i = 0;
 		ADD_STA_STATS(sta);
@@ -135,7 +135,7 @@ static void ieee80211_get_stats(struct net_device *dev,
 				continue;
 
 			memset(&sinfo, 0, sizeof(sinfo));
-			sta_set_sinfo(sta, &sinfo);
+			sta_set_sinfo(sta, &sinfo, false);
 			i = 0;
 			ADD_STA_STATS(sta);
 		}
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 04d47689b557..6428f1ac37b6 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -1008,7 +1008,7 @@ static void __sta_info_destroy_part2(struct sta_info *sta)
 
 	sinfo = kzalloc(sizeof(*sinfo), GFP_KERNEL);
 	if (sinfo)
-		sta_set_sinfo(sta, sinfo);
+		sta_set_sinfo(sta, sinfo, true);
 	cfg80211_del_sta_sinfo(sdata->dev, sta->sta.addr, sinfo, GFP_KERNEL);
 	kfree(sinfo);
 
@@ -2079,7 +2079,8 @@ static inline u64 sta_get_stats_bytes(struct ieee80211_sta_rx_stats *rxstats)
 	return value;
 }
 
-void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo)
+void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo,
+		   bool tidstats)
 {
 	struct ieee80211_sub_if_data *sdata = sta->sdata;
 	struct ieee80211_local *local = sdata->local;
@@ -2233,7 +2234,7 @@ void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo)
 			sinfo->filled |= BIT(NL80211_STA_INFO_RX_BITRATE);
 	}
 
-	if (!cfg80211_sinfo_alloc_tid_stats(sinfo, GFP_KERNEL)) {
+	if (tidstats && !cfg80211_sinfo_alloc_tid_stats(sinfo, GFP_KERNEL)) {
 		for (i = 0; i < IEEE80211_NUM_TIDS + 1; i++) {
 			struct cfg80211_tid_stats *tidstats = &sinfo->pertid[i];
 
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index d79bd6eeb549..81b35f623792 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -744,7 +744,8 @@ static inline int sta_info_flush(struct ieee80211_sub_if_data *sdata)
 void sta_set_rate_info_tx(struct sta_info *sta,
 			  const struct ieee80211_tx_rate *rate,
 			  struct rate_info *rinfo);
-void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo);
+void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo,
+		   bool tidstats);
 
 u32 sta_get_expected_throughput(struct sta_info *sta);
 
-- 
2.14.3

[PATCH 2/2] cfg80211: release station info tidstats where needed

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

This fixes memory leaks in cases where we got the station
info but failed sending it out properly.

Fixes: 8689c051a201 ("cfg80211: dynamically allocate per-tid stats for station info")
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/cfg80211.h | 13 +++++++++++++
 net/wireless/nl80211.c | 11 ++++++++---
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 8984d24d68b7..1c6364591856 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -5710,6 +5710,19 @@ void cfg80211_remain_on_channel_expired(struct wireless_dev *wdev, u64 cookie,
  */
 int cfg80211_sinfo_alloc_tid_stats(struct station_info *sinfo, gfp_t gfp);
 
+/**
+ * cfg80211_sinfo_release_sinfo - release contents of station info
+ * @sinfo: the station information
+ *
+ * Releases any potentially allocated sub-information of the station
+ * information, but not the struct itself (since it's typically on
+ * the stack.)
+ */
+static inline void cfg80211_sinfo_release_sinfo(struct station_info *sinfo)
+{
+	kfree(sinfo->pertid);
+}
+
 /**
  * cfg80211_new_sta - notify userspace about station
  *
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 7daceb1f253d..0996fdc002e0 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -4702,7 +4702,6 @@ static int nl80211_send_station(struct sk_buff *msg, u32 cmd, u32 portid,
 		}
 
 		nla_nest_end(msg, tidsattr);
-		kfree(sinfo->pertid);
 	}
 
 	nla_nest_end(msg, sinfoattr);
@@ -4712,10 +4711,12 @@ static int nl80211_send_station(struct sk_buff *msg, u32 cmd, u32 portid,
 		    sinfo->assoc_req_ies))
 		goto nla_put_failure;
 
+	cfg80211_sinfo_release_sinfo(sinfo);
 	genlmsg_end(msg, hdr);
 	return 0;
 
  nla_put_failure:
+	cfg80211_sinfo_release_sinfo(sinfo);
 	genlmsg_cancel(msg, hdr);
 	return -EMSGSIZE;
 }
@@ -4797,8 +4798,10 @@ static int nl80211_get_station(struct sk_buff *skb, struct genl_info *info)
 		return err;
 
 	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
-	if (!msg)
+	if (!msg) {
+		cfg80211_sinfo_release_sinfo(sinfo);
 		return -ENOMEM;
+	}
 
 	if (nl80211_send_station(msg, NL80211_CMD_NEW_STATION,
 				 info->snd_portid, info->snd_seq, 0,
@@ -14624,8 +14627,10 @@ void cfg80211_del_sta_sinfo(struct net_device *dev, const u8 *mac_addr,
 	trace_cfg80211_del_sta(dev, mac_addr);
 
 	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, gfp);
-	if (!msg)
+	if (!msg) {
+		cfg80211_sinfo_release_sinfo(sinfo);
 		return;
+	}
 
 	if (nl80211_send_station(msg, NL80211_CMD_DEL_STATION, 0, 0, 0,
 				 rdev, dev, mac_addr, sinfo) < 0) {
-- 
2.14.3

Re: [PATCH 2/2] cfg80211: release station info tidstats where needed

[Arend van Spriel]

On 5/18/2018 11:47 AM, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> This fixes memory leaks in cases where we got the station
> info but failed sending it out properly.

Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Fixes: 8689c051a201 ("cfg80211: dynamically allocate per-tid stats for station info")
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>   include/net/cfg80211.h | 13 +++++++++++++
>   net/wireless/nl80211.c | 11 ++++++++---
>   2 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index 8984d24d68b7..1c6364591856 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -5710,6 +5710,19 @@ void cfg80211_remain_on_channel_expired(struct wireless_dev *wdev, u64 cookie,
>    */
>   int cfg80211_sinfo_alloc_tid_stats(struct station_info *sinfo, gfp_t gfp);
>
> +/**
> + * cfg80211_sinfo_release_sinfo - release contents of station info

Maybe drop one '_sinfo' from the function name? Or 
cfg80211_sinfo_release_contents?

Regards,
Arend

Re: [PATCH 2/2] cfg80211: release station info tidstats where needed

[Johannes Berg]

On Fri, 2018-05-18 at 12:25 +0200, Arend van Spriel wrote:
> On 5/18/2018 11:47 AM, Johannes Berg wrote:
> > From: Johannes Berg <johannes.berg@intel.com>
> > 
> > This fixes memory leaks in cases where we got the station
> > info but failed sending it out properly.
> 
> Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> > Fixes: 8689c051a201 ("cfg80211: dynamically allocate per-tid stats for station info")
> > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> > ---
> >   include/net/cfg80211.h | 13 +++++++++++++
> >   net/wireless/nl80211.c | 11 ++++++++---
> >   2 files changed, 21 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> > index 8984d24d68b7..1c6364591856 100644
> > --- a/include/net/cfg80211.h
> > +++ b/include/net/cfg80211.h
> > @@ -5710,6 +5710,19 @@ void cfg80211_remain_on_channel_expired(struct wireless_dev *wdev, u64 cookie,
> >    */
> >   int cfg80211_sinfo_alloc_tid_stats(struct station_info *sinfo, gfp_t gfp);
> > 
> > +/**
> > + * cfg80211_sinfo_release_sinfo - release contents of station info
> 
> Maybe drop one '_sinfo' from the function name? Or 
> cfg80211_sinfo_release_contents?

Heh. I guess I wasn't paying attention, I like _content()

johannes