linux-kernel - Re: [PATCH 3/6] tracefs: dentry lookup crapectomy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240130232621.GL2087318@ZenIV>
Date: Tue, 30 Jan 2024 23:26:21 +0000
From: Al Viro <viro@...iv.linux.org.uk>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Steven Rostedt <rostedt@...dmis.org>,
	Masami Hiramatsu <mhiramat@...nel.org>,
	linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH 3/6] tracefs: dentry lookup crapectomy

On Tue, Jan 30, 2024 at 11:03:52AM -0800, Linus Torvalds wrote:
> The dentry lookup for eventfs files was very broken, and had lots of
> signs of the old situation where the filesystem names were all created
> statically in the dentry tree, rather than being looked up dynamically
> based on the eventfs data structures.
> 
> You could see it in the naming - how it claimed to "create" dentries
> rather than just look up the dentries that were given it.
> 
> You could see it in various nonsensical and very incorrect operations,
> like using "simple_lookup()" on the dentries that were passed in, which
> only results in those dentries becoming negative dentries.  Which meant
> that any other lookup would possibly return ENOENT if it saw that
> negative dentry before the data rwas then later filled in.
> 
> You could see it in the immesnse amount of nonsensical code that didn't
> actually just do lookups.

> -static struct dentry *create_file(const char *name, umode_t mode,
> +static struct dentry *lookup_file(struct dentry *dentry,
> +				  umode_t mode,
>  				  struct eventfs_attr *attr,
> -				  struct dentry *parent, void *data,
> +				  void *data,
>  				  const struct file_operations *fop)
>  {
>  	struct tracefs_inode *ti;
> -	struct dentry *dentry;
>  	struct inode *inode;
>  
>  	if (!(mode & S_IFMT))
> @@ -307,12 +304,6 @@ static struct dentry *create_file(const char *name, umode_t mode,
>  	if (WARN_ON_ONCE(!S_ISREG(mode)))
>  		return NULL;
>  
> -	WARN_ON_ONCE(!parent);
> -	dentry = eventfs_start_creating(name, parent);

Used to lock the inode of parent.

>  	if (unlikely(!inode))
>  		return eventfs_failed_creating(dentry);

.. and that still unlocks it.

> @@ -331,29 +322,25 @@ static struct dentry *create_file(const char *name, umode_t mode,
>  	ti->flags = TRACEFS_EVENT_INODE;
>  	ti->private = NULL;			// Directories have 'ei', files not
>  
> -	d_instantiate(dentry, inode);
> +	d_add(dentry, inode);
>  	fsnotify_create(dentry->d_parent->d_inode, dentry);
>  	return eventfs_end_creating(dentry);

.. and so does this.

>  };

Where has that inode_lock() gone and how could that possibly work?