linux-kernel - Re: [PATCH] locking: type cleanup when accessing fast_read

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]

Message-ID: <20150519112513.GA1337@opentech.at>
Date:	Tue, 19 May 2015 13:25:13 +0200
From:	Nicholas Mc Guire <der.herr@...r.at>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	Nicholas Mc Guire <hofrat@...dl.org>,
	Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org,
	Oleg Nesterov <oleg@...hat.com>
Subject: Re: [PATCH] locking: type cleanup when accessing fast_read_ctr

On Tue, 19 May 2015, Peter Zijlstra wrote:

> On Mon, May 18, 2015 at 07:48:02PM +0200, Nicholas Mc Guire wrote:
> > static code checking with coccinelle was unhappy with:
> > ./kernel/locking/percpu-rwsem.c:113 WARNING: return of wrong type
> > 	int != unsigned int
> > 
> > 
> > current state:
> > 
> > include/linux/percpu-rwsem.h:
> > struct percpu_rw_semaphore {
> >   unsigned int __percpu   *fast_read_ctr;
> >   ...
> >   atomic_t                slow_read_ctr;
> > 
> > allocated as int:
> > 
> > int __percpu_init_rwsem(struct percpu_rw_semaphore *brw,
> >                         const char *name, struct lock_class_key *rwsem_key)
> > {
> > 	brw->fast_read_ctr = alloc_percpu(int); 
> > 
> > used compliant with type int in:
> > 
> > static bool update_fast_ctr(struct percpu_rw_semaphore *brw, 
> > 			    unsigned int val)
> > usage (2):
> >   update_fast_ctr(brw, +1)
> >   update_fast_ctr(brw, -1)
> >  
> > so val should be int here not unsigned int 
> > 
> > static int clear_fast_ctr(struct percpu_rw_semaphore *brw)
> > usage (1):
> >   atomic_add(clear_fast_ctr(brw), &brw->slow_read_ctr);
> > 
> > slow_read_ctr is atomic_t which is int so it would be consistent here to
> > have fast_read_ctr being changed to int as well.
> > 
> > This patch changes:
> >   fast_read_ctr to int
> >   clear_fast_ctr():sum to int
> >   update_fast_ctr():val to int
> > to make usage of fast_read_ctr consistent with respect to type.
> > 
> >   ( Patch was build tested with x86_64_defconfig + 
> >     CONFIG_UPROBE_EVENT (implies CONFIG_PERCPU_RWSEM=y))
> > 
> > Signed-off-by: Nicholas Mc Guire <hofrat@...dl.org>
> 
> So the value is unsigned by purpose; that said we should never cross the
> 2G I think, so it really doesn't matter much.

I assumed it would not matter but did not see a simple way of getting it
type clean with unsigned either mainly due to the atomic_t being int and
val in update_fast_ctr() being passed as -1.

thanks for the review comments.

thx!
hofrat
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/