Re: [PATCH 2/7] cifs: Improve creating native symlinks pointing to directory

[Paulo Alcantara <pc@manguebit.com>]

Pali Rohár <pali@kernel.org> writes:

> SMB protocol for native symlinks distinguish between symlink to directory
> and symlink to file. These two symlink types cannot be exchanged, which
> means that symlink of file type pointing to directory cannot be resolved at
> all (and vice-versa).
>
> Windows follows this rule for local filesystems (NTFS) and also for SMB.
>
> Linux SMB client currenly creates all native symlinks of file type. Which
> means that Windows (and some other SMB clients) cannot resolve symlinks
> pointing to directory created by Linux SMB client.
>
> As Linux system does not distinguish between directory and file symlinks,
> its API does not provide enough information for Linux SMB client during
> creating of native symlinks.
>
> Add some heuristic into the Linux SMB client for choosing the correct
> symlink type during symlink creation. Check if the symlink target location
> ends with slash, or last path component is dot or dot dot, and check if the
> target location on SMB share exists and is a directory. If at least one
> condition is truth then create a new SMB symlink of directory type.
> Otherwise create it as file type symlink.
>
> This change improves interoperability with Windows systems. Windows systems
> would be able to resolve more SMB symlinks created by Linux SMB client
> which points to existing directory.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  fs/smb/client/reparse.c   | 131 ++++++++++++++++++++++++++++++++++++--
>  fs/smb/client/smb2inode.c |   3 +-
>  fs/smb/client/smb2proto.h |   1 +
>  3 files changed, 130 insertions(+), 5 deletions(-)
>
> diff --git a/fs/smb/client/reparse.c b/fs/smb/client/reparse.c
> index 507e17244ed3..9390ab801696 100644
> --- a/fs/smb/client/reparse.c
> +++ b/fs/smb/client/reparse.c
> @@ -24,13 +24,16 @@ int smb2_create_reparse_symlink(const unsigned int xid, struct inode *inode,
>  	struct inode *new;
>  	struct kvec iov;
>  	__le16 *path;
> +	bool directory = false;
>  	char *sym, sep = CIFS_DIR_SEP(cifs_sb);
>  	u16 len, plen;
>  	int rc = 0;
>  
> -	sym = kstrdup(symname, GFP_KERNEL);
> +	len = strlen(symname)+1;
> +	sym = kzalloc(len+1, GFP_KERNEL); /* +1 for possible directory slash */
>  	if (!sym)
>  		return -ENOMEM;
> +	memcpy(sym, symname, len);
>  
>  	data = (struct cifs_open_info_data) {
>  		.reparse_point = true,
> @@ -45,6 +48,125 @@ int smb2_create_reparse_symlink(const unsigned int xid, struct inode *inode,
>  		goto out;
>  	}
>  
> +	/*
> +	 * SMB distinguish between symlink to directory and symlink to file.
> +	 * They cannot be exchanged (symlink of file type which points to
> +	 * directory cannot be resolved and vice-versa). First do some simple
> +	 * check, if the original Linux symlink target ends with slash, or
> +	 * last path component is dot or dot dot then it is for sure symlink
> +	 * to the directory.
> +	 */
> +	if (!directory) {
> +		const char *basename = kbasename(symname);
> +		int basename_len = strlen(basename);
> +		if (basename_len == 0 || /* symname ends with slash */
> +		    (basename_len == 1 && basename[0] == '.') || /* last component is "." */
> +		    (basename_len == 2 && basename[0] == '.' && basename[1] == '.')) /* last component is ".." */
> +			directory = true;
> +	}
> +
> +	/*
> +	 * If it was not detected as directory yet and the symlink is relative
> +	 * then try to resolve the path on the SMB server, check if the path
> +	 * exists and determinate if it is a directory or not.
> +	 */
> +	if (!directory && symname[0] != '/') {
> +		__u32 oplock;
> +		struct tcon_link *tlink;
> +		struct cifs_tcon *tcon;
> +		struct cifs_fid fid;
> +		struct cifs_open_parms oparms;
> +		char *resolved_path;
> +		char *path_sep;
> +		int open_rc;
> +		int full_path_len = strlen(full_path);
> +		int symname_len = strlen(symname);
> +
> +		tlink = cifs_sb_tlink(cifs_sb);
> +		if (IS_ERR(tlink)) {
> +			rc = PTR_ERR(tlink);
> +			goto out;
> +		}
> +
> +		resolved_path = kzalloc(full_path_len + symname_len + 1, GFP_KERNEL);
> +		if (!resolved_path) {
> +			rc = -ENOMEM;
> +			goto out;
> +		}

If !@resolved_path, then you will end up leaking @tlink.

> +
> +		/*
> +		 * Compose the resolved SMB symlink path from the SMB full path
> +		 * and Linux target symlink path.
> +		 */
> +		memcpy(resolved_path, full_path, full_path_len+1);
> +		path_sep = strrchr(resolved_path, sep);
> +		if (path_sep)
> +			path_sep++;
> +		else
> +			path_sep = resolved_path;
> +		memcpy(path_sep, symname, symname_len+1);
> +		if (sep == '\\')
> +			convert_delimiter(path_sep, sep);
> +
> +		tcon = tlink_tcon(tlink);
> +
> +		oparms = (struct cifs_open_parms) {
> +			.tcon = tcon,
> +			.cifs_sb = cifs_sb,
> +			.desired_access = FILE_READ_ATTRIBUTES,
> +			.disposition = FILE_OPEN,
> +			.path = resolved_path,
> +			.fid = &fid,
> +		};

Please use CIFS_OPARMS().

> +
> +		/* Try to open as NOT_FILE */
> +		oplock = 0;
> +		oparms.create_options = cifs_create_options(cifs_sb, CREATE_NOT_FILE);
> +		open_rc = tcon->ses->server->ops->open(xid, &oparms, &oplock, NULL);
> +		if (open_rc == 0) {
> +			/* Successful open means that the target path is definitely a directory. */
> +			directory = true;
> +			tcon->ses->server->ops->close(xid, tcon, &fid);
> +		} else if (open_rc != -ENOTDIR) {
> +			/* Try to open as NOT_DIR */
> +			oplock = 0;
> +			oparms.create_options = cifs_create_options(cifs_sb, CREATE_NOT_DIR);
> +			open_rc = tcon->ses->server->ops->open(xid, &oparms, &oplock, NULL);
> +			if (open_rc == 0) {
> +				tcon->ses->server->ops->close(xid, tcon, &fid);
> +			} else if (open_rc == -EISDIR) {
> +				/* -EISDIR means that the target path is definitely a directory. */
> +				directory = true;
> +			} else {
> +				cifs_dbg(FYI,
> +					 "%s: cannot determinate if the symlink target path '%s' "
> +					 "is directory or not, creating '%s' as file symlink\n",
> +					 __func__, symname, full_path);
> +			}
> +		}
> +
> +		kfree(resolved_path);
> +		cifs_put_tlink(tlink);
> +	}
> +
> +	/*
> +	 * For absolute symlinks it is not possible to determinate
> +	 * if it should point to directory or file.
> +	 */
> +	if (!directory && symname[0] == '/')
> +		cifs_dbg(FYI,
> +			 "%s: cannot determinate if the symlink target path '%s' "
> +			 "is directory or not, creating '%s' as file symlink\n",
> +			 __func__, symname, full_path);
> +

Create a helper with all of this and then call it in
smb2_create_reparse_symlink() to determine whether symlink target is a
directory or file.