Re: [RFC] struct filename, io_uring and audit troubles

[Jens Axboe <axboe@kernel.dk>]

On 9/22/24 7:50 PM, Al Viro wrote:
> On Sun, Sep 22, 2024 at 01:49:01AM +0100, Al Viro wrote:
> 
>> 	Another fun bit is that both audit_inode() and audit_inode_child()
>> may bump the refcount on struct filename.  Which can get really fishy
>> if they get called by helper thread while the originator is exiting the
>> syscall - putname() from audit_free_names() in originator vs. refcount
>> increment in helper is Not Nice(tm), what with the refcount not being
>> atomic.
> 
> *blink*
> 
> OK, I really wonder which version had I been reading at the time; refcount
> is, indeed, atomic these days.
> 
> Other problems (->aname pointing to other thread's struct audit_names
> and outliving reuse of those, as well as insane behaviour of audit predicates
> on symlink(2)) are, unfortunately, quite real - on the current mainline.

Traveling but took a quick look. As far as I can tell, for the "reuse
someone elses aname", we could do either:

1) Just don't reuse the entry. Then we can drop the struct
   filename->aname completely as well. Yes that might incur an extra
   alloc for the odd case of audit_enabled and being deep enough that
   the preallocated names have been used, but doesn't anyone really
   care? It'll be noise in the overhead anyway. Side note - that would
   unalign struct filename again. Would be nice to drop audit_names from
   a core fs struct...

2) Add a ref to struct audit_names, RCU kfree it when it drops to zero.
   This would mean dropping struct audit_context->preallocated_names, as
   otherwise we'd run into trouble there if a context gets blown away
   while someone else has a ref to that audit_names struct. We could do
   this without a ref as well, as long as we can store an audit_context
   pointer in struct audit_names and be able to validate it under RCU.
   If ctx doesn't match, don't use it.

And probably other ways too, those were just the two immediate ones I
thought it. Seems like option 1 is simpler and just fine? Quick hack:

diff --git a/fs/namei.c b/fs/namei.c
index 891b169e38c9..11263f779b96 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -206,7 +206,6 @@ getname_flags(const char __user *filename, int flags)
 
 	atomic_set(&result->refcnt, 1);
 	result->uptr = filename;
-	result->aname = NULL;
 	audit_getname(result);
 	return result;
 }
@@ -254,7 +253,6 @@ getname_kernel(const char * filename)
 	}
 	memcpy((char *)result->name, filename, len);
 	result->uptr = NULL;
-	result->aname = NULL;
 	atomic_set(&result->refcnt, 1);
 	audit_getname(result);
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 0df3e5f0dd2b..859244c877b4 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2685,10 +2685,8 @@ struct filename {
 	const char		*name;	/* pointer to actual string */
 	const __user char	*uptr;	/* original userland pointer */
 	atomic_t		refcnt;
-	struct audit_names	*aname;
 	const char		iname[];
 };
-static_assert(offsetof(struct filename, iname) % sizeof(long) == 0);
 
 static inline struct mnt_idmap *file_mnt_idmap(const struct file *file)
 {
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index cd57053b4a69..09caf8408225 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2240,7 +2240,6 @@ void __audit_getname(struct filename *name)
 
 	n->name = name;
 	n->name_len = AUDIT_NAME_FULL;
-	name->aname = n;
 	atomic_inc(&name->refcnt);
 }
 
@@ -2325,22 +2324,6 @@ void __audit_inode(struct filename *name, const struct dentry *dentry,
 	if (!name)
 		goto out_alloc;
 
-	/*
-	 * If we have a pointer to an audit_names entry already, then we can
-	 * just use it directly if the type is correct.
-	 */
-	n = name->aname;
-	if (n) {
-		if (parent) {
-			if (n->type == AUDIT_TYPE_PARENT ||
-			    n->type == AUDIT_TYPE_UNKNOWN)
-				goto out;
-		} else {
-			if (n->type != AUDIT_TYPE_PARENT)
-				goto out;
-		}
-	}
-
 	list_for_each_entry_reverse(n, &context->names_list, list) {
 		if (n->ino) {
 			/* valid inode number, use that for the comparison */

-- 
Jens Axboe