Re: [PATCH 0/8] namespaces: Introduce generic refcount

From: Kees Cook <keescook@chromium.org>
To: Christian Brauner <christian.brauner@ubuntu.com>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>,
	Kirill Tkhai <ktkhai@virtuozzo.com>,
	akpm@linux-foundation.org, viro@zeniv.linux.org.uk,
	adobriyan@gmail.com, davem@davemloft.net,
	linux-kernel@vger.kernel.org, avagin@gmail.com, serge@hallyn.com,
	Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH 0/8] namespaces: Introduce generic refcount
Date: Tue, 4 Aug 2020 14:38:29 -0700	[thread overview]
Message-ID: <202008041421.E8F1A404A@keescook> (raw)
In-Reply-To: <20200804145714.rcz6vg3bo6chsuoh@wittgenstein>

On Tue, Aug 04, 2020 at 04:57:14PM +0200, Christian Brauner wrote:
> On Tue, Aug 04, 2020 at 08:21:51AM -0500, Eric W. Biederman wrote:
> > This change is not as trivial as a spelling change so it is not ok to
> > say it is just a cleanup and move on.  A change in the reference
> > counting can be noticable.  This needs at least to be acknowledged in
> > the change log and at a minimum a hand wavy reason put forth why it is
> > ok.
> > 
> > 
> > Instead what I am seeing as justification is this is a trivial cleanup
> > and no one will notice or care.   And it is not that trivial so I
> > object to the patchset.
> 
> The "not seeing a motivation for this" is a suprising argument to me to
> which I honestly can only respond that I don't see a motivation for not
> doing this. It unifies and simplifies code and removes variance.

Thanks for the CC! Ironically I had noticed this series yesterday at the
end of my day and thought to myself, "that's great; I need to go Ack
that tomorrow" and added it to my TODO list. :P

> > So I am opposed because the patchset does not explain at all why it
> > makes sense to do, nor what tradeoffs were considered, nor what
> > testing was done.

This is a reasonable concern; to me the rationale is clear, but you have
a point that it's not very well spelled out. From my perspective I see
the following benefits:

- The code is simplified because now there is a single lifetime counter
  and it goes in the right place: ns_common ... because it's common to
  all of the namespaces. (Yes there are other counters, but the lifetime
  count is common to all of them.) And the stats on the refactoring
  seems to support it with "51 insertions(+), 72 deletions(-)" which,
  while not "proof" that it's correct, is a positive signal that it's
  going in the right direction, implying a reduced maintenance cost.

- refcount_t is safer than atomic_t, and just as fast[1].

- This reduces the atomic_t -> refcount_t delta[2] that is still pending
  from Elena's efforts started in 2017.

So, while the commit logs could perhaps be clarified (and maybe include
a "in preparation for showing namespaces in ..." prefix describing the
work that triggered this clean-up), I think this is a good idea. Please
consider the whole series:

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

[1] https://git.kernel.org/linus/dcb786493f3e48da3272b710028d42ec608cfda1
[2] https://lore.kernel.org/lkml/1511954324-14414-1-git-send-email-elena.reshetova@intel.com/

-- 
Kees Cook