Re: [PATCH 1/6] VFS: improve interface for lookup_one functions

[Al Viro <viro@xxxxxxxxxxxxxxxxxx>]

On Wed, Mar 19, 2025 at 02:01:32PM +1100, NeilBrown wrote:

> -struct dentry *lookup_one(struct mnt_idmap *idmap, const char *name,
> -			  struct dentry *base, int len)
> +struct dentry *lookup_one(struct mnt_idmap *idmap, struct qstr name,
> +			  struct dentry *base)

>  {
>  	struct dentry *dentry;
>  	struct qstr this;
> @@ -2942,7 +2940,7 @@ struct dentry *lookup_one(struct mnt_idmap *idmap, const char *name,
>  
>  	WARN_ON_ONCE(!inode_is_locked(base->d_inode));
>  
> -	err = lookup_one_common(idmap, name, base, len, &this);
> +	err = lookup_one_common(idmap, name.name, base, name.len, &this);

No.  Just look at what lookup_one_common() is doing as the first step.

        this->name = name;
	this->len = len;

You copy your argument's fields to corresponding fields of *&this.  It might make
sense to pass a qstr, but not like that - just pass a _pointer_ to struct qstr instead.

Have lookup_one_common() do this:

static int lookup_one_common(struct mnt_idmap *idmap,
                             struct qstr *this, struct dentry *base)
{
	const unsigned char *name = this->name;
	int len = this->len;
        if (!len)
                return -EACCES;

        this->hash = full_name_hash(base, name, len);
        if (is_dot_dotdot(name, len))
                return -EACCES;

        while (len--) {
                unsigned int c = *name++;
                if (c == '/' || c == '\0')
                        return -EACCES;
        }
        /*
         * See if the low-level filesystem might want
         * to use its own hash..
         */
        if (base->d_flags & DCACHE_OP_HASH) {
                int err = base->d_op->d_hash(base, this);
                if (err < 0)
                        return err;
        }

        return inode_permission(idmap, base->d_inode, MAY_EXEC);
}

and adjust the callers; e.g.
struct dentry *lookup_one(struct mnt_idmap *idmap, struct qstr *this,
			  struct dentry *base)
{
        struct dentry *dentry;
        int err;

        WARN_ON_ONCE(!inode_is_locked(base->d_inode));

        err = lookup_one_common(idmap, this, base);
        if (err)
                return ERR_PTR(err);

        dentry = lookup_dcache(this, base, 0);
        return dentry ? dentry : __lookup_slow(this, base, 0);
}

with callers passing idmap, &QSTR_LEN(name, len), base instead of
idmap, name, base, len.  lookup_one_common() looks at the fields
separately; its callers do not.