[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2236FBA76BA1254E88B949DDB74E612B41C1891A@IRSMSX102.ger.corp.intel.com>
Date: Mon, 28 Nov 2016 14:12:19 +0000
From: "Reshetova, Elena" <elena.reshetova@...el.com>
To: Peter Zijlstra <peterz@...radead.org>
CC: "kernel-hardening@...ts.openwall.com"
<kernel-hardening@...ts.openwall.com>, Greg KH <gregkh@...uxfoundation.org>,
Kees Cook <keescook@...omium.org>, "will.deacon@....com"
<will.deacon@....com>, Boqun Feng <boqun.feng@...il.com>, Hans Liljestrand
<ishkamiel@...il.com>, David Windsor <dwindsor@...il.com>, "aik@...abs.ru"
<aik@...abs.ru>, "david@...son.dropbear.id.au" <david@...son.dropbear.id.au>
Subject: RE: Conversion from atomic_t to refcount_t: summary of issues
> > Also, refcount_add() seems to be needed in number of places since it
> > looks like refcounts in some cases are increased by two or by some
> > constant. Luckily we haven't seen a need a sub().
>
> There is sub_and_test() usage in for example memcontrol.c.
Right (sub_and_test is one pattern I haven't processed yet, but I am *hoping* that there are not that many).
Also now I run into atomic_sub(len -1, &sk->sk_wmem_alloc) in net/core/sock.c
>
> > The following functions are also needed quite commonly:
>
> > refcount_inc_return()
> > refcount_dec_return()
>
> What for? They don't typicaly make sense for refcounting? Other than the
> trivial pattern of dec_return() == 0, which is already well covered.
Well, I guess you have to ask developers what for. They do verify the return to be equal to different numbers or values...
Sometimes it is ">0", sometimes "==1", sometimes "!=-1". These all seem to be border cases and checked in some scenarios.
> > I also saw one use of this from net/ipv4/udp.c:
> > if (!sk || !atomic_inc_not_zero_hint(&sk->sk_refcnt, 2))
>
> Yes, that one is quite unfortunate, we can trivially support that
> ofcourse, but it does make a bit of a mess of things.
>
> > Lastly as I mentioned previously, almost half of invocations of dec()
> > in the code is plain atomic_dec() without any if statements and any
> > checks on what happens as a result of dec(). Peter previously
> > suggested to turn them into WARN_ON(refcount_dec_and_test()), but
> > looking in the code, it is not really clear what would this help to
> > achieve?
>
> Well, it clearly marks where refcounting goes bad and we leak crap. A
> regular decrement should _never_ hit 0.
>
> > It is clear that in that places the caller explicitly
> > doesn't care about how the dec() goes and what is the end result....
>
> No, the typical usage would be you _know_ it will not hit 0. Any other
> usage is broken and bad.
Ok, makes sense.
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
