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

["Pali Rohár" <pali@kernel.org>]

On Monday 30 September 2024 12:09:48 Paulo Alcantara wrote:
> 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.

Hello, thanks for review! Of course I will update this patch to address
all your points.