Re: [PATCH v10 5/8] fuse: Revalidate positive entries in fuse_atomic_open

[Al Viro <viro@zeniv.linux.org.uk>]

On Mon, Oct 23, 2023 at 08:30:32PM +0200, Bernd Schubert wrote:
> +static struct dentry *
> +fuse_atomic_open_revalidate(struct fuse_conn *fc, struct dentry *entry,
> +			    struct inode *inode, int switched,
> +			    struct fuse_entry_out *outentry,
> +			    wait_queue_head_t *wq, int *alloc_inode)
> +{
> +	u64 attr_version;
> +	struct dentry *prev = entry;
> +
> +	if (outentry->nodeid != get_node_id(inode) ||
> +	    (bool) IS_AUTOMOUNT(inode) !=
> +	    (bool) (outentry->attr.flags & FUSE_ATTR_SUBMOUNT)) {
> +		*alloc_inode = 1;
> +	} else if (fuse_stale_inode(inode, outentry->generation,
> +				  &outentry->attr)) {
> +		fuse_make_bad(inode);
> +		*alloc_inode = 1;
> +	}
> +
> +	if (*alloc_inode) {
> +		struct dentry *new = NULL;
> +
> +		if (!switched && !d_in_lookup(entry)) {
> +			d_drop(entry);
> +			new = d_alloc_parallel(entry->d_parent, &entry->d_name,
> +					       wq);
> +			if (IS_ERR(new))
> +				return new;
> +
> +			if (unlikely(!d_in_lookup(new))) {
> +				dput(new);
> +				new = ERR_PTR(-EIO);
> +				return new;
> +			}
> +		}
> +
> +		fuse_invalidate_entry(entry);
> +
> +		entry = new;
> +	} else if (!*alloc_inode) {
> +		attr_version = fuse_get_attr_version(fc);
> +		forget_all_cached_acls(inode);
> +		fuse_change_attributes(inode, &outentry->attr, NULL,
> +				       ATTR_TIMEOUT(outentry),
> +				       attr_version);
> +	}
> +
> +	if (prev == entry) {
> +		/* nothing changed, atomic-open on the server side
> +		 * had increased the lookup count - do the same here
> +		 */
> +		struct fuse_inode *fi = get_fuse_inode(inode);
> +
> +		spin_lock(&fi->lock);
> +		fi->nlookup++;
> +		spin_unlock(&fi->lock);
> +	}
> +
> +	return entry;
> +}
> +
> +/**
> + * Does 'lookup + create + open' or 'lookup + open' atomically.
> + * @entry might be positive as well, therefore inode is re-validated.
> + * Positive dentry is invalidated in case inode attributes differ or
> + * we encountered error.
> + */
>  static int _fuse_atomic_open(struct inode *dir, struct dentry *entry,
>  			     struct file *file, unsigned int flags,
>  			     umode_t mode)
>  {
>  	int err;
> -	struct inode *inode;
> +	struct inode *inode = d_inode(entry);
>  	FUSE_ARGS(args);
>  	struct fuse_mount *fm = get_fuse_mount(dir);
>  	struct fuse_conn *fc = fm->fc;
> @@ -780,10 +865,7 @@ static int _fuse_atomic_open(struct inode *dir, struct dentry *entry,
>  	struct fuse_file *ff;
>  	struct dentry *switched_entry = NULL, *alias = NULL;
>  	DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq);
> -
> -	/* Expect a negative dentry */
> -	if (unlikely(d_inode(entry)))
> -		goto fallback;
> +	int alloc_inode = 0;
>  
>  	/* Userspace expects S_IFREG in create mode */
>  	if ((flags & O_CREAT) && (mode & S_IFMT) != S_IFREG)
> @@ -835,36 +917,56 @@ static int _fuse_atomic_open(struct inode *dir, struct dentry *entry,
>  
>  	err = fuse_simple_request(fm, &args);
>  	free_ext_value(&args);
> -	if (err == -ENOSYS || err == -ELOOP) {
> -		if (unlikely(err == -ENOSYS))
> -			fc->no_open_atomic = 1;
> -		goto free_and_fallback;
> -	}
>  
>  	if (!err && !outentry.nodeid)
>  		err = -ENOENT;
>  
> -	if (err)
> -		goto out_free_ff;
> +	if (err) {
> +		if (unlikely(err == -ENOSYS)) {
> +			fc->no_open_atomic = 1;
> +
> +			/* Might come up if userspace tricks us and would
> +			 * return -ENOSYS for OPEN_ATOMIC after it was
> +			 * aready working
> +			 */
> +			if (unlikely(fc->has_open_atomic == 1))
> +				pr_info("fuse server/daemon bug, atomic open "
> +					"got -ENOSYS although it was already "
> +					"succeeding before.");
> +
> +			/* This should better never happen, revalidate
> +			 * is missing for this entry
> +			 */
> +			if (WARN_ON_ONCE(d_really_is_positive(entry))) {
> +				err = -EIO;
> +				goto out_free_ff;
> +			}
> +			goto free_and_fallback;
> +		} else if (err == -ELOOP) {
> +			/* likely a symlink */
> +			goto free_and_fallback;
> +		} else {
> +			if (d_really_is_positive(entry)) {
> +				if (err != -EINTR && err != -ENOMEM)
> +					fuse_invalidate_entry(entry);
> +			}
> +
> +			goto out_free_ff;
> +		}
> +	}
> +
> +	if (!err && !fc->has_open_atomic) {
> +		/* Only set this flag when atomic open did not return an error,
> +		 * so that we are absolutely sure it is implemented.
> +		 */
> +		fc->has_open_atomic = 1;
> +	}
>  
>  	err = -EIO;
>  	if (invalid_nodeid(outentry.nodeid) || fuse_invalid_attr(&outentry.attr))
>  		goto out_free_ff;
>  
> -	ff->fh = outopen.fh;
> -	ff->nodeid = outentry.nodeid;
> -	ff->open_flags = outopen.open_flags;
> -	inode = fuse_iget(dir->i_sb, outentry.nodeid, outentry.generation,
> -			  &outentry.attr, ATTR_TIMEOUT(&outentry), 0);
> -	if (!inode) {
> -		flags &= ~(O_CREAT | O_EXCL | O_TRUNC);
> -		fuse_sync_release(NULL, ff, flags);
> -		fuse_queue_forget(fm->fc, forget, outentry.nodeid, 1);
> -		err = -ENOMEM;
> -		goto out_err;
> -	}
> -
> -	/* prevent racing/parallel lookup on a negative hashed */
> +	/* prevent racing/parallel lookup */
>  	if (!(flags & O_CREAT) && !d_in_lookup(entry)) {
>  		d_drop(entry);
>  		switched_entry = d_alloc_parallel(entry->d_parent,
> @@ -879,10 +981,52 @@ static int _fuse_atomic_open(struct inode *dir, struct dentry *entry,
>  			/* fall back */
>  			dput(switched_entry);
>  			switched_entry = NULL;
> -			goto free_and_fallback;
> +
> +			if (!inode) {
> +				goto free_and_fallback;
> +			} else {
> +				/* XXX can this happen at all and is there a
> +				 * better way to handle it?
> +				 */

"this" being !d_in_lookup() on result of d_alloc_parallel()?  Sure,
that's what you get if there had been a lookup on the same thing
when you called d_alloc_parallel().  Or, for that matter, if that
lookup got completed just as you called the damn thing.

What are you trying to achieve here?