Currently MPP table entries are only removed from memory when their
802.11s mesh interface goes down. This can make the kernel to remember
a growing list of proxied mac addresses which are not relevant or even
reachable anymore.
This patchset adds two additional cleanup paths for MPP table entries
to remove them when their corresponding mesh node is removed or when
the MPP table entry is not used for MESH_PATH_EXPIRE time.
Henning Rogge (2):
mac80211: Remove connected MPP table entries with MPath
mac80211: let unused MPP table entries timeout
net/mac80211/mesh_pathtbl.c | 84 +++++++++++++++++++++++++++++++++++++++++++--
net/mac80211/rx.c | 1 +
net/mac80211/tx.c | 8 ++++-
3 files changed, 90 insertions(+), 3 deletions(-)
--
2.5.0
On Tue, Jan 26, 2016 at 09:53:33PM +0100, Johannes Berg wrote:
> Oh. Interesting. Yeah, I guess that should be OK then.
>
> It's not *nice*, since that's pretty much unexpected, and you then do
> need the rcu_read_lock() ... hmm.
Yeah, I puzzled over that a bit last week as well -- I rewrote it like
this in the series I haven't posted yet:
void mesh_path_flush_by_nexthop(struct sta_info *sta)
{
[...]
rhashtable_walk_start(&iter);
while ((mpath = rhashtable_walk_next(&iter))) {
if (IS_ERR(mpath) && PTR_ERR(mpath) == -EAGAIN)
continue;
if (IS_ERR(mpath))
break;
if (rcu_access_pointer(mpath->next_hop) == sta)
__mesh_path_del(tbl, mpath);
}
rhashtable_walk_stop(&iter);
[...]
}
...this still relies on the rcu read lock inside _walk_start and _walk_stop,
though.
--
Bob Copeland %% http://bobcopeland.com/
On Tue, Jan 26, 2016 at 7:36 PM, Bob Copeland <[email protected]> wrote:
> On Tue, Jan 26, 2016 at 12:41:25PM +0100, Johannes Berg wrote:
>> > + mpp = node->mpath;
>> > + if (ether_addr_equal(mpp->mpp, proxy)) {
>> > + spin_lock(&tbl->hashwlock[i]);
>> > + __mesh_path_del(tbl, node);
>> > + spin_unlock(&tbl->hashwlock[i]);
>>
>> It also doesn't seem like for_each_mesh_entry() can deal with "node"
>> getting deleted from underneath it? It accesses it through
>> hlist_next_rcu() after the deletion, so you have a use-after-free here
>> afaict.
>
> But __mesh_path_del() doesn't free it immediately: it does:
>
> hlist_del_rcu(&node->list);
> call_rcu(&node->rcu, mesh_path_node_reclaim);
>
> ...so this should be ok if in an rcu read-side critical section, right?
The code is a direct copy what was going on the the cleanup path of
the mpath objects... just modified to run on the mpp objects.
Henning
On Tue, Jan 19, 2016 at 09:04:32AM +0100, Henning Rogge wrote:
> err = -ENXIO;
> -enddel:
> +enddelpath:
Concur about it being better to leave this label alone, also the diff looks
weird because it continues:
> + mesh_paths_generation++;
> + spin_unlock(&tbl->hashwlock[hash_idx]);
> + read_unlock_bh(&pathtbl_resize_lock);
> + return err;
> +}
[...]
> + err = -ENXIO;
> +enddelmpp:
> mesh_paths_generation++;
> spin_unlock(&tbl->hashwlock[hash_idx]);
> read_unlock_bh(&pathtbl_resize_lock);
At first I wondered why the last half of the function was changed, but then
I saw that. Shouldn't the above be "mpp_paths_generation++;"?
In general I'd like to merge these two into one function; the only thing
different is the initial table pointer that gets dereferenced and the
generation counter (and now the labels). So something like this should
be doable:
static int mesh_table_delete(struct ieee80211_sub_if_data *sdata,
struct mesh_table *tbl,
const u8 *addr)
{
/* basically what mesh_path_del is today */
}
int mesh_path_del(...)
{
tbl = resize_dereference_mesh_paths();
ret = mesh_table_delete(sdata, tbl, addr);
mesh_paths_generation++;
return ret;
}
int mpp_path_del(...)
{
tbl = resize_dereference_mpp_paths();
ret = mesh_table_delete(sdata, tbl, addr);
mpp_paths_generation++;
return ret;
}
--
Bob Copeland %% http://bobcopeland.com/
On Tue, Jan 19, 2016 at 09:04:30AM +0100, Henning Rogge wrote:
> Currently MPP table entries are only removed from memory when their
> 802.11s mesh interface goes down. This can make the kernel to remember
> a growing list of proxied mac addresses which are not relevant or even
> reachable anymore.
Just so I know what to do with my series: do you plan to resend these
with comments addressed? Or I could adopt the patches if you want.
--
Bob Copeland %% http://bobcopeland.com/
Make the mesh_path_del() function remove all mpp table entries
that are proxied by the removed mesh path.
Signed-off-by: Henning Rogge <[email protected]>
---
net/mac80211/mesh_pathtbl.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/net/mac80211/mesh_pathtbl.c b/net/mac80211/mesh_pathtbl.c
index dadf8dc..d9424ea 100644
--- a/net/mac80211/mesh_pathtbl.c
+++ b/net/mac80211/mesh_pathtbl.c
@@ -835,6 +835,29 @@ void mesh_path_flush_by_nexthop(struct sta_info *sta)
rcu_read_unlock();
}
+static void mpp_flush_by_proxy(struct ieee80211_sub_if_data *sdata,
+ const u8 *proxy)
+{
+ struct mesh_table *tbl;
+ struct mesh_path *mpp;
+ struct mpath_node *node;
+ int i;
+
+ rcu_read_lock();
+ read_lock_bh(&pathtbl_resize_lock);
+ tbl = resize_dereference_mpp_paths();
+ for_each_mesh_entry(tbl, node, i) {
+ mpp = node->mpath;
+ if (ether_addr_equal(mpp->mpp, proxy)) {
+ spin_lock(&tbl->hashwlock[i]);
+ __mesh_path_del(tbl, node);
+ spin_unlock(&tbl->hashwlock[i]);
+ }
+ }
+ read_unlock_bh(&pathtbl_resize_lock);
+ rcu_read_unlock();
+}
+
static void table_flush_by_iface(struct mesh_table *tbl,
struct ieee80211_sub_if_data *sdata)
{
@@ -892,6 +915,9 @@ int mesh_path_del(struct ieee80211_sub_if_data *sdata, const u8 *addr)
int hash_idx;
int err = 0;
+ /* flush relevant mpp entries first */
+ mpp_flush_by_proxy(sdata, addr);
+
read_lock_bh(&pathtbl_resize_lock);
tbl = resize_dereference_mesh_paths();
hash_idx = mesh_table_hash(addr, sdata, tbl);
--
2.5.0
On Tue, 2016-01-26 at 22:31 +0100, Johannes Berg wrote:
> On Tue, 2016-01-26 at 16:22 -0500, Bob Copeland wrote:
> >
> > void mesh_path_flush_by_nexthop(struct sta_info *sta)
> > {
> > [...]
> > rhashtable_walk_start(&iter);
>
> It seems you need to check the return value here?
>
Actually, maybe not. Not sure I understand the return to beginning
though.
johannes
On Tue, Jan 26, 2016 at 12:41:25PM +0100, Johannes Berg wrote:
> > + mpp = node->mpath;
> > + if (ether_addr_equal(mpp->mpp, proxy)) {
> > + spin_lock(&tbl->hashwlock[i]);
> > + __mesh_path_del(tbl, node);
> > + spin_unlock(&tbl->hashwlock[i]);
>
> It also doesn't seem like for_each_mesh_entry() can deal with "node"
> getting deleted from underneath it? It accesses it through
> hlist_next_rcu() after the deletion, so you have a use-after-free here
> afaict.
But __mesh_path_del() doesn't free it immediately: it does:
hlist_del_rcu(&node->list);
call_rcu(&node->rcu, mesh_path_node_reclaim);
...so this should be ok if in an rcu read-side critical section, right?
--
Bob Copeland %% http://bobcopeland.com/
On Thu, Jan 28, 2016 at 04:07:52PM +0100, Henning Rogge wrote:
> > Just so I know what to do with my series: do you plan to resend these
> > with comments addressed? Or I could adopt the patches if you want.
>
> What would be easier for you?
>
> I can easily resend a "v2" tomorrow, depends on how much work it would
> be for you to integrate them (or adapt them) to your patch set.
It's roughly the same amount of work, so whatever works for you is fine
by me.
--
Bob Copeland %% http://bobcopeland.com/
On Fri, Jan 22, 2016 at 9:30 PM, Bob Copeland <[email protected]> wrote:
> On Tue, Jan 19, 2016 at 09:04:32AM +0100, Henning Rogge wrote:
>> Remember the last time when a mpp table entry is used for
>> rx or tx and remove them after MESH_PATH_EXPIRE time.
>
> FYI I have a patch set I'm testing which rewrites a big chunk of
> the path table stuff.
Does it include some cleanup paths for the MPP table? At the moment
the "missing cleanup" still allows remote users to make the Linux
kernel to allocate as much memory as it wants... with no way to free
it except for shutting down the interface.
If you run a "meshed group of Access Points", any user of the AP could
run an attack on the kernel of all mesh nodes by changing its MAC
address very often.
> As I haven't posted it yet, I guess it doesn't matter if this
> goes in first or not, I can adjust -- but it will conflict as-is.
> I was hoping to post it early next week after a few fixes.
>
> Let me know if you want me to base on top.
I would like them to go in first... my experience of the kernel code
(outside some parts of the wifi stack) is not that good, so I don't
know how long I would need to adapt the patches to your new data
structures.
Most likely it is not that complicated, but I don't know.
> Shortlog looks like:
>
> Bob Copeland (6):
> mac80211: mesh: move path tables into ieee82011_if_mesh
> mac80211: mesh: don't hash subif data in mesh path tables
> mac80211: mesh: factor out common mesh path allocation code
> mac80211: mesh: embed known gates list in struct mesh_path
> mac80211: mesh: convert path table to rhashtable
> mac80211: mesh: get rid of write-only field mean_chain_len
Sounds useful, the 80211s data structures are really a mess.
Henning
On Sat, Jan 23, 2016 at 4:15 PM, Bob Copeland <[email protected]> wrote:
> On Sat, Jan 23, 2016 at 10:39:04AM +0100, Henning Rogge wrote:
>> > FYI I have a patch set I'm testing which rewrites a big chunk of
>> > the path table stuff.
>>
>> Does it include some cleanup paths for the MPP table? At the moment
>> the "missing cleanup" still allows remote users to make the Linux
>> kernel to allocate as much memory as it wants... with no way to free
>> it except for shutting down the interface.
>
> No, it needs these patches too. I did harmonize them a little bit,
> so that e.g. the expiry check will be done during lookup, but I didn't
> add an equivalent to flush-by-proxy.
>
> One issue it does address is that the path table can eventually shrink
> by virtue of the rhashtable, whereas now the bucket size is
> ever-growing.
>
>> > Let me know if you want me to base on top.
>>
>> I would like them to go in first... my experience of the kernel code
>> (outside some parts of the wifi stack) is not that good, so I don't
>> know how long I would need to adapt the patches to your new data
>> structures.
>
> Ok, sounds good, I'll just rebase on top of yours.
Thank you...
if you have problem with the "remove by proxy" patch I could look over
the patches you made... this part was quite trivial to do on the old
code and it should be easy for the new one too (as long as there is a
"remove MPath" and a "remove MPP" function).
Henning
On Thu, Jan 28, 2016 at 2:44 PM, Bob Copeland <[email protected]> wrote:
> On Tue, Jan 19, 2016 at 09:04:30AM +0100, Henning Rogge wrote:
>> Currently MPP table entries are only removed from memory when their
>> 802.11s mesh interface goes down. This can make the kernel to remember
>> a growing list of proxied mac addresses which are not relevant or even
>> reachable anymore.
>
> Just so I know what to do with my series: do you plan to resend these
> with comments addressed? Or I could adopt the patches if you want.
What would be easier for you?
I can easily resend a "v2" tomorrow, depends on how much work it would
be for you to integrate them (or adapt them) to your patch set.
Henning
Remember the last time when a mpp table entry is used for
rx or tx and remove them after MESH_PATH_EXPIRE time.
Signed-off-by: Henning Rogge <[email protected]>
---
net/mac80211/mesh_pathtbl.c | 58 +++++++++++++++++++++++++++++++++++++++++++--
net/mac80211/rx.c | 1 +
net/mac80211/tx.c | 8 ++++++-
3 files changed, 64 insertions(+), 3 deletions(-)
diff --git a/net/mac80211/mesh_pathtbl.c b/net/mac80211/mesh_pathtbl.c
index d9424ea..4b4b0b2 100644
--- a/net/mac80211/mesh_pathtbl.c
+++ b/net/mac80211/mesh_pathtbl.c
@@ -929,12 +929,55 @@ int mesh_path_del(struct ieee80211_sub_if_data *sdata, const u8 *addr)
if (mpath->sdata == sdata &&
ether_addr_equal(addr, mpath->dst)) {
__mesh_path_del(tbl, node);
- goto enddel;
+ goto enddelpath;
}
}
err = -ENXIO;
-enddel:
+enddelpath:
+ mesh_paths_generation++;
+ spin_unlock(&tbl->hashwlock[hash_idx]);
+ read_unlock_bh(&pathtbl_resize_lock);
+ return err;
+}
+
+/**
+ * mpp_path_del - delete a mesh proxy path from the table
+ *
+ * @addr: addr address (ETH_ALEN length)
+ * @sdata: local subif
+ *
+ * Returns: 0 if successful
+ */
+static int mpp_path_del(struct ieee80211_sub_if_data *sdata, const u8 *addr)
+{
+ struct mesh_table *tbl;
+ struct mesh_path *mpath;
+ struct mpath_node *node;
+ struct hlist_head *bucket;
+ int hash_idx;
+ int err = 0;
+
+ /* flush relevant mpp entries first */
+ mpp_flush_by_proxy(sdata, addr);
+
+ read_lock_bh(&pathtbl_resize_lock);
+ tbl = resize_dereference_mpp_paths();
+ hash_idx = mesh_table_hash(addr, sdata, tbl);
+ bucket = &tbl->hash_buckets[hash_idx];
+
+ spin_lock(&tbl->hashwlock[hash_idx]);
+ hlist_for_each_entry(node, bucket, list) {
+ mpath = node->mpath;
+ if (mpath->sdata == sdata &&
+ ether_addr_equal(addr, mpath->dst)) {
+ __mesh_path_del(tbl, node);
+ goto enddelmpp;
+ }
+ }
+
+ err = -ENXIO;
+enddelmpp:
mesh_paths_generation++;
spin_unlock(&tbl->hashwlock[hash_idx]);
read_unlock_bh(&pathtbl_resize_lock);
@@ -1157,6 +1200,17 @@ void mesh_path_expire(struct ieee80211_sub_if_data *sdata)
time_after(jiffies, mpath->exp_time + MESH_PATH_EXPIRE))
mesh_path_del(mpath->sdata, mpath->dst);
}
+
+ tbl = rcu_dereference(mpp_paths);
+ for_each_mesh_entry(tbl, node, i) {
+ if (node->mpath->sdata != sdata)
+ continue;
+ mpath = node->mpath;
+ if ((!(mpath->flags & MESH_PATH_FIXED)) &&
+ time_after(jiffies, mpath->exp_time + MESH_PATH_EXPIRE))
+ mpp_path_del(mpath->sdata, mpath->dst);
+ }
+
rcu_read_unlock();
}
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index bc08185..d028581 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -2290,6 +2290,7 @@ ieee80211_rx_h_mesh_fwding(struct ieee80211_rx_data *rx)
spin_lock_bh(&mppath->state_lock);
if (!ether_addr_equal(mppath->mpp, mpp_addr))
memcpy(mppath->mpp, mpp_addr, ETH_ALEN);
+ mppath->exp_time = jiffies;
spin_unlock_bh(&mppath->state_lock);
}
rcu_read_unlock();
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 3311ce0..7a2848b 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -2099,8 +2099,14 @@ static struct sk_buff *ieee80211_build_hdr(struct ieee80211_sub_if_data *sdata,
mpp_lookup = true;
}
- if (mpp_lookup)
+ if (mpp_lookup) {
mppath = mpp_path_lookup(sdata, skb->data);
+ if (mppath) {
+ spin_lock_bh(&mppath->state_lock);
+ mppath->exp_time = jiffies;
+ spin_unlock_bh(&mppath->state_lock);
+ }
+ }
if (mppath && mpath)
mesh_path_del(mpath->sdata, mpath->dst);
--
2.5.0
On Sat, Jan 23, 2016 at 10:39:04AM +0100, Henning Rogge wrote:
> > FYI I have a patch set I'm testing which rewrites a big chunk of
> > the path table stuff.
>
> Does it include some cleanup paths for the MPP table? At the moment
> the "missing cleanup" still allows remote users to make the Linux
> kernel to allocate as much memory as it wants... with no way to free
> it except for shutting down the interface.
No, it needs these patches too. I did harmonize them a little bit,
so that e.g. the expiry check will be done during lookup, but I didn't
add an equivalent to flush-by-proxy.
One issue it does address is that the path table can eventually shrink
by virtue of the rhashtable, whereas now the bucket size is
ever-growing.
> > Let me know if you want me to base on top.
>
> I would like them to go in first... my experience of the kernel code
> (outside some parts of the wifi stack) is not that good, so I don't
> know how long I would need to adapt the patches to your new data
> structures.
Ok, sounds good, I'll just rebase on top of yours.
--
Bob Copeland %% http://bobcopeland.com/
On Tue, Jan 19, 2016 at 09:04:32AM +0100, Henning Rogge wrote:
> Remember the last time when a mpp table entry is used for
> rx or tx and remove them after MESH_PATH_EXPIRE time.
FYI I have a patch set I'm testing which rewrites a big chunk of
the path table stuff.
As I haven't posted it yet, I guess it doesn't matter if this
goes in first or not, I can adjust -- but it will conflict as-is.
I was hoping to post it early next week after a few fixes.
Let me know if you want me to base on top.
Shortlog looks like:
Bob Copeland (6):
mac80211: mesh: move path tables into ieee82011_if_mesh
mac80211: mesh: don't hash subif data in mesh path tables
mac80211: mesh: factor out common mesh path allocation code
mac80211: mesh: embed known gates list in struct mesh_path
mac80211: mesh: convert path table to rhashtable
mac80211: mesh: get rid of write-only field mean_chain_len
--
Bob Copeland %% http://bobcopeland.com/
On Tue, 2016-01-26 at 13:36 -0500, Bob Copeland wrote:
> On Tue, Jan 26, 2016 at 12:41:25PM +0100, Johannes Berg wrote:
> > > + mpp = node->mpath;
> > > + if (ether_addr_equal(mpp->mpp, proxy)) {
> > > + spin_lock(&tbl->hashwlock[i]);
> > > + __mesh_path_del(tbl, node);
> > > + spin_unlock(&tbl->hashwlock[i]);
> >
> > It also doesn't seem like for_each_mesh_entry() can deal with
> > "node"
> > getting deleted from underneath it? It accesses it through
> > hlist_next_rcu() after the deletion, so you have a use-after-free
> > here
> > afaict.
>
> But __mesh_path_del() doesn't free it immediately: it does:
>
> hlist_del_rcu(&node->list);
> call_rcu(&node->rcu, mesh_path_node_reclaim);
>
> ...so this should be ok if in an rcu read-side critical section,
> right?
Oh. Interesting. Yeah, I guess that should be OK then.
It's not *nice*, since that's pretty much unexpected, and you then do
need the rcu_read_lock() ... hmm.
johannes
On Tue, 2016-01-19 at 09:04 +0100, Henning Rogge wrote:
> +++ b/net/mac80211/mesh_pathtbl.c
> @@ -929,12 +929,55 @@ int mesh_path_del(struct ieee80211_sub_if_data
> *sdata, const u8 *addr)
> if (mpath->sdata == sdata &&
> ether_addr_equal(addr, mpath->dst)) {
> __mesh_path_del(tbl, node);
> - goto enddel;
> + goto enddelpath;
C labels are scoped, why rename it?
> }
> }
>
> err = -ENXIO;
> -enddel:
> +enddelpath:
> + mesh_paths_generation++;
> + spin_unlock(&tbl->hashwlock[hash_idx]);
> + read_unlock_bh(&pathtbl_resize_lock);
> + return err;
> +}
> +
> +/**
> + * mpp_path_del - delete a mesh proxy path from the table
> + *
> + * @addr: addr address (ETH_ALEN length)
> + * @sdata: local subif
> + *
> + * Returns: 0 if successful
> + */
> +static int mpp_path_del(struct ieee80211_sub_if_data *sdata, const
> u8 *addr)
> +{
> + struct mesh_table *tbl;
> + struct mesh_path *mpath;
> + struct mpath_node *node;
> + struct hlist_head *bucket;
> + int hash_idx;
> + int err = 0;
> +
> + /* flush relevant mpp entries first */
> + mpp_flush_by_proxy(sdata, addr);
> +
> + read_lock_bh(&pathtbl_resize_lock);
> + tbl = resize_dereference_mpp_paths();
> + hash_idx = mesh_table_hash(addr, sdata, tbl);
> + bucket = &tbl->hash_buckets[hash_idx];
> +
> + spin_lock(&tbl->hashwlock[hash_idx]);
> + hlist_for_each_entry(node, bucket, list) {
> + mpath = node->mpath;
> + if (mpath->sdata == sdata &&
> + ether_addr_equal(addr, mpath->dst)) {
> + __mesh_path_del(tbl, node);
> + goto enddelmpp;
This has the same use-after-free, I'd say?
And another instance later in this file.
> + spin_lock_bh(&mppath-
> >state_lock);
> + mppath->exp_time = jiffies;
That locking seems fairly pointless? You only write to this value here,
and it's an unsigned long, so in itself it's atomic and there's no
synchronisation against anything else needed.
johannes
On Tue, 2016-01-19 at 09:04 +0100, Henning Rogge wrote:
>
> +static void mpp_flush_by_proxy(struct ieee80211_sub_if_data *sdata,
> + const u8 *proxy)
> +{
> + struct mesh_table *tbl;
> + struct mesh_path *mpp;
> + struct mpath_node *node;
> + int i;
> +
> + rcu_read_lock();
> + read_lock_bh(&pathtbl_resize_lock);
> + tbl = resize_dereference_mpp_paths();
> + for_each_mesh_entry(tbl, node, i) {
It does not seem to me that the rcu_read_lock() above is necessary or
correct, though it's probably not hurting it should be removed to avoid
having misleading code.
> + mpp = node->mpath;
> + if (ether_addr_equal(mpp->mpp, proxy)) {
> + spin_lock(&tbl->hashwlock[i]);
> + __mesh_path_del(tbl, node);
> + spin_unlock(&tbl->hashwlock[i]);
It also doesn't seem like for_each_mesh_entry() can deal with "node"
getting deleted from underneath it? It accesses it through
hlist_next_rcu() after the deletion, so you have a use-after-free here
afaict.
johannes
On Tue, Jan 26, 2016 at 10:32:23PM +0100, Johannes Berg wrote:
> On Tue, 2016-01-26 at 22:31 +0100, Johannes Berg wrote:
> > On Tue, 2016-01-26 at 16:22 -0500, Bob Copeland wrote:
> > > ?
> > > void mesh_path_flush_by_nexthop(struct sta_info *sta)
> > > {
> > > ????[...]
> > > rhashtable_walk_start(&iter);
> >
> > It seems you need to check the return value here?
> >
>
> Actually, maybe not. Not sure I understand the return to beginning
> though.
I'll RFC that patch, it doesn't work right now anyway as
rhashtable_walk_init() does a GFP_KERNEL allocation and we want to call
it from the RX path (maybe not a good idea, but that's what the code
currently does).
--
Bob Copeland %% http://bobcopeland.com/
On Tue, Jan 26, 2016 at 12:44 PM, Johannes Berg
<[email protected]> wrote:
> On Tue, 2016-01-19 at 09:04 +0100, Henning Rogge wrote:
>
>> +++ b/net/mac80211/mesh_pathtbl.c
>> @@ -929,12 +929,55 @@ int mesh_path_del(struct ieee80211_sub_if_data
>> *sdata, const u8 *addr)
>> if (mpath->sdata == sdata &&
>> ether_addr_equal(addr, mpath->dst)) {
>> __mesh_path_del(tbl, node);
>> - goto enddel;
>> + goto enddelpath;
>
> C labels are scoped, why rename it?
I think I had a warning with the two labels the same... might be an IDE mistake.
>> }
>> }
>>
>> err = -ENXIO;
>> -enddel:
>> +enddelpath:
>> + mesh_paths_generation++;
>> + spin_unlock(&tbl->hashwlock[hash_idx]);
>> + read_unlock_bh(&pathtbl_resize_lock);
>> + return err;
>> +}
>> +
>> +/**
>> + * mpp_path_del - delete a mesh proxy path from the table
>> + *
>> + * @addr: addr address (ETH_ALEN length)
>> + * @sdata: local subif
>> + *
>> + * Returns: 0 if successful
>> + */
>> +static int mpp_path_del(struct ieee80211_sub_if_data *sdata, const
>> u8 *addr)
>> +{
>> + struct mesh_table *tbl;
>> + struct mesh_path *mpath;
>> + struct mpath_node *node;
>> + struct hlist_head *bucket;
>> + int hash_idx;
>> + int err = 0;
>> +
>> + /* flush relevant mpp entries first */
>> + mpp_flush_by_proxy(sdata, addr);
>> +
>> + read_lock_bh(&pathtbl_resize_lock);
>> + tbl = resize_dereference_mpp_paths();
>> + hash_idx = mesh_table_hash(addr, sdata, tbl);
>> + bucket = &tbl->hash_buckets[hash_idx];
>> +
>> + spin_lock(&tbl->hashwlock[hash_idx]);
>> + hlist_for_each_entry(node, bucket, list) {
>> + mpath = node->mpath;
>> + if (mpath->sdata == sdata &&
>> + ether_addr_equal(addr, mpath->dst)) {
>> + __mesh_path_del(tbl, node);
>> + goto enddelmpp;
>
> This has the same use-after-free, I'd say?
> And another instance later in this file.
>
>
>> + spin_lock_bh(&mppath-
>> >state_lock);
>> + mppath->exp_time = jiffies;
>
> That locking seems fairly pointless? You only write to this value here,
> and it's an unsigned long, so in itself it's atomic and there's no
> synchronisation against anything else needed.
Most likely you are right... kernel locking is an arcane knowledge and
I am still looking for a good way to get initiated into it.
Henning
On Tue, 2016-01-26 at 16:22 -0500, Bob Copeland wrote:
>
> void mesh_path_flush_by_nexthop(struct sta_info *sta)
> {
> [...]
> rhashtable_walk_start(&iter);
It seems you need to check the return value here?
> while ((mpath = rhashtable_walk_next(&iter))) {
> if (IS_ERR(mpath) && PTR_ERR(mpath) == -EAGAIN)
> continue;
> if (IS_ERR(mpath))
> break;
>
> if (rcu_access_pointer(mpath->next_hop) == sta)
> __mesh_path_del(tbl, mpath);
> }
> rhashtable_walk_stop(&iter);
> [...]
> }
>
> ...this still relies on the rcu read lock inside _walk_start and
> _walk_stop, though.
At least that's kinda hidden in walk_start/stop though ;-)
johannes