Re: [PATCH 1/2] vfs: make LAST_XXX private to fs/namei.c

[NeilBrown]

On Fri, 29 May 2026, Jori Koolstra wrote:
> The only user of LAST_XXX outside of fs/namei.c is fs/smb/server/vfs.c;
> ksmbd_vfs_path_lookup() calls vfs_path_parent_lookup() and expects a
> LAST_NORM last type (or it will be ENOENT). ksmbd_vfs_rename() also calls
> vfs_path_parent_lookup() but forgets the LAST_NORM check.

Are you certain that it "forgets" the LAST_NORM check?
I suspect you are - rename needs a name to change.  But it
often helps to explain an assertion like this.

I would be good to enhance the kernel-doc for vfs_path_parent_lookup()
to make it clear that the path must end with a name component (not /, .,
or ..).

But even without that it is a good cleanup. 

Reviewed-by: NeilBrown <neil@xxxxxxxxxx>

Thanks,
NeilBrown


> 
> It does not really make sense to have vfs_path_parent_lookup() expose
> the last_type because it is only needed to ensure it is LAST_NORM. So
> let's do this check in vfs_path_parent_lookup() instead and keep the
> LAST_XXX internal to fs/namei.c. This changes the ENOENT errno in
> ksmbd_vfs_path_lookup() to EINVAL, which matches better with how this is
> handled by callers of filename_parentat().
> 
> Signed-off-by: Jori Koolstra <jkoolstra@xxxxxxxxx>
> ---
>  fs/namei.c            | 20 ++++++++++++++++----
>  fs/smb/server/vfs.c   | 15 +++------------
>  include/linux/namei.h |  7 +------
>  3 files changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index c7fac83c9a85..889792761bc2 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -129,6 +129,11 @@
>  static struct kmem_cache *__names_cache __ro_after_init;
>  #define names_cache	runtime_const_ptr(__names_cache)
>  
> +/*
> + * Type of the last component on LOOKUP_PARENT
> + */
> +enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT};
> +
>  void __init filename_init(void)
>  {
>  	__names_cache = kmem_cache_create_usercopy("names_cache", sizeof(struct filename), 0,
> @@ -3051,15 +3056,22 @@ EXPORT_SYMBOL(kern_path);
>   * @flags: lookup flags
>   * @parent: pointer to struct path to fill
>   * @last: last component
> - * @type: type of the last component
>   * @root: pointer to struct path of the base directory
>   */
>  int vfs_path_parent_lookup(struct filename *filename, unsigned int flags,
> -			   struct path *parent, struct qstr *last, int *type,
> +			   struct path *parent, struct qstr *last,
>  			   const struct path *root)
>  {
> -	return  __filename_parentat(AT_FDCWD, filename, flags, parent, last,
> -				    type, root);
> +	int type;
> +	int err =  __filename_parentat(AT_FDCWD, filename, flags, parent, last,
> +				       &type, root);
> +	if (err)
> +		return err;
> +	if (unlikely(type != LAST_NORM)) {
> +		path_put(parent);
> +		return -EINVAL;
> +	}
> +	return 0;
>  }
>  EXPORT_SYMBOL(vfs_path_parent_lookup);
>  
> diff --git a/fs/smb/server/vfs.c b/fs/smb/server/vfs.c
> index d08973b288e5..cd1dbca0cffb 100644
> --- a/fs/smb/server/vfs.c
> +++ b/fs/smb/server/vfs.c
> @@ -19,7 +19,6 @@
>  #include <linux/vmalloc.h>
>  #include <linux/sched/xacct.h>
>  #include <linux/crc32c.h>
> -#include <linux/namei.h>
>  #include <linux/splice.h>
>  
>  #include "glob.h"
> @@ -56,7 +55,7 @@ static int ksmbd_vfs_path_lookup(struct ksmbd_share_config *share_conf,
>  {
>  	struct qstr last;
>  	const struct path *root_share_path = &share_conf->vfs_path;
> -	int err, type;
> +	int err;
>  	struct dentry *d;
>  
>  	if (pathname[0] == '\0') {
> @@ -67,17 +66,11 @@ static int ksmbd_vfs_path_lookup(struct ksmbd_share_config *share_conf,
>  	}
>  
>  	CLASS(filename_kernel, filename)(pathname);
> -	err = vfs_path_parent_lookup(filename, flags,
> -				     path, &last, &type,
> +	err = vfs_path_parent_lookup(filename, flags, path, &last,
>  				     root_share_path);
>  	if (err)
>  		return err;
>  
> -	if (unlikely(type != LAST_NORM)) {
> -		path_put(path);
> -		return -ENOENT;
> -	}
> -
>  	if (for_remove) {
>  		err = mnt_want_write(path->mnt);
>  		if (err) {
> @@ -668,7 +661,6 @@ int ksmbd_vfs_rename(struct ksmbd_work *work, const struct path *old_path,
>  	struct renamedata rd;
>  	struct ksmbd_share_config *share_conf = work->tcon->share_conf;
>  	struct ksmbd_file *parent_fp;
> -	int new_type;
>  	int err, lookup_flags = LOOKUP_NO_SYMLINKS;
>  
>  	if (ksmbd_override_fsids(work))
> @@ -678,8 +670,7 @@ int ksmbd_vfs_rename(struct ksmbd_work *work, const struct path *old_path,
>  
>  retry:
>  	err = vfs_path_parent_lookup(to, lookup_flags | LOOKUP_BENEATH,
> -				     &new_path, &new_last, &new_type,
> -				     &share_conf->vfs_path);
> +				     &new_path, &new_last, &share_conf->vfs_path);
>  	if (err)
>  		goto out1;
>  
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index 2ad6dd9987b9..3941b9f1dec7 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -13,11 +13,6 @@ enum { MAX_NESTED_LINKS = 8 };
>  
>  #define MAXSYMLINKS 40
>  
> -/*
> - * Type of the last component on LOOKUP_PARENT
> - */
> -enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT};
> -
>  /* pathwalk mode */
>  #define LOOKUP_FOLLOW		BIT(0)	/* follow links at the end */
>  #define LOOKUP_DIRECTORY	BIT(1)	/* require a directory */
> @@ -67,7 +62,7 @@ static inline void end_removing_path(const struct path *path , struct dentry *de
>  	end_creating_path(path, dentry);
>  }
>  int vfs_path_parent_lookup(struct filename *filename, unsigned int flags,
> -			   struct path *parent, struct qstr *last, int *type,
> +			   struct path *parent, struct qstr *last,
>  			   const struct path *root);
>  int vfs_path_lookup(struct dentry *, struct vfsmount *, const char *,
>  		    unsigned int, struct path *);
> -- 
> 2.54.0
> 
>