Re: [PATCH] fs: namespace: convert mnt_namespace.count from atomic_t to refcount_t

Re: [PATCH] fs: namespace: convert mnt_namespace.count from atomic_t to refcount_t

[Al Viro]

On Wed, Nov 28, 2018 at 07:30:10AM +0000, Yang Xiao wrote:
> From: Young Xiao <YangX92@hotmail.com>
> 
> refcount_t type and corresponding API should be
					^^^^^^
					ITYM "could"
> used instead of atomic_t when the variable is used as
> a reference counter. This allows to avoid accidental
> refcounter overflows that might lead to use-after-free
> situations.


>  static inline void get_mnt_ns(struct mnt_namespace *ns)
>  {
> -	atomic_inc(&ns->count);
> +	if (ns)
> +		refcount_inc(&ns->count);
>  }

And this can be called with NULL ns... how, exactly?

>  void put_mnt_ns(struct mnt_namespace *ns)
>  {
> -	if (!atomic_dec_and_test(&ns->count))
> +	if (!ns || !refcount_dec_and_test(&ns->count))
>  		return;

Ditto.  Incidentally, if you are into "defensive programming"
voodoo, how do you choose between checking for NULL and checking
for ERR_PTR(...)?

This kind of "just in case" stuff has its place, but it should
never be used mindlessly.  NAK, unless you add a decent analysis
of the situation and a better rationale.

Re: [PATCH] fs: namespace: convert mnt_namespace.count from atomic_t to refcount_t

[Al Viro]

On Thu, Nov 29, 2018 at 01:58:40AM +0000, Yang Xiao wrote:
> Hello, see commit 387ad9674b00 ("kernel: convert cgroup_namespace.count 
> from atomic_t to refcount_t") for detail.

That would be
    refcount_t type and corresponding API should be
    used instead of atomic_t when the variable is used as
    a reference counter. This allows to avoid accidental
    refcounter overflows that might lead to use-after-free
    situations.

AFAICS, that is the text you'd put into the original posting.  Verbatim.

Which additional information have I failed to notice in the commit
you'd mentioned?