Re: [RFC][PATCHSET] hopefully saner handling of pathnames in cifs

[Steve French <smfrench@gmail.com>]

WIll run the automated tests on these.

Also FYI - patches 2 and 6 had some checkpatch warnings (although fairly minor).

On Fri, Mar 19, 2021 at 11:36 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
>         Patch series (#work.cifs in vfs.git) tries to clean the things
> up in and around build_path_from_dentry().  Part of that is constifying
> the pointers around that stuff, then it lifts the allocations into
> callers and finally switches build_path_from_dentry() to using
> dentry_path_raw() instead of open-coding it.  Handling of ->d_name
> and friends is subtle enough, and it would be better to have fewer
> places besides fs/d_path.c that need to mess with those...
>
>         Help with review and testing would be very much appreciated -
> there's a plenty of mount options/server combinations ;-/
>
>         For those who prefer to look at it in git, it lives in
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #work.cifs;
> individual patches go in followups.
>
> Shortlog:
> Al Viro (7):
>       cifs: don't cargo-cult strndup()
>       cifs: constify get_normalized_path() properly
>       cifs: constify path argument of ->make_node()
>       cifs: constify pathname arguments in a bunch of helpers
>       cifs: make build_path_from_dentry() return const char *
>       cifs: allocate buffer in the caller of build_path_from_dentry()
>       cifs: switch build_path_from_dentry() to using dentry_path_raw()
>
> 1) a bunch of kstrdup() calls got cargo-culted as kstrndup().
> This is unidiomatic *and* pointless - it's not any "safer"
> that way (pass it a non-NUL-terminated array, and strlen()
> will barf same as kstrdup()) and it's actually a pessimization.
> Converted to plain kstrdup() calls.
>
> 2) constifying pathnames: get_normalized_path() gets a
> constant string and, on success, returns either that string
> or its modified copy.  It is declared with the wrong prototype -
> int get_normalized_path(const char *path, char **npath)
> so the caller might get a non-const alias of the original const
> string.  Fortunately, none of the callers actually use that
> alias to modify the string, so it's not an active bug - just
> the wrong typization.
>
> 3) constifying pathnames: ->make_node().  Unlike the rest of
> methods that take pathname as an argument, it has that argument
> declared as char *, not const char *.  Pure misannotation,
> since all instances never modify that actual string (or pass it
> to anything that might do the same).
>
> 4) constifying pathnames: a bunch of helpers.  Several functions
> have pathname argument declared as char *, when const char *
> would be fine - they neither modify the string nor pass it to
> anything that might.
>
> 5) constifying pathnames: build_path_from_dentry().
> That's the main source of pathnames; all callers are actually
> treating the string it returns as constant one.  Declare it
> to return const char * and adjust the callers.
>
> 6) take buffer allocation out of build_path_from_dentry().
> Trying to do exact-sized allocation is pointless - allocated
> object are short-lived anyway (the caller is always the one
> to free the string it gets from build_path_from_dentry()).
> As the matter of fact, we are in the same situation as with
> pathname arguments of syscalls - short-lived allocations
> limited to 4Kb and freed before the caller returns to userland.
> So we can just do allocations from names_cachep and do that
> in the caller; that way we don't need to bother with GFP_ATOMIC
> allocations.  Moreover, having the caller do allocations will
> permit us to switch build_path_from_dentry() to use of dentry_path_raw()
> (in the next commit).
>
> 7) build_path_from_dentry() essentially open-codes dentry_path_raw();
> the difference is that it wants to put the result in the beginning
> of the buffer (which we don't need anymore, since the caller knows
> what to free anyway) _and_ we might want '\\' for component separator
> instead of the normal '/'.  It's easier to use dentry_path_raw()
> and (optionally) post-process the result, replacing all '/' with
> '\\'.  Note that the last part needs profiling - I would expect it
> to be noise (we have just formed the string and it's all in hot cache),
> but that needs to be verified.
>
> Diffstat:
>  fs/cifs/cifs_dfs_ref.c |  14 +++--
>  fs/cifs/cifsglob.h     |   2 +-
>  fs/cifs/cifsproto.h    |  19 +++++--
>  fs/cifs/connect.c      |   9 +--
>  fs/cifs/dfs_cache.c    |  41 +++++++-------
>  fs/cifs/dir.c          | 148 ++++++++++++++++++-------------------------------
>  fs/cifs/file.c         |  79 +++++++++++++-------------
>  fs/cifs/fs_context.c   |   2 +-
>  fs/cifs/inode.c        | 110 ++++++++++++++++++------------------
>  fs/cifs/ioctl.c        |  13 +++--
>  fs/cifs/link.c         |  46 +++++++++------
>  fs/cifs/misc.c         |   2 +-
>  fs/cifs/readdir.c      |  15 ++---
>  fs/cifs/smb1ops.c      |   6 +-
>  fs/cifs/smb2ops.c      |  19 ++++---
>  fs/cifs/unc.c          |   4 +-
>  fs/cifs/xattr.c        |  40 +++++++------
>  17 files changed, 278 insertions(+), 291 deletions(-)



-- 
Thanks,

Steve