Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
Message-ID: <e0307260-c438-41d9-97ec-563e9932a60e@kernel.dk>
Date:   Sat, 7 Oct 2023 07:11:39 -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
From:   Jens Axboe <axboe@kernel.dk>
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>
 <ab758860-e51e-409c-8353-6205fbe515dc@kernel.dk>
In-Reply-To: <ab758860-e51e-409c-8353-6205fbe515dc@kernel.dk>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 10/6/23 8:32 PM, Jens Axboe wrote:
> 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.

After taking a closer look at this, I think the best course of action
would be to make the struct filename refcnt and atomic_t. With audit in
the picture, it's quite possible to have multiple threads manipulating
the filename refcnt at the same time, which is obviously not currently
safe.

Dan, would you mind sending that as a patch? Include a link to your
original email:

Link: https://lore.kernel.org/lkml/MW2PR2101MB1033FFF044A258F84AEAA584F1C9A@MW2PR2101MB1033.namprd21.prod.outlook.com/

and a Fixes tag as well:

Fixes: 5bd2182d58e9 ("audit,io_uring,io-wq: add some basic audit support to io_uring")

and CC linux-fsdevel@vger.kernel.org and
Christian Brauner <brauner@kernel.org> as well.

Thanks!

-- 
Jens Axboe