Re: [RFC][PATCH 10/13] get rid of audit_reusename()

[Jan Kara <jack@suse.cz>]

On Sun 09-11-25 06:37:42, Al Viro wrote:
> Originally we tried to avoid multiple insertions into audit names array
> during retry loop by a cute hack - memorize the userland pointer and
> if there already is a match, just grab an extra reference to it.
> 
> Cute as it had been, it had problems - two identical pointers had
> audit aux entries merged, two identical strings did not.  Having
> different behaviour for syscalls that differ only by addresses of
> otherwise identical string arguments is obviously wrong - if nothing
> else, compiler can decide to merge identical string literals.
> 
> Besides, this hack does nothing for non-audited processes - they get
> a fresh copy for retry.  It's not time-critical, but having behaviour
> subtly differ that way is bogus.
> 
> These days we have very few places that import filename more than once
> (9 functions total) and it's easy to massage them so we get rid of all
> re-imports.  With that done, we don't need audit_reusename() anymore.
> There's no need to memorize userland pointer either.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/namei.c            | 11 +++--------
>  include/linux/audit.h | 11 -----------
>  include/linux/fs.h    |  1 -
>  kernel/auditsc.c      | 23 -----------------------
>  4 files changed, 3 insertions(+), 43 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 7377020a2cba..dd86e41deeeb 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -125,9 +125,8 @@
>  
>  #define EMBEDDED_NAME_MAX	(PATH_MAX - offsetof(struct filename, iname))
>  
> -static inline void initname(struct filename *name, const char __user *uptr)
> +static inline void initname(struct filename *name)
>  {
> -	name->uptr = uptr;
>  	name->aname = NULL;
>  	atomic_set(&name->refcnt, 1);
>  }
> @@ -139,10 +138,6 @@ getname_flags(const char __user *filename, int flags)
>  	char *kname;
>  	int len;
>  
> -	result = audit_reusename(filename);
> -	if (result)
> -		return result;
> -
>  	result = __getname();
>  	if (unlikely(!result))
>  		return ERR_PTR(-ENOMEM);
> @@ -210,7 +205,7 @@ getname_flags(const char __user *filename, int flags)
>  			return ERR_PTR(-ENAMETOOLONG);
>  		}
>  	}
> -	initname(result, filename);
> +	initname(result);
>  	audit_getname(result);
>  	return result;
>  }
> @@ -268,7 +263,7 @@ struct filename *getname_kernel(const char * filename)
>  		return ERR_PTR(-ENAMETOOLONG);
>  	}
>  	memcpy((char *)result->name, filename, len);
> -	initname(result, NULL);
> +	initname(result);
>  	audit_getname(result);
>  	return result;
>  }
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 536f8ee8da81..d936a604d056 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -316,7 +316,6 @@ extern void __audit_uring_exit(int success, long code);
>  extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
>  				  unsigned long a2, unsigned long a3);
>  extern void __audit_syscall_exit(int ret_success, long ret_value);
> -extern struct filename *__audit_reusename(const __user char *uptr);
>  extern void __audit_getname(struct filename *name);
>  extern void __audit_inode(struct filename *name, const struct dentry *dentry,
>  				unsigned int flags);
> @@ -380,12 +379,6 @@ static inline void audit_syscall_exit(void *pt_regs)
>  		__audit_syscall_exit(success, return_code);
>  	}
>  }
> -static inline struct filename *audit_reusename(const __user char *name)
> -{
> -	if (unlikely(!audit_dummy_context()))
> -		return __audit_reusename(name);
> -	return NULL;
> -}
>  static inline void audit_getname(struct filename *name)
>  {
>  	if (unlikely(!audit_dummy_context()))
> @@ -624,10 +617,6 @@ static inline struct audit_context *audit_context(void)
>  {
>  	return NULL;
>  }
> -static inline struct filename *audit_reusename(const __user char *name)
> -{
> -	return NULL;
> -}
>  static inline void audit_getname(struct filename *name)
>  { }
>  static inline void audit_inode(struct filename *name,
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index c895146c1444..bbae3cfdc338 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2835,7 +2835,6 @@ extern struct kobject *fs_kobj;
>  struct audit_names;
>  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[];
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index d1966144bdfe..e59a094bb9f7 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2169,29 +2169,6 @@ static struct audit_names *audit_alloc_name(struct audit_context *context,
>  	return aname;
>  }
>  
> -/**
> - * __audit_reusename - fill out filename with info from existing entry
> - * @uptr: userland ptr to pathname
> - *
> - * Search the audit_names list for the current audit context. If there is an
> - * existing entry with a matching "uptr" then return the filename
> - * associated with that audit_name. If not, return NULL.
> - */
> -struct filename *
> -__audit_reusename(const __user char *uptr)
> -{
> -	struct audit_context *context = audit_context();
> -	struct audit_names *n;
> -
> -	list_for_each_entry(n, &context->names_list, list) {
> -		if (!n->name)
> -			continue;
> -		if (n->name->uptr == uptr)
> -			return refname(n->name);
> -	}
> -	return NULL;
> -}
> -
>  /**
>   * __audit_getname - add a name to the list
>   * @name: name to add
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR