Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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.