Re: [PATCH] negative dev use in /proc/net/rose_neigh

[David Miller <davem@xxxxxxxxxxxxx>]

From: Bernard Pidoux <bpidoux@xxxxxxx>
Date: Tue, 30 Sep 2008 23:44:55 +0200

> I suspect that the bug was unravelled when we added more than one
> neighbour per route. The protocole accepts three, but this was not
> much used during the early days since the density of radio stations
> on the network was not big (only one node station per destination
> address usually). The network is now denser with Internet links.
>
> However, I don't understand why the test
> 
> if (rose->neighbour == neigh)
> 
> does not work, for
> rose->neighbour = NULL;
> should prevent next comparison to be valid and thus instruction
> rose->neighbour->use--; not executed.
>
> I have seen that a problem with sk_for_each() macro was identified a
> while ago into ax25 code. The problem here could be similar ?

I took a look at this some more.

That neighbour case loop you mention does set ->neighbour to NULL.

But other paths do not.  Just look for all of the pieces of code
that do "rose->neighbour->use--;" and you'll find a few that do not
NULL it out.

One such example is rose_kill_by_device().

That would cause a problem because, while rose_disconnect() marks
the socket DEAD, it doesn't actually remove it from "rose_list".
That happens later when the user actually closes the socket or
some other similar event occurs.

Therefore if rose_kill_by_neigh() happens next, the ->neighbour could
match and we'll decrement again.

But I have no idea how safe it is to NULL out this ->neighbour
unconditionally.  Lots of code seems to deref the thing unconditionally.
For example the ROSE_STATE_2 handling in rose_release().

I guess since rose_disconnect() sets sk->sk_state to TCP_CLOSE, we'll
be OK here.

Can you try this patch?

diff --git a/net/rose/af_rose.c b/net/rose/af_rose.c
index a7f1ce1..41dd630 100644
--- a/net/rose/af_rose.c
+++ b/net/rose/af_rose.c
@@ -197,6 +197,7 @@ static void rose_kill_by_device(struct net_device *dev)
 		if (rose->device == dev) {
 			rose_disconnect(s, ENETUNREACH, ROSE_OUT_OF_ORDER, 0);
 			rose->neighbour->use--;
+			rose->neighbour = NULL;
 			rose->device = NULL;
 		}
 	}
@@ -625,6 +626,7 @@ static int rose_release(struct socket *sock)
 
 	case ROSE_STATE_2:
 		rose->neighbour->use--;
+		rose->neighbour = NULL;
 		release_sock(sk);
 		rose_disconnect(sk, 0, -1, -1);
 		lock_sock(sk);
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html