linux-kernel - Re: + atomic-improve-atomic_inc_unless_negative-atomic_dec_unless

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:	Fri, 15 Mar 2013 18:23:15 +0100
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Ming Lei <tom.leiming@...il.com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Shaohua Li <shli@...nel.org>,
	Al Viro <viro@...iv.linux.org.uk>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: + atomic-improve-atomic_inc_unless_negative-atomic_dec_unless_positive
 .patch added to -mm tree

2013/3/15 Oleg Nesterov <oleg@...hat.com>:
> On 03/15, Ming Lei wrote:
>>
>> On Fri, Mar 15, 2013 at 9:46 PM, Oleg Nesterov <oleg@...hat.com> wrote:
>> > On 03/15, Ming Lei wrote:
>> >>
>> >> On Fri, Mar 15, 2013 at 12:24 AM, Oleg Nesterov <oleg@...hat.com> wrote:
>> >> >  static inline int atomic_inc_unless_negative(atomic_t *p)
>> >> >  {
>> >> >         int v, v1;
>> >> > -       for (v = 0; v >= 0; v = v1) {
>> >> > +       for (v = atomic_read(p); v >= 0; v = v1) {
>> >> >                 v1 = atomic_cmpxchg(p, v, v + 1);
>> >>
>> >> Unfortunately, the above will exchange the current value even though
>> >> it is negative, so it isn't correct.
>> >
>> > Hmm, why? We always check "v >= 0" before we try to do
>> > atomic_cmpxchg(old => v) ?
>>
>> Sorry, yes, you are right. But then your patch is basically same with the
>> previous one, isn't it?
>
> Sure, the logic is the same, just the patch (and the code) looks simpler
> and more understandable.
>
>> And has same problem, see below discussion:
>>
>> http://marc.info/?t=136284366900001&r=1&w=2
>
> The lack of the barrier?
>
> I thought about this, this should be fine? atomic_add_unless() has the same
> "problem", but this is documented in atomic_ops.txt:
>
>         atomic_add_unless requires explicit memory barriers around the operation
>         unless it fails (returns 0).
>
> I thought that atomic_add_unless_negative() should have the same
> guarantees?

I feel very uncomfortable with that. The memory barrier is needed
anyway to make sure we don't deal with a stale value of the atomic val
(wrt. ordering against another object).
The following should really be expected to work without added barrier:

void put_object(foo *obj)
{
      if (atomic_dec_return(obj->ref) == -1)
          free_rcu(obj);
}

bool try_get_object(foo *obj)
{
      if (atomic_add_unless_negative(obj, 1))
           return true;
      return false;
}

= CPU 0 =                = CPU 1
                                rcu_read_lock()
put_object(obj0);
                                obj = rcu_derefr(obj0);
rcu_assign_ptr(obj0, NULL);
                                if (try_get_object(obj))
                                     do_something...
                                else
                                     object is dying
                                rcu_read_unlock()


But anyway I must defer on Paul, he's the specialist here.
--
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/