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

[Pali Rohár]

On Monday 30 September 2024 19:17:15 Pali Rohár wrote:
> On Monday 30 September 2024 12:09:48 Paulo Alcantara wrote:
> > Pali Rohár <pali@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > >  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.

Hello, I addresses this issues in V2 of the patch series. Please look at it.