netdev - Re: [IPV4] ROUTE: Avoid sparse warnings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 08 Jan 2008 08:53:10 +0100
From:	Eric Dumazet <dada1@...mosbay.com>
To:	David Miller <davem@...emloft.net>
CC:	netdev@...r.kernel.org
Subject: Re: [IPV4] ROUTE: Avoid sparse warnings

David Miller a écrit :
> From: Eric Dumazet <dada1@...mosbay.com>
> Date: Mon, 7 Jan 2008 12:01:17 +0100
> 
>>   CHECK   net/ipv4/route.c
>> net/ipv4/route.c:298:2: warning: context imbalance in 'rt_cache_get_first' - wrong count at exit
>> net/ipv4/route.c:307:3: warning: context imbalance in 'rt_cache_get_next' - unexpected unlock
>> net/ipv4/route.c:346:3: warning: context imbalance in 'rt_cache_seq_stop' - unexpected unlock
>>
>> Signed-off-by: Eric Dumazet <dada1@...mosbay.com>
>>
>> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
>> index 10915bb..fec0571 100644
>> --- a/net/ipv4/route.c
>> +++ b/net/ipv4/route.c
>> @@ -289,11 +289,11 @@ static struct rtable *rt_cache_get_first(struct seq_file *seq)
>>  	struct rt_cache_iter_state *st = seq->private;
>>  
>>  	for (st->bucket = rt_hash_mask; st->bucket >= 0; --st->bucket) {
>> -		rcu_read_lock_bh();
>>  		r = rt_hash_table[st->bucket].chain;
>>  		if (r)
>>  			break;
>>  		rcu_read_unlock_bh();
>> +		rcu_read_lock_bh();
>>  	}
>>  	return r;
>>  }
> 
> Like for Herbert, this patch leaves a bad taste in my mouth.
> 
> I don't understand, is it the case that sparse doesn't like
> that rt_cache_get_first() returns with rcu_read_lock_bh()
> held?  That's kind of rediculious.

Not exactly.
It warns us because rt_cache_get_first() can returns with RCU_BH *acquired* or 
not. I find this warning very usefull. In rt_cache_get_first() this warning
is a false alarm.

> 
> Furthermore, these:
> 
> 	rcu_read_unlock_bh()
> 	rcu_read_lock_bh()
> 
> sequences are at best funny looking.  For other lock types we would
> look at this and ask "Does this even accomplish anything reliably?"

Well, original code exactly does the same thing.

> 
> The answer here is that it wants the preempt_enable() to run to get
> any potential kernel preemptions executed.  It also allows any
> pending software interrupts to run.
> 
> So this does something reliably only because rcu_read_unlock_bh() has
> specific and explicit side effects.
> 

I will post a patch to introduce a helper function, so that this is clearly 
documented and not relying on side effects. Actual implementation has latency
problems on empty hash tables if CONFIG_PREEMPT=n


> I don't know, to me it just looks awful :-)  I better understood the
> original code.
> 
> We can continue splitting hairs over this but I'll hold off on this
> patch for now. :)

Fair enough

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