Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <ab758860-e51e-409c-8353-6205fbe515dc@kernel.dk>
Date:   Fri, 6 Oct 2023 20:32:47 -0600
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: audit: io_uring openat triggers audit reference count underflow
 in worker thread
Content-Language: en-US
To:     Dan Clash <Dan.Clash@microsoft.com>,
        "audit@vger.kernel.org" <audit@vger.kernel.org>,
        "io-uring@vger.kernel.org" <io-uring@vger.kernel.org>
Cc:     "paul@paul-moore.com" <paul@paul-moore.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <MW2PR2101MB1033FFF044A258F84AEAA584F1C9A@MW2PR2101MB1033.namprd21.prod.outlook.com>
From:   Jens Axboe <axboe@kernel.dk>
In-Reply-To: <MW2PR2101MB1033FFF044A258F84AEAA584F1C9A@MW2PR2101MB1033.namprd21.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 10/6/23 2:09 PM, Dan Clash wrote:
> diff --git a/fs/namei.c b/fs/namei.c
> index 2a8baa6ce3e8..4f7ac131c9d1 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -187,7 +187,7 @@ getname_flags(const char __user *filename, int flags, int *empty)
>  		}
>  	}
> 
> -	result->refcnt = 1;
> +	refcount_set(&result->refcnt, 1);
>  	/* The empty path is special. */
>  	if (unlikely(!len)) {
>  		if (empty)
> @@ -248,7 +248,7 @@ getname_kernel(const char * filename)
>  	memcpy((char *)result->name, filename, len);
>  	result->uptr = NULL;
>  	result->aname = NULL;
> -	result->refcnt = 1;
> +	refcount_set(&result->refcnt, 1);
>  	audit_getname(result);
> 
>  	return result;
> @@ -259,9 +259,10 @@ void putname(struct filename *name)
>  	if (IS_ERR(name))
>  		return;
> 
> -	BUG_ON(name->refcnt <= 0);
> +	BUG_ON(refcount_read(&name->refcnt) == 0);
> +	BUG_ON(refcount_read(&name->refcnt) == REFCOUNT_SATURATED);
> 
> -	if (--name->refcnt > 0)
> +	if (!refcount_dec_and_test(&name->refcnt))
>  		return;
> 
>  	if (name->name != name->iname) {
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index d0a54e9aac7a..8217e07726d4 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2719,7 +2719,7 @@ struct audit_names;
>  struct filename {
>  	const char		*name;	/* pointer to actual string */
>  	const __user char	*uptr;	/* original userland pointer */
> -	int			refcnt;
> +	refcount_t		refcnt;
>  	struct audit_names	*aname;
>  	const char		iname[];
>  };
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 37cded22497e..232e0be9f6d9 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2188,7 +2188,7 @@ __audit_reusename(const __user char *uptr)
>  		if (!n->name)
>  			continue;
>  		if (n->name->uptr == uptr) {
> -			n->name->refcnt++;
> +			refcount_inc(&n->name->refcnt);
>  			return n->name;
>  		}
>  	}
> @@ -2217,7 +2217,7 @@ void __audit_getname(struct filename *name)
>  	n->name = name;
>  	n->name_len = AUDIT_NAME_FULL;
>  	name->aname = n;
> -	name->refcnt++;
> +	refcount_inc(&name->refcnt);
>  }
> 
>  static inline int audit_copy_fcaps(struct audit_names *name,
> @@ -2349,7 +2349,7 @@ void __audit_inode(struct filename *name, const struct dentry *dentry,
>  		return;
>  	if (name) {
>  		n->name = name;
> -		name->refcnt++;
> +		refcount_inc(&name->refcnt);
>  	}
> 
>  out:
> @@ -2474,7 +2474,7 @@ void __audit_inode_child(struct inode *parent,
>  		if (found_parent) {
>  			found_child->name = found_parent->name;
>  			found_child->name_len = AUDIT_NAME_FULL;
> -			found_child->name->refcnt++;
> +			refcount_inc(&found_child->name->refcnt);
>  		}
>  	}

I'm not fully aware of what audit is doing with struct filename outside
of needing it for the audit log. Rather than impose the atomic
references for everyone, would it be doable to simply dupe the struct
instead of grabbing the (non-atomic) reference to the existing one?

If not, since there's no over/underflow handling right now, it'd
certainly be cheaper to use an atomic_t here rather than a full
refcount.

-- 
Jens Axboe