On Mon, Dec 06, 2021 at 01:17:17PM -0800, Linus Torvalds wrote: > End result: atomics are _better_ in the overflow case, and it's why > the page counters could not use the garbage that is refcount_t, and > instead did it properly. > > See? In absolutely neither case is recount_t "safer". It's only worse. Right, I understand your objection; it is valid. One of the dimensions of "safe" is "not exploitable", which is _also_ a valid concern. As you say, refcount_t works for the "never make them all handle overflows properly" case, and I'm fine with using something else where we need a better set of behaviors. > I like Jens' patches. They take the _good_ code - the code we use for > page counters - and make that proper interface available to others. I am fine with whatever can be made safe in all dimensions, but I don't want to lose sight of the exploitability dimension. This is a lot like the BUG vs WARN situation: BUG is unfixable because there is no recovery. WARN allows the code to do something sensible in the pathological case, but the code needs to have been designed to handle that case. The widely used older atomic_t reference counting code pattern had no path to handling failure. In the proposed API, we get a warning (sometimes) in bad states, but there is no handling of the broken reference counter. For example, with atomic_ref_inc_not_zero(): - there's no __must_check hint for callers to actually check it happened - overflow is silent, so wrapping around to 1 and then having a call to atomic_ref_put_and_test() has no protection against exploitation at all. This particular code pattern mistake (missed "put") is the fundamental path to nearly all the refcount overflow exploits of the past couple decades. e.g. see: - CVE-2016-0728 - Keyring refcount overflow. Exploit: https://www.exploit-db.com/exploits/39277/ - CVE-2016-4558 - BPF reference count mishandling. Explot: https://www.exploit-db.com/exploits/39773/ Losing that protection is just inviting these exploits back again (of which we've had none in the past few years). For the API generally, nothing about the type stops someone from accidentally using the standard atomic_t helpers instead, accidentally bypassing any potential WARNs. It should do something similar to refcount_t so the compiler can help guide people correctly instead of blindly accepting an accident. And if we're speaking to safety/robustness generally, where are the unit tests, run-time tests (LKDTM provides this for refcount_t so it should be easy to repurpose them), kern-doc, etc? I'm not arguing for refcount_t -- I'm arguing for an API that isn't a regression of features that have been protecting the kernel from bugs. -- Kees Cook
