On Sat, 22 Mar 2025, Al Viro wrote:
> 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;
This code is cleaned up in a later patch. lookup_one_common receives the
address of just one qstr which is initialised with qstr that is passed
in.
So on x86_64, the original qstr is passed in as 2 registers. These are
stored in the stack and the address is passed to lookup_noperm_common(),
as lookup_one_common() gets inlined.
We have to put the two values onto the stack at some point, either in
the original callers, or in the lookup_one family of functions. I think
it is cleaner in lookup_one as we don't need to put a & in front of all
the QSTR calls. But we can change it to pass the pointer if you really
think that is better.
Thanks,
NeilBrown
>
> 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.
>
