Re: Inconsistent lock state and possible irq lock inversion dependency detected in ax25.ko

[Jarek Poplawski <jarkao2@gmail.com>]

Bernard Pidoux wrote, On 12/04/2007 11:26 PM:

> Jarek Poplawski wrote:
>> Bernard Pidoux wrote, On 12/02/2007 06:37 PM:
>>
>>> Hi,
>>>
>>> Many thanks for your patch for ~/net/ax25/ax25_subr.c
>>>
>>> Introduction of local_bh_disable() ... local_bh_enable()
>>>
>>> cured the inconsistent lock state related to AX25 connect timeout.
>>>
>>> I have now a stable monoprocessor system running AX25 and ROSE network 
>>> packet switching application FPAC, whether kernel is compiled with or 
>>> without hack option.
>>>
>>> There is no more problem during normal operations.
>>>
>>> This was achieved, thanks to your AX25 patch and the patch from Alexey 
>>> Dobriyan for rose module.
>>>
>>> I also patched rose module in order to get packet routing more 
>>> efficient, taking into account the "restarted" flag that is raised when 
>>> a neighbour node is already connected.
>>>
>>> To summarize the present situation on my Linux machine, I built a patch 
>>> against kernel 2.6.23.9.
>>>
>>> I would appreciate if you could make it included into a next kernel release.
>> ... 
>>
>> Bernard, I'm very glad I could be a little helpful, but I'm not sure of
>> your intentions: my patch proposal is rather trivial interpretation of
>> lockdep's report; I haven't studied AX25 enough even to be sure there is
>> a real lockup possible in this place. Since this change looks not very
>> costly and quite safe, I can 'take a risk' to sign this off after your
>> testing. But anything more is beyond my 'range'.
>>
>> So, since you've spent quite a lot of time on this all, maybe it would
>> be simpler if you've tried the same with the current kernel, and resent
>> "proper" (not gzipped and with changelog) patch or patches. Then, I hope,
>> Ralf, as the maintainer, will make the rest.
>>
>> Regards,
>> Jarek P.
>>
>>
> 
> As required I send again in clear text the summary of ax25 and rose 
> patch against 2.6.23.9.
> As I said, the kernel stability, after applying these patch, is behind us.
> I did not observe any warning nor lockup after a few days of AX25 
> intensive use.
> Also rose module is handling routing of frames much more efficiently.
> 
> This will considerably help us to focus on application programs now.
> I am now concentrating my efforts on ROSE/FPAC and Linux FBB code 
> adjustement.
> 
> Thanks to you all for your help.
> 
> 73 de Bernard, f6bvp
> 
> 
> 


Bernard, I think you've forgotten at least about some distinct
changelog and signed-off-by?

Jarek P.
---

ax25_subr.c part:
Acked-by: Jarek Poplawski <jarkao2@gmail.com>

> ------------------------------------------------------------------------
> 
> diff -pruN a/include/net/rose.h b/include/net/rose.h
> --- a/include/net/rose.h	2007-10-12 18:43:44.000000000 +0200
> +++ b/include/net/rose.h	2007-12-01 23:56:57.000000000 +0100
> @@ -202,6 +202,7 @@ extern struct net_device *rose_dev_first
>  extern struct net_device *rose_dev_get(rose_address *);
>  extern struct rose_route *rose_route_free_lci(unsigned int, struct rose_neigh *);
>  extern struct rose_neigh *rose_get_neigh(rose_address *, unsigned char *, unsigned char *);
> +extern struct rose_neigh *__rose_get_neigh(rose_address *, unsigned char *, unsigned char *);
>  extern int  rose_rt_ioctl(unsigned int, void __user *);
>  extern void rose_link_failed(ax25_cb *, int);
>  extern int  rose_route_frame(struct sk_buff *, ax25_cb *);
> diff -pruN a/net/ax25/ax25_subr.c b/net/ax25/ax25_subr.c
> --- a/net/ax25/ax25_subr.c	2007-10-12 18:43:44.000000000 +0200
> +++ b/net/ax25/ax25_subr.c	2007-12-01 23:32:01.000000000 +0100
> @@ -279,6 +279,7 @@ void ax25_disconnect(ax25_cb *ax25, int 
>  	ax25_link_failed(ax25, reason);
>  
>  	if (ax25->sk != NULL) {
> +		local_bh_disable();
>  		bh_lock_sock(ax25->sk);
>  		ax25->sk->sk_state     = TCP_CLOSE;
>  		ax25->sk->sk_err       = reason;
> @@ -288,5 +289,6 @@ void ax25_disconnect(ax25_cb *ax25, int 
>  			sock_set_flag(ax25->sk, SOCK_DEAD);
>  		}
>  		bh_unlock_sock(ax25->sk);
> +		local_bh_enable(); 
>  	}
>  }
> diff -pruN a/net/rose/af_rose.c b/net/rose/af_rose.c
> --- a/net/rose/af_rose.c	2007-10-12 18:43:44.000000000 +0200
> +++ b/net/rose/af_rose.c	2007-12-02 10:06:31.000000000 +0100
> @@ -62,7 +62,7 @@ int sysctl_rose_window_size             
>  static HLIST_HEAD(rose_list);
>  static DEFINE_SPINLOCK(rose_list_lock);
>  
> -static struct proto_ops rose_proto_ops;
> +static const struct proto_ops rose_proto_ops;
>  
>  ax25_address rose_callsign;
>  
> @@ -741,7 +741,7 @@ static int rose_connect(struct socket *s
>  	sk->sk_state   = TCP_CLOSE;
>  	sock->state = SS_UNCONNECTED;
>  
> -	rose->neighbour = rose_get_neigh(&addr->srose_addr, &cause,
> +	rose->neighbour = __rose_get_neigh(&addr->srose_addr, &cause,
>  					 &diagnostic);
>  	if (!rose->neighbour)
>  		return -ENETUNREACH;
> @@ -773,7 +773,6 @@ static int rose_connect(struct socket *s
>  
>  		rose_insert_socket(sk);		/* Finish the bind */
>  	}
> -rose_try_next_neigh:
>  	rose->dest_addr   = addr->srose_addr;
>  	rose->dest_call   = addr->srose_call;
>  	rose->rand        = ((long)rose & 0xFFFF) + rose->lci;
> @@ -835,12 +834,6 @@ rose_try_next_neigh:
>  	}
>  
>  	if (sk->sk_state != TCP_ESTABLISHED) {
> -	/* Try next neighbour */
> -		rose->neighbour = rose_get_neigh(&addr->srose_addr, &cause, &diagnostic);
> -		if (rose->neighbour)
> -			goto rose_try_next_neigh;
> -
> -		/* No more neighbours */
>  		sock->state = SS_UNCONNECTED;
>  		err = sock_error(sk);	/* Always set at this point */
>  		goto out_release;
> @@ -1481,7 +1474,7 @@ static struct net_proto_family rose_fami
>  	.owner		=	THIS_MODULE,
>  };
>  
> -static struct proto_ops rose_proto_ops = {
> +static const struct proto_ops rose_proto_ops = {
>  	.family		=	PF_ROSE,
>  	.owner		=	THIS_MODULE,
>  	.release	=	rose_release,
> Les fichiers linux-2.6.23.9-orig/net/rose/rose.ko et linux-2.6.23.9/net/rose/rose.ko sont différents.
> diff -pruN a/net/rose/rose_route.c b/net/rose/rose_route.c
> --- a/net/rose/rose_route.c	2007-10-12 18:43:44.000000000 +0200
> +++ b/net/rose/rose_route.c	2007-12-02 00:15:24.000000000 +0100
> @@ -664,25 +664,22 @@ struct rose_route *rose_route_free_lci(u
>  /*
>   *	Find a neighbour given a ROSE address.
>   */
> -struct rose_neigh *rose_get_neigh(rose_address *addr, unsigned char *cause,
> +struct rose_neigh *__rose_get_neigh(rose_address *addr, unsigned char *cause,
>  	unsigned char *diagnostic)
>  {
> -	struct rose_neigh *res = NULL;
>  	struct rose_node *node;
>  	int failed = 0;
>  	int i;
>  
> -	spin_lock_bh(&rose_node_list_lock);
>  	for (node = rose_node_list; node != NULL; node = node->next) {
>  		if (rosecmpm(addr, &node->address, node->mask) == 0) {
>  			for (i = 0; i < node->count; i++) {
> -				if (!rose_ftimer_running(node->neighbour[i])) {
> -					res = node->neighbour[i];
> -					goto out;
> -				} else
> -					failed = 1;
> +				if (node->neighbour[i]->restarted)
> +					return node->neighbour[i];
> +				if (!rose_ftimer_running(node->neighbour[i]))			
> +					return node->neighbour[i];
> +				failed = 1;
>  			}
> -			break;
>  		}
>  	}
>  
> @@ -694,7 +691,16 @@ struct rose_neigh *rose_get_neigh(rose_a
>  		*diagnostic = 0;
>  	}
>  
> -out:
> +	return NULL;
> +}
> +
> +struct rose_neigh *rose_get_neigh(rose_address *addr, unsigned char *cause,
> +	unsigned char *diagnostic)
> +{
> +	struct rose_neigh *res;
> +
> +	spin_lock_bh(&rose_node_list_lock);
> +	res = __rose_get_neigh(addr, cause, diagnostic);
>  	spin_unlock_bh(&rose_node_list_lock);
>  
>  	return res;
> @@ -1019,7 +1025,7 @@ int rose_route_frame(struct sk_buff *skb
>  		rose_route = rose_route->next;
>  	}
>  
> -	if ((new_neigh = rose_get_neigh(dest_addr, &cause, &diagnostic)) == NULL) {
> +	if ((new_neigh = __rose_get_neigh(dest_addr, &cause, &diagnostic)) == NULL) {
>  		rose_transmit_clear_request(rose_neigh, lci, cause, diagnostic);
>  		goto out;
>  	}
>