Re: [PATCHv3 RESEND 07/11] cfg80211: add utility functions to clone and free nan_func

[Johannes Berg]

> +int cfg80211_clone_nan_func_members(struct cfg80211_nan_func *f1,
> + const struct cfg80211_nan_func *f2)
> +{
> + memcpy(f1, f2, sizeof(*f1));

That's pretty weird. And the only user of this (in a later patch) is
first allocating the f1. I think this function should be changed to
also allocate the entire struct cfg80211_nan_func, with all the
contents.

Yes, mac80211 is actually allocating a bit more - but only a list_head.
I think I'd prefer just putting a "struct list_head list" into the
cfg80211 struct "for driver use" and getting rid of all of these
contortions.


With some care, I'm pretty sure you could even get away with a single
allocation that's big enough to cover everything, so that you don't
need to have cfg80211_free_nan_func_members() exported but can simply
kfree() the pointer returned from this function.

That's basically doing 

size = sizeof(*dst);
size += src->serv_spec_info_len;
size += src->srf_bf_len;
size += src->srf_num_macs * size(...);
size += 
size += 

and then using pointers to the single block.

The only problem might be if that can get really large, but I think it
would make memory management simpler.

In fact, it'd be *really* nice if that could also be done when parsing
this from nl80211.


Additionally, and alternatively to exporting this function from
cfg80211, why don't you change the rules around the memory?
If nl80211_nan_add_func() doesn't put the func on the stack, but
allocates it, then rdev_add_nan_func() can be forced to take ownership
of the pointer. That way, there's no need to even copy the data at all.
Yes, that'd have to be documented (particularly whether or not it also
takes ownership in error cases, it probably should), but overall I'm
pretty sure it'd simplify things.

Even if we *don't* get away with putting everything into a single
allocation in nl80211 - that might be too complicated - we'd only have
to export the free function and would never copy around things.

johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv3 RESEND 07/11] cfg80211: add utility functions to clone and free nan_func

[Emmanuel Grumbach]

From: Andrei Otcheretianski 

Add cfg80211_free_nan_func and cfg80211_clone_nan_func. These helper
functions will be used by mac80211 in the next patch.

Signed-off-by: Andrei Otcheretianski 
Signed-off-by: Emmanuel Grumbach 
---
 include/net/cfg80211.h |  21 ++
 net/wireless/util.c| 112 +
 2 files changed, 133 insertions(+)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 3a94b21..5e972f4 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -5544,6 +5544,27 @@ void cfg80211_nan_func_terminated(struct wireless_dev 
*wdev,
  enum nl80211_nan_func_term_reason reason,
  u64 cookie, gfp_t gfp);
 
+/**
+ * cfg80211_free_nan_func_members - free nan function members
+ * @f: NAN function that should be freed
+ *
+ * Frees all the allocated members of the given function, however
+ * it doesn't frees the pointed memory. This function can be only called if @f
+ * was cloned using cfg80211_clone_nan_func_members()
+ */
+void cfg80211_free_nan_func_members(struct cfg80211_nan_func *f);
+
+/**
+ * cfg80211_clone_nan_func_members - clone nan function
+ * @f1: destination
+ * @f2: source
+ *
+ * Clones @f2 to @f1. The function doesn't allocate @f1. Returns 0 on success.
+ * To free @f1's members cfg80211_free_nan_func_members() should be used.
+ */
+int cfg80211_clone_nan_func_members(struct cfg80211_nan_func *f1,
+   const struct cfg80211_nan_func *f2);
+
 /* ethtool helper */
 void cfg80211_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo 
*info);
 
diff --git a/net/wireless/util.c b/net/wireless/util.c
index bdf0c40..09f07c7 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -1757,6 +1757,118 @@ int cfg80211_get_station(struct net_device *dev, const 
u8 *mac_addr,
 }
 EXPORT_SYMBOL(cfg80211_get_station);
 
+void cfg80211_free_nan_func_members(struct cfg80211_nan_func *f)
+{
+   int i;
+
+   kfree(f->serv_spec_info);
+   kfree(f->srf_bf);
+   kfree(f->srf_macs);
+   for (i = 0; i < f->num_rx_filters; i++)
+   kfree(f->rx_filters[i].filter);
+
+   for (i = 0; i < f->num_tx_filters; i++)
+   kfree(f->tx_filters[i].filter);
+
+   kfree(f->rx_filters);
+   kfree(f->tx_filters);
+}
+EXPORT_SYMBOL(cfg80211_free_nan_func_members);
+
+static int
+ieee80211_clone_match_filters(struct cfg80211_nan_func_filter *filt1,
+ const struct cfg80211_nan_func_filter *filt2,
+ int num_filters)
+{
+   int i;
+
+   for (i = 0; i < num_filters; i++) {
+   filt1[i].filter = kmemdup(filt2[i].filter, filt2[i].len,
+ GFP_KERNEL);
+   filt1[i].len = filt2[i].len;
+
+   if (!filt1[i].filter)
+   return -ENOMEM;
+   }
+
+   return 0;
+}
+
+int cfg80211_clone_nan_func_members(struct cfg80211_nan_func *f1,
+   const struct cfg80211_nan_func *f2)
+{
+   memcpy(f1, f2, sizeof(*f1));
+
+   /* set all pointers to NULL so we can free them in case of failure */
+   f1->serv_spec_info = NULL;
+   f1->srf_bf = NULL;
+   f1->srf_macs = NULL;
+   f1->rx_filters = NULL;
+   f1->tx_filters = NULL;
+   f1->num_rx_filters = 0;
+   f1->num_tx_filters = 0;
+
+   if (f2->serv_spec_info_len) {
+   f1->serv_spec_info = kmemdup(f2->serv_spec_info,
+f2->serv_spec_info_len,
+GFP_KERNEL);
+   if (!f1->serv_spec_info)
+   goto err;
+   }
+
+   if (f2->srf_bf_len) {
+   f1->srf_bf = kmemdup(f2->srf_bf, f2->srf_bf_len, GFP_KERNEL);
+   if (!f1->srf_bf)
+   goto err;
+   }
+
+   if (f2->srf_num_macs) {
+   f1->srf_macs = kmemdup(f2->srf_macs,
+  f2->srf_num_macs *
+  sizeof(*f2->srf_macs),
+  GFP_KERNEL);
+   if (!f1->srf_macs)
+   goto err;
+   }
+
+   if (f2->num_rx_filters) {
+   f1->rx_filters = kcalloc(f2->num_rx_filters,
+sizeof(*f1->rx_filters),
+GFP_KERNEL);
+   if (!f1->rx_filters)
+   goto err;
+
+   if (!ieee80211_clone_match_filters(f1->rx_filters,
+  f2->rx_filters,
+  f2->num_rx_filters))
+   goto err;
+
+   f1->num_rx_filters = f2->num_rx_filters;
+   }
+
+