Message ID | 200809282156.39466.bpidoux@free.fr |
---|---|
State | Rejected, archived |
Delegated to: | David Miller |
Headers | show |
From: Bernard Pidoux <bpidoux@free.fr> Date: Sun, 28 Sep 2008 21:56:38 +0200 > I propose the following patch to avoid use counter underflow. > However a KERN_WARNING message could be better instead of KERN_ERR. I don't see any large value in adding this patch right now. At best it's a new BUG check, it doesn't actually fix the problem. I'd rather apply a fix :-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
I slightly disagree. The patch actually prevents use counter to become underflowed (starting with 65535 then 65534 etc...) The patch makes sure that use never becomes less than 0. This may prevent further problems. However I agree that the real reason of the bug is still unknown. It must be hidden in the very obscure loop (at least for me). Here is the original code : void rose_kill_by_neigh( struct rose_neigh *neigh) { struct sock *s; struct hlist_node *node; spin_lock_bh(&rose_list_lock); sk_for_each(s,node,&rose_list) { struct rose_sock *rose = rose_sk(s); if (rose->neighbour == neigh) { rose_disconnect(s, ENETUNREACH, ROSE_OUT_OF_ORDER, 0); rose->neighbour->use--; rose->neighbour = NULL; } } spin_unlock_bh(&rose_list_lock); } 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 ? Bernard David Miller wrote: > From: Bernard Pidoux <bpidoux@free.fr> > Date: Sun, 28 Sep 2008 21:56:38 +0200 > >> I propose the following patch to avoid use counter underflow. >> However a KERN_WARNING message could be better instead of KERN_ERR. > > I don't see any large value in adding this patch right now. > At best it's a new BUG check, it doesn't actually fix the > problem. I'd rather apply a fix :-) > > -- To unsubscribe from this list: send the line "unsubscribe netdev" 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/net/rose/af_rose.c b/net/rose/af_rose.c index a7f1ce1..8a54cff 100644 --- a/net/rose/af_rose.c +++ b/net/rose/af_rose.c @@ -175,7 +175,10 @@ void rose_kill_by_neigh(struct rose_neigh *neigh) if (rose->neighbour == neigh) { rose_disconnect(s, ENETUNREACH, ROSE_OUT_OF_ORDER, 0); - rose->neighbour->use--; + if (rose->neighbour->use > 0 ) + rose->neighbour->use--; + else + printk(KERN_ERR "ROSE: rose_kill_by_neigh() - neighbour->use-- could be < 0\n"); rose->neighbour = NULL; } }
When running rose network applications, rose_neigh use counter can become negative as shown below. Number 65535 actually represents a short integer underflow, meaning that use counter has been decremented while equal to zero. Then use counter continues to decrease by one each time the function is called. proc/net/rose_neigh addr callsign dev count use mode restart t0 tf digipeaters 00005 F5KCK-11 ax1 4 1 DTE yes 0 0 00004 F6BVP-5 ax4 6 0 DTE no 0 0 00003 F6BVP-7 ax4 6 0 DCE yes 0 0 00002 F6BVP-11 ax4 6 65535 DCE yes 0 0 00001 RSLOOP-0 ??? 1 4 DCE yes 0 0 After investigations I found that use counter value was going negative when rose_kill_by_neigh() (in af_rose.c) was called and sk_for_each() macro loop activated rose->neighbour->use-- more than once. I propose the following patch to avoid use counter underflow. However a KERN_WARNING message could be better instead of KERN_ERR. Signed-off-by: Bernard Pidoux <f6bvp@amsat.org> --- net/rose/af_rose.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)