linux-kernel - RE: [PATCH 01/15] sched: convert sighand_struct.count to refcount

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:   Mon, 23 Oct 2017 10:22:01 +0000
From:   "Reshetova, Elena" <elena.reshetova@...el.com>
To:     Thomas Gleixner <tglx@...utronix.de>
CC:     "mingo@...hat.com" <mingo@...hat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>,
        "tj@...nel.org" <tj@...nel.org>,
        "hannes@...xchg.org" <hannes@...xchg.org>,
        "lizefan@...wei.com" <lizefan@...wei.com>,
        "acme@...nel.org" <acme@...nel.org>,
        "alexander.shishkin@...ux.intel.com" 
        <alexander.shishkin@...ux.intel.com>,
        "eparis@...hat.com" <eparis@...hat.com>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "arnd@...db.de" <arnd@...db.de>,
        "luto@...nel.org" <luto@...nel.org>,
        "keescook@...omium.org" <keescook@...omium.org>,
        "dvhart@...radead.org" <dvhart@...radead.org>,
        "ebiederm@...ssion.com" <ebiederm@...ssion.com>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "axboe@...nel.dk" <axboe@...nel.dk>
Subject: RE: [PATCH 01/15] sched: convert sighand_struct.count to refcount_t

> On Fri, 20 Oct 2017, Elena Reshetova wrote:
> 
> > atomic_t variables are currently used to implement reference
> > counters with the following properties:
> >  - counter is initialized to 1 using atomic_set()
> >  - a resource is freed upon counter reaching zero
> >  - once counter reaches zero, its further
> >    increments aren't allowed
> >  - counter schema uses basic atomic operations
> >    (set, inc, inc_not_zero, dec_and_test, etc.)
> >
> > Such atomic variables should be converted to a newly provided
> > refcount_t type and API that prevents accidental counter overflows
> > and underflows. This is important since overflows and underflows
> > can lead to use-after-free situation and be exploitable.
> >
> > The variable sighand_struct.count is used as pure reference counter.
> 
> This still does not mention that atomic_t != recfcount_t ordering wise and
> why you think that this does not matter in that use case.
>
>
> And looking deeper:
> 
> > @@ -1381,7 +1381,7 @@ static int copy_sighand(unsigned long clone_flags,
> struct task_struct *tsk)
> >  	struct sighand_struct *sig;
> >
> >  	if (clone_flags & CLONE_SIGHAND) {
> > -		atomic_inc(&current->sighand->count);
> > +		refcount_inc(&current->sighand->count);
> >  		return 0;
> 
> >  void __cleanup_sighand(struct sighand_struct *sighand)
> >  {
> > -	if (atomic_dec_and_test(&sighand->count)) {
> > +	if (refcount_dec_and_test(&sighand->count)) {
> 
> How did you make sure that these atomic operations have no other
> serialization effect and can be replaced with refcount?

What serialization effects? Are you taking about smth else than memory
ordering? 

For memory ordering my current hope is that we can just make refcount_t
to use same strict atomic primitives and then it would not make any difference.
I think this would be the simplest way for everyone since I think even some maintainers
are having issues understanding all the implications of "relaxed" ordering. 

Best Regards,
Elena

> 
> I complained about that before and Peter explained it to you in great
> length, but you just resend the same thing again. Where is the correctness
> analysis? Seriously, for this kind of stuff it's not sufficient to use a
> coccinelle script and copy boiler plate change logs and be done with it.
> 
> Thanks,
> 
> 	tglx