Currently Linux SMB client returns EIO for lstat() and AT_SYMLINK_NOFOLLOW
calls on symlink node when the symlink target location is broken or cannot
be read or parsed.

Fix this problem by relaxing the errors from various locations which parses
information about symlink file node (UNIX SMB1, native SMB2+, NFS-style,
WSL-style) and let readlink() syscall to return EIO when the symlink target
location is not available.

Note that SFU symlinks and MF symlinks are not affected by this issue,
their parser has already relaxed code.

This change fixes the 'ls -l -a' call on directory which has symlink nodes
with broken target locations.

Reported-by: Remy Monsen <monsen@monsen.cc>
Signed-off-by: Pali Rohár <pali@kernel.org>
---
 fs/smb/client/cifsfs.c   |  3 +-
 fs/smb/client/inode.c    | 24 ++++++++++++++++
 fs/smb/client/reparse.c  | 61 ++++++++++++++++++++++++++++++++--------
 fs/smb/client/smb2file.c | 10 ++++---
 4 files changed, 81 insertions(+), 17 deletions(-)

diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
index a08c42363ffc..f4b923f73dca 100644
--- a/fs/smb/client/cifsfs.c
+++ b/fs/smb/client/cifsfs.c
@@ -1180,7 +1180,8 @@ const char *cifs_get_link(struct dentry *dentry, struct inode *inode,
 		strscpy(target_path, CIFS_I(inode)->symlink_target, PATH_MAX);
 	} else {
 		kfree(target_path);
-		target_path = ERR_PTR(-EOPNOTSUPP);
+		/* If symlink_target is not filled for symlink then it is an IO error. */
+		target_path = ERR_PTR(-EIO);
 	}
 	spin_unlock(&inode->i_lock);
 
diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
index b1c6e3986278..762cd194946a 100644
--- a/fs/smb/client/inode.c
+++ b/fs/smb/client/inode.c
@@ -480,6 +480,12 @@ static int cifs_get_unix_fattr(const unsigned char *full_path,
 						cifs_sb, full_path,
 						&fattr->cf_symlink_target);
 		cifs_dbg(FYI, "%s: query_symlink: %d\n", __func__, rc);
+		/*
+		 * Convert -EIO to 0. This let lstat() success and
+		 * empty data->cf_symlink_target triggers readlink() to fail with -EIO.
+		 */
+		if (rc == -EIO)
+			rc = 0;
 	}
 	return rc;
 }
@@ -1133,6 +1139,12 @@ static int reparse_info_to_fattr(struct cifs_open_info_data *data,
 			rc = server->ops->query_symlink(xid, tcon,
 							cifs_sb, full_path,
 							&data->symlink_target);
+			/*
+			 * Convert -EIO to 0. This let lstat() success and
+			 * empty data->symlink_target triggers readlink() to fail with -EIO.
+			 */
+			if (rc == -EIO)
+				rc = 0;
 		}
 		if (rc == -EOPNOTSUPP)
 			data->reparse.tag = IO_REPARSE_TAG_INTERNAL;
@@ -1182,6 +1194,18 @@ static int reparse_info_to_fattr(struct cifs_open_info_data *data,
 			 */
 			if (rc == -EOPNOTSUPP)
 				rc = 0;
+		} else if (data->reparse.tag == IO_REPARSE_TAG_SYMLINK) {
+			/*
+			 * data->reparse.tag can be set to IO_REPARSE_TAG_SYMLINK
+			 * by STATUS_STOPPED_ON_SYMLINK error code. In this case
+			 * we do not have a real reparse point iov buffer so
+			 * data->reparse.buf and data->reparse.io.iov.iov_base
+			 * are not set. And in the case symlink target location
+			 * in the struct smb2_symlink_err_rsp is parsable then we
+			 * even do not have data->symlink_target. So set rc to 0
+			 * which let lstat() success and readlink() to fail.
+			 */
+			rc = 0;
 		}
 
 		if (data->reparse.tag == IO_REPARSE_TAG_SYMLINK && !rc) {
diff --git a/fs/smb/client/reparse.c b/fs/smb/client/reparse.c
index d1d25f5f72ca..c70affb7b7f7 100644
--- a/fs/smb/client/reparse.c
+++ b/fs/smb/client/reparse.c
@@ -739,7 +739,11 @@ static int parse_reparse_nfs(struct reparse_nfs_data_buffer *buf,
 	case NFS_SPECFILE_LNK:
 		if (len == 0 || (len % 2)) {
 			cifs_dbg(VFS, "srv returned malformed nfs symlink buffer\n");
-			return -EIO;
+			/*
+			 * This is an -EIO error. Convert it to 0. This let lstat() success and
+			 * empty data->symlink_target triggers readlink() to fail with -EIO.
+			 */
+			return 0;
 		}
 		/*
 		 * Check that buffer does not contain UTF-16 null codepoint
@@ -747,7 +751,11 @@ static int parse_reparse_nfs(struct reparse_nfs_data_buffer *buf,
 		 */
 		if (UniStrnlen((wchar_t *)buf->DataBuffer, len/2) != len/2) {
 			cifs_dbg(VFS, "srv returned null byte in nfs symlink target location\n");
-			return -EIO;
+			/*
+			 * This is an -EIO error. Convert it to 0. This let lstat() success and
+			 * empty data->symlink_target triggers readlink() to fail with -EIO.
+			 */
+			return 0;
 		}
 		data->symlink_target = cifs_strndup_from_utf16(buf->DataBuffer,
 							       len, true,
@@ -986,6 +994,14 @@ int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len,
 	if (rc != 0)
 		kfree(linux_target);
 	kfree(smb_target);
+
+	/*
+	* Convert -EIO to 0. This let lstat() success and
+	* empty *target triggers readlink() to fail with -EIO.
+	*/
+	if (rc == -EIO)
+		rc = 0;
+
 	return rc;
 }
 
@@ -1004,7 +1020,11 @@ static int parse_reparse_native_symlink(struct reparse_symlink_data_buffer *sym,
 	len = le16_to_cpu(sym->SubstituteNameLength);
 	if (offs + 20 > plen || offs + len + 20 > plen) {
 		cifs_dbg(VFS, "srv returned malformed symlink buffer\n");
-		return -EIO;
+		/*
+		 * This is an -EIO error. Convert it to 0. This let lstat() success and
+		 * empty data->symlink_target triggers readlink() to fail with -EIO.
+		 */
+		return 0;
 	}
 
 	return smb2_parse_native_symlink(&data->symlink_target,
@@ -1024,16 +1044,19 @@ static int parse_reparse_wsl_symlink(struct reparse_wsl_symlink_data_buffer *buf
 	int symname_utf8_len;
 	__le16 *symname_utf16;
 	int symname_utf16_len;
+	int rc = 0;
 
 	if (len <= data_offset) {
 		cifs_dbg(VFS, "srv returned malformed wsl symlink buffer\n");
-		return -EIO;
+		rc = -EIO;
+		goto out;
 	}
 
 	/* MS-FSCC 2.1.2.7 defines layout of the Target field only for Version 2. */
 	if (le32_to_cpu(buf->Version) != 2) {
 		cifs_dbg(VFS, "srv returned unsupported wsl symlink version %u\n", le32_to_cpu(buf->Version));
-		return -EIO;
+		rc = -EIO;
+		goto out;
 	}
 
 	/* Target for Version 2 is in UTF-8 but without trailing null-term byte */
@@ -1044,17 +1067,21 @@ static int parse_reparse_wsl_symlink(struct reparse_wsl_symlink_data_buffer *buf
 	 */
 	if (strnlen(buf->Target, symname_utf8_len) != symname_utf8_len) {
 		cifs_dbg(VFS, "srv returned null byte in wsl symlink target location\n");
-		return -EIO;
+		rc = -EIO;
+		goto out;
 	}
 	symname_utf16 = kzalloc(symname_utf8_len * 2, GFP_KERNEL);
-	if (!symname_utf16)
-		return -ENOMEM;
+	if (!symname_utf16) {
+		rc = -ENOMEM;
+		goto out;
+	}
 	symname_utf16_len = utf8s_to_utf16s(buf->Target, symname_utf8_len,
 					    UTF16_LITTLE_ENDIAN,
 					    (wchar_t *) symname_utf16, symname_utf8_len * 2);
 	if (symname_utf16_len < 0) {
 		kfree(symname_utf16);
-		return symname_utf16_len;
+		rc = symname_utf16_len;
+		goto out;
 	}
 	symname_utf16_len *= 2; /* utf8s_to_utf16s() returns number of u16 items, not byte length */
 
@@ -1062,10 +1089,20 @@ static int parse_reparse_wsl_symlink(struct reparse_wsl_symlink_data_buffer *buf
 						       symname_utf16_len, true,
 						       cifs_sb->local_nls);
 	kfree(symname_utf16);
-	if (!data->symlink_target)
-		return -ENOMEM;
+	if (!data->symlink_target) {
+		rc = -ENOMEM;
+		goto out;
+	}
 
-	return 0;
+out:
+	/*
+	* Convert -EIO to 0. This let lstat() success and
+	* empty data->symlink_target triggers readlink() to fail with -EIO.
+	*/
+	if (rc == -EIO)
+		rc = 0;
+
+	return rc;
 }
 
 int parse_reparse_point(struct reparse_data_buffer *buf,
diff --git a/fs/smb/client/smb2file.c b/fs/smb/client/smb2file.c
index a7f629238830..9ac359f7be43 100644
--- a/fs/smb/client/smb2file.c
+++ b/fs/smb/client/smb2file.c
@@ -76,11 +76,11 @@ int smb2_fix_symlink_target_type(char **target, bool directory, struct cifs_sb_i
 		return 0;
 
 	if (!*target)
-		return -EIO;
+		return 0;
 
 	len = strlen(*target);
 	if (!len)
-		return -EIO;
+		return 0;
 
 	/*
 	 * If this is directory symlink and it does not have trailing slash then
@@ -103,8 +103,10 @@ int smb2_fix_symlink_target_type(char **target, bool directory, struct cifs_sb_i
 	 * cannot contain slash character. File name with slash is invalid on
 	 * both Windows and Linux systems. So return an error for such symlink.
 	 */
-	if (!directory && (*target)[len-1] == '/')
-		return -EIO;
+	if (!directory && (*target)[len-1] == '/') {
+		kfree(*target);
+		*target = NULL;
+	}
 
 	return 0;
 }
-- 
2.20.1

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

> Currently Linux SMB client returns EIO for lstat() and AT_SYMLINK_NOFOLLOW
> calls on symlink node when the symlink target location is broken or cannot
> be read or parsed.
>
> Fix this problem by relaxing the errors from various locations which parses
> information about symlink file node (UNIX SMB1, native SMB2+, NFS-style,
> WSL-style) and let readlink() syscall to return EIO when the symlink target
> location is not available.

Please, don't.  We still want those validations for the other types of
symlinks.  The problem is just that cifs.ko can't handle absolute
symlink targets in the form of '\??\UNC\srv\share\foo', while Windows
client can.  They are still valid symlink targets, but cifs.ko doesn't
know how to follow them.

The following should do it and then restore old behavior

diff --git a/fs/smb/client/reparse.c b/fs/smb/client/reparse.c
index bb25e77c5540..11d44288e75a 100644
--- a/fs/smb/client/reparse.c
+++ b/fs/smb/client/reparse.c
@@ -875,15 +875,8 @@ int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len,
 			abs_path += sizeof("\\DosDevices\\")-1;
 		else if (strstarts(abs_path, "\\GLOBAL??\\"))
 			abs_path += sizeof("\\GLOBAL??\\")-1;
-		else {
-			/* Unhandled absolute symlink, points outside of DOS/Win32 */
-			cifs_dbg(VFS,
-				 "absolute symlink '%s' cannot be converted from NT format "
-				 "because points to unknown target\n",
-				 smb_target);
-			rc = -EIO;
-			goto out;
-		}
+		else
+			goto out_unhandled_target;
 
 		/* Sometimes path separator after \?? is double backslash */
 		if (abs_path[0] == '\\')
@@ -910,13 +903,7 @@ int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len,
 			abs_path++;
 			abs_path[0] = drive_letter;
 		} else {
-			/* Unhandled absolute symlink. Report an error. */
-			cifs_dbg(VFS,
-				 "absolute symlink '%s' cannot be converted from NT format "
-				 "because points to unknown target\n",
-				 smb_target);
-			rc = -EIO;
-			goto out;
+			goto out_unhandled_target;
 		}
 
 		abs_path_len = strlen(abs_path)+1;
@@ -966,6 +953,7 @@ int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len,
 		 * These paths have same format as Linux symlinks, so no
 		 * conversion is needed.
 		 */
+out_unhandled_target:
 		linux_target = smb_target;
 		smb_target = NULL;
 	}

On Friday 20 June 2025 20:44:37 Paulo Alcantara wrote:
> Pali Rohár <pali@kernel.org> writes:
> 
> > Currently Linux SMB client returns EIO for lstat() and AT_SYMLINK_NOFOLLOW
> > calls on symlink node when the symlink target location is broken or cannot
> > be read or parsed.
> >
> > Fix this problem by relaxing the errors from various locations which parses
> > information about symlink file node (UNIX SMB1, native SMB2+, NFS-style,
> > WSL-style) and let readlink() syscall to return EIO when the symlink target
> > location is not available.
> 
> Please, don't.  We still want those validations for the other types of
> symlinks.

Well, validation was not removed. Validation is still there, just the
error is signalled by the readlink() syscall instead of the lstat() or
AT_SYMLINK_NOFOLLOW syscalls.

My opinion is that the lstat() or AT_SYMLINK_NOFOLLOW should work on
symlink node independently of where the symlink points (and whether the
symlink target is valid POSIX path or not). That is because the lstat()
and AT_SYMLINK_NOFOLLOW says that the symlink target location must not
be used and must not be resolved.

But still the invalid / incorrect / broken or non-representable symlink
target path in POSIX notation should be reported as an issue and the
readlink() is the correct syscall which should report these errors.

> The problem is just that cifs.ko can't handle absolute
> symlink targets in the form of '\??\UNC\srv\share\foo', while Windows
> client can.  They are still valid symlink targets, but cifs.ko doesn't
> know how to follow them.

Windows client can represent and follow such symlink because the symlink
is in the NT style format and Windows kernel uses NT style of paths
internally. Linux kernel uses POSIX paths and POSIX does not contain any
GLOBAL?? namespace for NT object hierarchy.

Leaking raw NT object hierarchy from SMB to POSIX userspace via
readlink() syscall is a bad idea. Applications are really not expecting
that the readlink() syscall will return NT kernel internals (exported
over SMB protocol and passed to cifs.ko).

For UNC paths encoded in NT object hierarchy, which is just some subset
of all possible NT paths, I had an idea that we could convert these
paths to some format like:

   <prefix>/server/share/path...

Where <prefix> would be specified by the string mount option. So user
could say that wants all UNC symlinks pointing to /mnt/unc/.

And in the same way if user would want to create symlink pointing to
/mnt/unc/server/share/path... then cifs.ko will transform it into valid
NT UNC path and create a symlink to this location.

But this would solve only problem with UNC symlink, not symlinks
pointing to NT object hierarchy in general.

> The following should do it and then restore old behavior
> 
> diff --git a/fs/smb/client/reparse.c b/fs/smb/client/reparse.c
> index bb25e77c5540..11d44288e75a 100644
> --- a/fs/smb/client/reparse.c
> +++ b/fs/smb/client/reparse.c
> @@ -875,15 +875,8 @@ int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len,
>  			abs_path += sizeof("\\DosDevices\\")-1;
>  		else if (strstarts(abs_path, "\\GLOBAL??\\"))
>  			abs_path += sizeof("\\GLOBAL??\\")-1;
> -		else {
> -			/* Unhandled absolute symlink, points outside of DOS/Win32 */
> -			cifs_dbg(VFS,
> -				 "absolute symlink '%s' cannot be converted from NT format "
> -				 "because points to unknown target\n",
> -				 smb_target);
> -			rc = -EIO;
> -			goto out;
> -		}
> +		else
> +			goto out_unhandled_target;
>  
>  		/* Sometimes path separator after \?? is double backslash */
>  		if (abs_path[0] == '\\')
> @@ -910,13 +903,7 @@ int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len,
>  			abs_path++;
>  			abs_path[0] = drive_letter;
>  		} else {
> -			/* Unhandled absolute symlink. Report an error. */
> -			cifs_dbg(VFS,
> -				 "absolute symlink '%s' cannot be converted from NT format "
> -				 "because points to unknown target\n",
> -				 smb_target);
> -			rc = -EIO;
> -			goto out;
> +			goto out_unhandled_target;
>  		}
>  
>  		abs_path_len = strlen(abs_path)+1;
> @@ -966,6 +953,7 @@ int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len,
>  		 * These paths have same format as Linux symlinks, so no
>  		 * conversion is needed.
>  		 */
> +out_unhandled_target:
>  		linux_target = smb_target;
>  		smb_target = NULL;
>  	}

I'm really not sure if removing the messages and error reporting about
symlinks which cannot be represented in POSIX system is a good idea.

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

> On Friday 20 June 2025 20:44:37 Paulo Alcantara wrote:
>> Pali Rohár <pali@kernel.org> writes:
>> 
>> > Currently Linux SMB client returns EIO for lstat() and AT_SYMLINK_NOFOLLOW
>> > calls on symlink node when the symlink target location is broken or cannot
>> > be read or parsed.
>> >
>> > Fix this problem by relaxing the errors from various locations which parses
>> > information about symlink file node (UNIX SMB1, native SMB2+, NFS-style,
>> > WSL-style) and let readlink() syscall to return EIO when the symlink target
>> > location is not available.
>> 
>> Please, don't.  We still want those validations for the other types of
>> symlinks.
>
> Well, validation was not removed. Validation is still there, just the
> error is signalled by the readlink() syscall instead of the lstat() or
> AT_SYMLINK_NOFOLLOW syscalls.
>
> My opinion is that the lstat() or AT_SYMLINK_NOFOLLOW should work on
> symlink node independently of where the symlink points (and whether the
> symlink target is valid POSIX path or not). That is because the lstat()
> and AT_SYMLINK_NOFOLLOW says that the symlink target location must not
> be used and must not be resolved.
>
> But still the invalid / incorrect / broken or non-representable symlink
> target path in POSIX notation should be reported as an issue and the
> readlink() is the correct syscall which should report these errors.

The only issue is breaking existing customer or user applications that
really don't care if cifs.ko could follow those kind of symlinks.

Samba create symlinks to represent DFS links with targets like
'msdfs:srv1\share,srv2\share', which are not valid POSIX paths.  Does
that mean the filesystem should not allow readlink(2) to succeed just
because it is not a valid POSIX path?  Is that what you mean?

>> The problem is just that cifs.ko can't handle absolute
>> symlink targets in the form of '\??\UNC\srv\share\foo', while Windows
>> client can.  They are still valid symlink targets, but cifs.ko doesn't
>> know how to follow them.
>
> Windows client can represent and follow such symlink because the symlink
> is in the NT style format and Windows kernel uses NT style of paths
> internally. Linux kernel uses POSIX paths and POSIX does not contain any
> GLOBAL?? namespace for NT object hierarchy.
>
> Leaking raw NT object hierarchy from SMB to POSIX userspace via
> readlink() syscall is a bad idea. Applications are really not expecting
> that the readlink() syscall will return NT kernel internals (exported
> over SMB protocol and passed to cifs.ko).
>
> For UNC paths encoded in NT object hierarchy, which is just some subset
> of all possible NT paths, I had an idea that we could convert these
> paths to some format like:
>
>    <prefix>/server/share/path...
>
> Where <prefix> would be specified by the string mount option. So user
> could say that wants all UNC symlinks pointing to /mnt/unc/.
>
> And in the same way if user would want to create symlink pointing to
> /mnt/unc/server/share/path... then cifs.ko will transform it into valid
> NT UNC path and create a symlink to this location.

That's really a terrible idea.  The symlink targets in the form of
'\??\UNC\...' could be resolved by cifs.ko.  The ones that refer to a
file outside the mounted share, we would set those as automounts.

> But this would solve only problem with UNC symlink, not symlinks
> pointing to NT object hierarchy in general.
>
>> The following should do it and then restore old behavior
>> 
>> diff --git a/fs/smb/client/reparse.c b/fs/smb/client/reparse.c
>> index bb25e77c5540..11d44288e75a 100644
>> --- a/fs/smb/client/reparse.c
>> +++ b/fs/smb/client/reparse.c
>> @@ -875,15 +875,8 @@ int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len,
>>  			abs_path += sizeof("\\DosDevices\\")-1;
>>  		else if (strstarts(abs_path, "\\GLOBAL??\\"))
>>  			abs_path += sizeof("\\GLOBAL??\\")-1;
>> -		else {
>> -			/* Unhandled absolute symlink, points outside of DOS/Win32 */
>> -			cifs_dbg(VFS,
>> -				 "absolute symlink '%s' cannot be converted from NT format "
>> -				 "because points to unknown target\n",
>> -				 smb_target);
>> -			rc = -EIO;
>> -			goto out;
>> -		}
>> +		else
>> +			goto out_unhandled_target;
>>  
>>  		/* Sometimes path separator after \?? is double backslash */
>>  		if (abs_path[0] == '\\')
>> @@ -910,13 +903,7 @@ int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len,
>>  			abs_path++;
>>  			abs_path[0] = drive_letter;
>>  		} else {
>> -			/* Unhandled absolute symlink. Report an error. */
>> -			cifs_dbg(VFS,
>> -				 "absolute symlink '%s' cannot be converted from NT format "
>> -				 "because points to unknown target\n",
>> -				 smb_target);
>> -			rc = -EIO;
>> -			goto out;
>> +			goto out_unhandled_target;
>>  		}
>>  
>>  		abs_path_len = strlen(abs_path)+1;
>> @@ -966,6 +953,7 @@ int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len,
>>  		 * These paths have same format as Linux symlinks, so no
>>  		 * conversion is needed.
>>  		 */
>> +out_unhandled_target:
>>  		linux_target = smb_target;
>>  		smb_target = NULL;
>>  	}
>
> I'm really not sure if removing the messages and error reporting about
> symlinks which cannot be represented in POSIX system is a good idea.

Those messages are just useless and noisy.  Do you think it's useful
printing that message for _every_ symlink when someone is calling
readdir(2) in a directory that contain such files?

On Saturday 21 June 2025 12:38:34 Paulo Alcantara wrote:
> Pali Rohár <pali@kernel.org> writes:
> 
> > On Friday 20 June 2025 20:44:37 Paulo Alcantara wrote:
> >> Pali Rohár <pali@kernel.org> writes:
> >> 
> >> > Currently Linux SMB client returns EIO for lstat() and AT_SYMLINK_NOFOLLOW
> >> > calls on symlink node when the symlink target location is broken or cannot
> >> > be read or parsed.
> >> >
> >> > Fix this problem by relaxing the errors from various locations which parses
> >> > information about symlink file node (UNIX SMB1, native SMB2+, NFS-style,
> >> > WSL-style) and let readlink() syscall to return EIO when the symlink target
> >> > location is not available.
> >> 
> >> Please, don't.  We still want those validations for the other types of
> >> symlinks.
> >
> > Well, validation was not removed. Validation is still there, just the
> > error is signalled by the readlink() syscall instead of the lstat() or
> > AT_SYMLINK_NOFOLLOW syscalls.
> >
> > My opinion is that the lstat() or AT_SYMLINK_NOFOLLOW should work on
> > symlink node independently of where the symlink points (and whether the
> > symlink target is valid POSIX path or not). That is because the lstat()
> > and AT_SYMLINK_NOFOLLOW says that the symlink target location must not
> > be used and must not be resolved.
> >
> > But still the invalid / incorrect / broken or non-representable symlink
> > target path in POSIX notation should be reported as an issue and the
> > readlink() is the correct syscall which should report these errors.
> 
> The only issue is breaking existing customer or user applications that
> really don't care if cifs.ko could follow those kind of symlinks.
> 
> Samba create symlinks to represent DFS links with targets like
> 'msdfs:srv1\share,srv2\share', which are not valid POSIX paths.  Does
> that mean the filesystem should not allow readlink(2) to succeed just
> because it is not a valid POSIX path?  Is that what you mean?

But this is something totally different thing.

Here you are referring to the behavior of Samba server, which interprets
symlink node stored on local filesystem named e.g. "link1" pointing to
target relative file name 'msdfs:srv1\share,srv2\share' specially.

Calling "ln -s 'msdfs:srv1\share,srv2\share' link1" is perfectly fine on
the ext4 filesystem. It creates a relative symlink to the specified
file.

And if you call "echo test > 'msdfs:srv1\share,srv2\share'" then it would
world correctly and "cat link1" will print "test".

The 'msdfs:srv1\share,srv2\share' is a valid POSIX path and it is stored
on the local Linux filesystem. So I do not see anything wrong with it or
reason why local filesystem should disallow creating such symlink or why
would realink() should fail on such node.


That example has nothing with symlinks stored on NTFS-compatible
filesystems which has ability to store symlinks pointing to non-POSIX
NT object model paths.

Here the issue is with symlink target locations which are coming from
the remote NT server and are pointing to location which cannot be
directly represented by the Linux system. The translation needs to be
done in both directions and reversible. Otherwise moving the file or
symlink from cifs to ext4 and back would damage the file or symlink.

> >> The problem is just that cifs.ko can't handle absolute
> >> symlink targets in the form of '\??\UNC\srv\share\foo', while Windows
> >> client can.  They are still valid symlink targets, but cifs.ko doesn't
> >> know how to follow them.
> >
> > Windows client can represent and follow such symlink because the symlink
> > is in the NT style format and Windows kernel uses NT style of paths
> > internally. Linux kernel uses POSIX paths and POSIX does not contain any
> > GLOBAL?? namespace for NT object hierarchy.
> >
> > Leaking raw NT object hierarchy from SMB to POSIX userspace via
> > readlink() syscall is a bad idea. Applications are really not expecting
> > that the readlink() syscall will return NT kernel internals (exported
> > over SMB protocol and passed to cifs.ko).
> >
> > For UNC paths encoded in NT object hierarchy, which is just some subset
> > of all possible NT paths, I had an idea that we could convert these
> > paths to some format like:
> >
> >    <prefix>/server/share/path...
> >
> > Where <prefix> would be specified by the string mount option. So user
> > could say that wants all UNC symlinks pointing to /mnt/unc/.
> >
> > And in the same way if user would want to create symlink pointing to
> > /mnt/unc/server/share/path... then cifs.ko will transform it into valid
> > NT UNC path and create a symlink to this location.
> 
> That's really a terrible idea.  The symlink targets in the form of
> '\??\UNC\...' could be resolved by cifs.ko.  The ones that refer to a
> file outside the mounted share, we would set those as automounts.

I agree that above is not the best idea, but I wrote at least something
as an idea as I do not know how it could be solved in better way.

And I do not see how it could be resolved by cifs.ko somehow
automatically. I'm not sure to which you refer how it can be resolved by
cifs.ko. I understood you message as it could automount another share
and do the whole path symlink resolving in cifs.ko.

And I think that this is even worse idea than mine. Because that
automount means that symlinks pointing outside of the share would start
behaving like a mount point. Such thing can cause even a security issues
if not used carefully.

But moreover there is a big difference between symlink and mount point.
Symlinks are not resolved by filesystem itself (but rather by the VFS,
to ensure that all access checks are applied) and also moving the
symlink between filesystems should not break them. In this idea when the
symlink is going to be moved from smb share to e.g. ext4 local fs, then
it would stops working (if the path resolved is in the cifs.ko) as
ext4.ko would not be able to process special cifs.ko symlinks.

> > But this would solve only problem with UNC symlink, not symlinks
> > pointing to NT object hierarchy in general.
> >
> >> The following should do it and then restore old behavior
> >> 
> >> diff --git a/fs/smb/client/reparse.c b/fs/smb/client/reparse.c
> >> index bb25e77c5540..11d44288e75a 100644
> >> --- a/fs/smb/client/reparse.c
> >> +++ b/fs/smb/client/reparse.c
> >> @@ -875,15 +875,8 @@ int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len,
> >>  			abs_path += sizeof("\\DosDevices\\")-1;
> >>  		else if (strstarts(abs_path, "\\GLOBAL??\\"))
> >>  			abs_path += sizeof("\\GLOBAL??\\")-1;
> >> -		else {
> >> -			/* Unhandled absolute symlink, points outside of DOS/Win32 */
> >> -			cifs_dbg(VFS,
> >> -				 "absolute symlink '%s' cannot be converted from NT format "
> >> -				 "because points to unknown target\n",
> >> -				 smb_target);
> >> -			rc = -EIO;
> >> -			goto out;
> >> -		}
> >> +		else
> >> +			goto out_unhandled_target;
> >>  
> >>  		/* Sometimes path separator after \?? is double backslash */
> >>  		if (abs_path[0] == '\\')
> >> @@ -910,13 +903,7 @@ int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len,
> >>  			abs_path++;
> >>  			abs_path[0] = drive_letter;
> >>  		} else {
> >> -			/* Unhandled absolute symlink. Report an error. */
> >> -			cifs_dbg(VFS,
> >> -				 "absolute symlink '%s' cannot be converted from NT format "
> >> -				 "because points to unknown target\n",
> >> -				 smb_target);
> >> -			rc = -EIO;
> >> -			goto out;
> >> +			goto out_unhandled_target;
> >>  		}
> >>  
> >>  		abs_path_len = strlen(abs_path)+1;
> >> @@ -966,6 +953,7 @@ int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len,
> >>  		 * These paths have same format as Linux symlinks, so no
> >>  		 * conversion is needed.
> >>  		 */
> >> +out_unhandled_target:
> >>  		linux_target = smb_target;
> >>  		smb_target = NULL;
> >>  	}
> >
> > I'm really not sure if removing the messages and error reporting about
> > symlinks which cannot be represented in POSIX system is a good idea.
> 
> Those messages are just useless and noisy.  Do you think it's useful
> printing that message for _every_ symlink when someone is calling
> readdir(2) in a directory that contain such files?

I though that for any debugging purposes these messages are useful.
Now I see that VFS level is printed always, so maybe the FYI level could
be better. Or do you really think that it is useless even for debugging?

So what would be the next steps here?

On Saturday 21 June 2025 18:52:46 Pali Rohár wrote:
> On Saturday 21 June 2025 12:38:34 Paulo Alcantara wrote:
> > Pali Rohár <pali@kernel.org> writes:
> > 
> > > On Friday 20 June 2025 20:44:37 Paulo Alcantara wrote:
> > >> Pali Rohár <pali@kernel.org> writes:
> > >> 
> > >> > Currently Linux SMB client returns EIO for lstat() and AT_SYMLINK_NOFOLLOW
> > >> > calls on symlink node when the symlink target location is broken or cannot
> > >> > be read or parsed.
> > >> >
> > >> > Fix this problem by relaxing the errors from various locations which parses
> > >> > information about symlink file node (UNIX SMB1, native SMB2+, NFS-style,
> > >> > WSL-style) and let readlink() syscall to return EIO when the symlink target
> > >> > location is not available.
> > >> 
> > >> Please, don't.  We still want those validations for the other types of
> > >> symlinks.
> > >
> > > Well, validation was not removed. Validation is still there, just the
> > > error is signalled by the readlink() syscall instead of the lstat() or
> > > AT_SYMLINK_NOFOLLOW syscalls.
> > >
> > > My opinion is that the lstat() or AT_SYMLINK_NOFOLLOW should work on
> > > symlink node independently of where the symlink points (and whether the
> > > symlink target is valid POSIX path or not). That is because the lstat()
> > > and AT_SYMLINK_NOFOLLOW says that the symlink target location must not
> > > be used and must not be resolved.
> > >
> > > But still the invalid / incorrect / broken or non-representable symlink
> > > target path in POSIX notation should be reported as an issue and the
> > > readlink() is the correct syscall which should report these errors.
> > 
> > The only issue is breaking existing customer or user applications that
> > really don't care if cifs.ko could follow those kind of symlinks.
> > 
> > Samba create symlinks to represent DFS links with targets like
> > 'msdfs:srv1\share,srv2\share', which are not valid POSIX paths.  Does
> > that mean the filesystem should not allow readlink(2) to succeed just
> > because it is not a valid POSIX path?  Is that what you mean?
> 
> But this is something totally different thing.
> 
> Here you are referring to the behavior of Samba server, which interprets
> symlink node stored on local filesystem named e.g. "link1" pointing to
> target relative file name 'msdfs:srv1\share,srv2\share' specially.
> 
> Calling "ln -s 'msdfs:srv1\share,srv2\share' link1" is perfectly fine on
> the ext4 filesystem. It creates a relative symlink to the specified
> file.
> 
> And if you call "echo test > 'msdfs:srv1\share,srv2\share'" then it would
> world correctly and "cat link1" will print "test".
> 
> The 'msdfs:srv1\share,srv2\share' is a valid POSIX path and it is stored
> on the local Linux filesystem. So I do not see anything wrong with it or
> reason why local filesystem should disallow creating such symlink or why
> would realink() should fail on such node.
> 
> 
> That example has nothing with symlinks stored on NTFS-compatible
> filesystems which has ability to store symlinks pointing to non-POSIX
> NT object model paths.
> 
> Here the issue is with symlink target locations which are coming from
> the remote NT server and are pointing to location which cannot be
> directly represented by the Linux system. The translation needs to be
> done in both directions and reversible. Otherwise moving the file or
> symlink from cifs to ext4 and back would damage the file or symlink.
> 
> > >> The problem is just that cifs.ko can't handle absolute
> > >> symlink targets in the form of '\??\UNC\srv\share\foo', while Windows
> > >> client can.  They are still valid symlink targets, but cifs.ko doesn't
> > >> know how to follow them.
> > >
> > > Windows client can represent and follow such symlink because the symlink
> > > is in the NT style format and Windows kernel uses NT style of paths
> > > internally. Linux kernel uses POSIX paths and POSIX does not contain any
> > > GLOBAL?? namespace for NT object hierarchy.
> > >
> > > Leaking raw NT object hierarchy from SMB to POSIX userspace via
> > > readlink() syscall is a bad idea. Applications are really not expecting
> > > that the readlink() syscall will return NT kernel internals (exported
> > > over SMB protocol and passed to cifs.ko).
> > >
> > > For UNC paths encoded in NT object hierarchy, which is just some subset
> > > of all possible NT paths, I had an idea that we could convert these
> > > paths to some format like:
> > >
> > >    <prefix>/server/share/path...
> > >
> > > Where <prefix> would be specified by the string mount option. So user
> > > could say that wants all UNC symlinks pointing to /mnt/unc/.
> > >
> > > And in the same way if user would want to create symlink pointing to
> > > /mnt/unc/server/share/path... then cifs.ko will transform it into valid
> > > NT UNC path and create a symlink to this location.
> > 
> > That's really a terrible idea.  The symlink targets in the form of
> > '\??\UNC\...' could be resolved by cifs.ko.  The ones that refer to a
> > file outside the mounted share, we would set those as automounts.
> 
> I agree that above is not the best idea, but I wrote at least something
> as an idea as I do not know how it could be solved in better way.
> 
> And I do not see how it could be resolved by cifs.ko somehow
> automatically. I'm not sure to which you refer how it can be resolved by
> cifs.ko. I understood you message as it could automount another share
> and do the whole path symlink resolving in cifs.ko.
> 
> And I think that this is even worse idea than mine. Because that
> automount means that symlinks pointing outside of the share would start
> behaving like a mount point. Such thing can cause even a security issues
> if not used carefully.
> 
> But moreover there is a big difference between symlink and mount point.
> Symlinks are not resolved by filesystem itself (but rather by the VFS,
> to ensure that all access checks are applied) and also moving the
> symlink between filesystems should not break them. In this idea when the
> symlink is going to be moved from smb share to e.g. ext4 local fs, then
> it would stops working (if the path resolved is in the cifs.ko) as
> ext4.ko would not be able to process special cifs.ko symlinks.
> 
> > > But this would solve only problem with UNC symlink, not symlinks
> > > pointing to NT object hierarchy in general.
> > >
> > >> The following should do it and then restore old behavior
> > >> 
> > >> diff --git a/fs/smb/client/reparse.c b/fs/smb/client/reparse.c
> > >> index bb25e77c5540..11d44288e75a 100644
> > >> --- a/fs/smb/client/reparse.c
> > >> +++ b/fs/smb/client/reparse.c
> > >> @@ -875,15 +875,8 @@ int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len,
> > >>  			abs_path += sizeof("\\DosDevices\\")-1;
> > >>  		else if (strstarts(abs_path, "\\GLOBAL??\\"))
> > >>  			abs_path += sizeof("\\GLOBAL??\\")-1;
> > >> -		else {
> > >> -			/* Unhandled absolute symlink, points outside of DOS/Win32 */
> > >> -			cifs_dbg(VFS,
> > >> -				 "absolute symlink '%s' cannot be converted from NT format "
> > >> -				 "because points to unknown target\n",
> > >> -				 smb_target);
> > >> -			rc = -EIO;
> > >> -			goto out;
> > >> -		}
> > >> +		else
> > >> +			goto out_unhandled_target;
> > >>  
> > >>  		/* Sometimes path separator after \?? is double backslash */
> > >>  		if (abs_path[0] == '\\')
> > >> @@ -910,13 +903,7 @@ int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len,
> > >>  			abs_path++;
> > >>  			abs_path[0] = drive_letter;
> > >>  		} else {
> > >> -			/* Unhandled absolute symlink. Report an error. */
> > >> -			cifs_dbg(VFS,
> > >> -				 "absolute symlink '%s' cannot be converted from NT format "
> > >> -				 "because points to unknown target\n",
> > >> -				 smb_target);
> > >> -			rc = -EIO;
> > >> -			goto out;
> > >> +			goto out_unhandled_target;
> > >>  		}
> > >>  
> > >>  		abs_path_len = strlen(abs_path)+1;
> > >> @@ -966,6 +953,7 @@ int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len,
> > >>  		 * These paths have same format as Linux symlinks, so no
> > >>  		 * conversion is needed.
> > >>  		 */
> > >> +out_unhandled_target:
> > >>  		linux_target = smb_target;
> > >>  		smb_target = NULL;
> > >>  	}
> > >
> > > I'm really not sure if removing the messages and error reporting about
> > > symlinks which cannot be represented in POSIX system is a good idea.
> > 
> > Those messages are just useless and noisy.  Do you think it's useful
> > printing that message for _every_ symlink when someone is calling
> > readdir(2) in a directory that contain such files?
> 
> I though that for any debugging purposes these messages are useful.
> Now I see that VFS level is printed always, so maybe the FYI level could
> be better. Or do you really think that it is useless even for debugging?

Looks like Paulo's fix ("smb: client: fix regression with native SMB
symlinks") which has been in for-next for a while with no issues in
testing etc. does fix the regression.

I did verify that the simple act of creating symlink (e.g. to some UNC
name) on Windows and trying to do an "ls -l" or "stat" or "cp" does
regress without his patch and with his patch behaves as expected (e.g.
if you went to linux and created a symlink to "filethatdoesnotexist"
then with his patch the behavior for stat, cp, cp --no-dereference and
ls -l is the same for bad target on ext4 mounts locally as it is for
bad target on smb3 mount to Windows)

There may be ways to address corner cases in the future - but it does
clearly address the regression, and for those four operations (stat,
cp, cp --no-dereference and ls -l) now behaves as expected when you
have an unresolvable target path of a symlink.

Don't mind discussing a followon patch to address more corner cases
but his patch looks fine and multiple people have tried it and looked
at (not just me) so should be upstream soon

On Fri, Jun 27, 2025 at 4:03 PM Pali Rohár <pali@kernel.org> wrote:
>
> So what would be the next steps here?
>
> On Saturday 21 June 2025 18:52:46 Pali Rohár wrote:
> > On Saturday 21 June 2025 12:38:34 Paulo Alcantara wrote:
> > > Pali Rohár <pali@kernel.org> writes:
> > >
> > > > On Friday 20 June 2025 20:44:37 Paulo Alcantara wrote:
> > > >> Pali Rohár <pali@kernel.org> writes:
> > > >>
> > > >> > Currently Linux SMB client returns EIO for lstat() and AT_SYMLINK_NOFOLLOW
> > > >> > calls on symlink node when the symlink target location is broken or cannot
> > > >> > be read or parsed.
> > > >> >
> > > >> > Fix this problem by relaxing the errors from various locations which parses
> > > >> > information about symlink file node (UNIX SMB1, native SMB2+, NFS-style,
> > > >> > WSL-style) and let readlink() syscall to return EIO when the symlink target
> > > >> > location is not available.
> > > >>
> > > >> Please, don't.  We still want those validations for the other types of
> > > >> symlinks.
> > > >
> > > > Well, validation was not removed. Validation is still there, just the
> > > > error is signalled by the readlink() syscall instead of the lstat() or
> > > > AT_SYMLINK_NOFOLLOW syscalls.
> > > >
> > > > My opinion is that the lstat() or AT_SYMLINK_NOFOLLOW should work on
> > > > symlink node independently of where the symlink points (and whether the
> > > > symlink target is valid POSIX path or not). That is because the lstat()
> > > > and AT_SYMLINK_NOFOLLOW says that the symlink target location must not
> > > > be used and must not be resolved.
> > > >
> > > > But still the invalid / incorrect / broken or non-representable symlink
> > > > target path in POSIX notation should be reported as an issue and the
> > > > readlink() is the correct syscall which should report these errors.
> > >
> > > The only issue is breaking existing customer or user applications that
> > > really don't care if cifs.ko could follow those kind of symlinks.
> > >
> > > Samba create symlinks to represent DFS links with targets like
> > > 'msdfs:srv1\share,srv2\share', which are not valid POSIX paths.  Does
> > > that mean the filesystem should not allow readlink(2) to succeed just
> > > because it is not a valid POSIX path?  Is that what you mean?
> >
> > But this is something totally different thing.
> >
> > Here you are referring to the behavior of Samba server, which interprets
> > symlink node stored on local filesystem named e.g. "link1" pointing to
> > target relative file name 'msdfs:srv1\share,srv2\share' specially.
> >
> > Calling "ln -s 'msdfs:srv1\share,srv2\share' link1" is perfectly fine on
> > the ext4 filesystem. It creates a relative symlink to the specified
> > file.
> >
> > And if you call "echo test > 'msdfs:srv1\share,srv2\share'" then it would
> > world correctly and "cat link1" will print "test".
> >
> > The 'msdfs:srv1\share,srv2\share' is a valid POSIX path and it is stored
> > on the local Linux filesystem. So I do not see anything wrong with it or
> > reason why local filesystem should disallow creating such symlink or why
> > would realink() should fail on such node.
> >
> >
> > That example has nothing with symlinks stored on NTFS-compatible
> > filesystems which has ability to store symlinks pointing to non-POSIX
> > NT object model paths.
> >
> > Here the issue is with symlink target locations which are coming from
> > the remote NT server and are pointing to location which cannot be
> > directly represented by the Linux system. The translation needs to be
> > done in both directions and reversible. Otherwise moving the file or
> > symlink from cifs to ext4 and back would damage the file or symlink.
> >
> > > >> The problem is just that cifs.ko can't handle absolute
> > > >> symlink targets in the form of '\??\UNC\srv\share\foo', while Windows
> > > >> client can.  They are still valid symlink targets, but cifs.ko doesn't
> > > >> know how to follow them.
> > > >
> > > > Windows client can represent and follow such symlink because the symlink
> > > > is in the NT style format and Windows kernel uses NT style of paths
> > > > internally. Linux kernel uses POSIX paths and POSIX does not contain any
> > > > GLOBAL?? namespace for NT object hierarchy.
> > > >
> > > > Leaking raw NT object hierarchy from SMB to POSIX userspace via
> > > > readlink() syscall is a bad idea. Applications are really not expecting
> > > > that the readlink() syscall will return NT kernel internals (exported
> > > > over SMB protocol and passed to cifs.ko).
> > > >
> > > > For UNC paths encoded in NT object hierarchy, which is just some subset
> > > > of all possible NT paths, I had an idea that we could convert these
> > > > paths to some format like:
> > > >
> > > >    <prefix>/server/share/path...
> > > >
> > > > Where <prefix> would be specified by the string mount option. So user
> > > > could say that wants all UNC symlinks pointing to /mnt/unc/.
> > > >
> > > > And in the same way if user would want to create symlink pointing to
> > > > /mnt/unc/server/share/path... then cifs.ko will transform it into valid
> > > > NT UNC path and create a symlink to this location.
> > >
> > > That's really a terrible idea.  The symlink targets in the form of
> > > '\??\UNC\...' could be resolved by cifs.ko.  The ones that refer to a
> > > file outside the mounted share, we would set those as automounts.
> >
> > I agree that above is not the best idea, but I wrote at least something
> > as an idea as I do not know how it could be solved in better way.
> >
> > And I do not see how it could be resolved by cifs.ko somehow
> > automatically. I'm not sure to which you refer how it can be resolved by
> > cifs.ko. I understood you message as it could automount another share
> > and do the whole path symlink resolving in cifs.ko.
> >
> > And I think that this is even worse idea than mine. Because that
> > automount means that symlinks pointing outside of the share would start
> > behaving like a mount point. Such thing can cause even a security issues
> > if not used carefully.
> >
> > But moreover there is a big difference between symlink and mount point.
> > Symlinks are not resolved by filesystem itself (but rather by the VFS,
> > to ensure that all access checks are applied) and also moving the
> > symlink between filesystems should not break them. In this idea when the
> > symlink is going to be moved from smb share to e.g. ext4 local fs, then
> > it would stops working (if the path resolved is in the cifs.ko) as
> > ext4.ko would not be able to process special cifs.ko symlinks.
> >
> > > > But this would solve only problem with UNC symlink, not symlinks
> > > > pointing to NT object hierarchy in general.
> > > >
> > > >> The following should do it and then restore old behavior
> > > >>
> > > >> diff --git a/fs/smb/client/reparse.c b/fs/smb/client/reparse.c
> > > >> index bb25e77c5540..11d44288e75a 100644
> > > >> --- a/fs/smb/client/reparse.c
> > > >> +++ b/fs/smb/client/reparse.c
> > > >> @@ -875,15 +875,8 @@ int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len,
> > > >>                          abs_path += sizeof("\\DosDevices\\")-1;
> > > >>                  else if (strstarts(abs_path, "\\GLOBAL??\\"))
> > > >>                          abs_path += sizeof("\\GLOBAL??\\")-1;
> > > >> -                else {
> > > >> -                        /* Unhandled absolute symlink, points outside of DOS/Win32 */
> > > >> -                        cifs_dbg(VFS,
> > > >> -                                 "absolute symlink '%s' cannot be converted from NT format "
> > > >> -                                 "because points to unknown target\n",
> > > >> -                                 smb_target);
> > > >> -                        rc = -EIO;
> > > >> -                        goto out;
> > > >> -                }
> > > >> +                else
> > > >> +                        goto out_unhandled_target;
> > > >>
> > > >>                  /* Sometimes path separator after \?? is double backslash */
> > > >>                  if (abs_path[0] == '\\')
> > > >> @@ -910,13 +903,7 @@ int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len,
> > > >>                          abs_path++;
> > > >>                          abs_path[0] = drive_letter;
> > > >>                  } else {
> > > >> -                        /* Unhandled absolute symlink. Report an error. */
> > > >> -                        cifs_dbg(VFS,
> > > >> -                                 "absolute symlink '%s' cannot be converted from NT format "
> > > >> -                                 "because points to unknown target\n",
> > > >> -                                 smb_target);
> > > >> -                        rc = -EIO;
> > > >> -                        goto out;
> > > >> +                        goto out_unhandled_target;
> > > >>                  }
> > > >>
> > > >>                  abs_path_len = strlen(abs_path)+1;
> > > >> @@ -966,6 +953,7 @@ int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len,
> > > >>                   * These paths have same format as Linux symlinks, so no
> > > >>                   * conversion is needed.
> > > >>                   */
> > > >> +out_unhandled_target:
> > > >>                  linux_target = smb_target;
> > > >>                  smb_target = NULL;
> > > >>          }
> > > >
> > > > I'm really not sure if removing the messages and error reporting about
> > > > symlinks which cannot be represented in POSIX system is a good idea.
> > >
> > > Those messages are just useless and noisy.  Do you think it's useful
> > > printing that message for _every_ symlink when someone is calling
> > > readdir(2) in a directory that contain such files?
> >
> > I though that for any debugging purposes these messages are useful.
> > Now I see that VFS level is printed always, so maybe the FYI level could
> > be better. Or do you really think that it is useless even for debugging?
>


-- 
Thanks,

Steve

tentatively merged into cifs-2.6.git for-next pending more testing.

Also added Acked-by from Meetakshi

On Tue, Jun 10, 2025 at 4:34 PM Pali Rohár <pali@kernel.org> wrote:
>
> Currently Linux SMB client returns EIO for lstat() and AT_SYMLINK_NOFOLLOW
> calls on symlink node when the symlink target location is broken or cannot
> be read or parsed.
>
> Fix this problem by relaxing the errors from various locations which parses
> information about symlink file node (UNIX SMB1, native SMB2+, NFS-style,
> WSL-style) and let readlink() syscall to return EIO when the symlink target
> location is not available.
>
> Note that SFU symlinks and MF symlinks are not affected by this issue,
> their parser has already relaxed code.
>
> This change fixes the 'ls -l -a' call on directory which has symlink nodes
> with broken target locations.
>
> Reported-by: Remy Monsen <monsen@monsen.cc>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  fs/smb/client/cifsfs.c   |  3 +-
>  fs/smb/client/inode.c    | 24 ++++++++++++++++
>  fs/smb/client/reparse.c  | 61 ++++++++++++++++++++++++++++++++--------
>  fs/smb/client/smb2file.c | 10 ++++---
>  4 files changed, 81 insertions(+), 17 deletions(-)
>
> diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
> index a08c42363ffc..f4b923f73dca 100644
> --- a/fs/smb/client/cifsfs.c
> +++ b/fs/smb/client/cifsfs.c
> @@ -1180,7 +1180,8 @@ const char *cifs_get_link(struct dentry *dentry, struct inode *inode,
>                 strscpy(target_path, CIFS_I(inode)->symlink_target, PATH_MAX);
>         } else {
>                 kfree(target_path);
> -               target_path = ERR_PTR(-EOPNOTSUPP);
> +               /* If symlink_target is not filled for symlink then it is an IO error. */
> +               target_path = ERR_PTR(-EIO);
>         }
>         spin_unlock(&inode->i_lock);
>
> diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
> index b1c6e3986278..762cd194946a 100644
> --- a/fs/smb/client/inode.c
> +++ b/fs/smb/client/inode.c
> @@ -480,6 +480,12 @@ static int cifs_get_unix_fattr(const unsigned char *full_path,
>                                                 cifs_sb, full_path,
>                                                 &fattr->cf_symlink_target);
>                 cifs_dbg(FYI, "%s: query_symlink: %d\n", __func__, rc);
> +               /*
> +                * Convert -EIO to 0. This let lstat() success and
> +                * empty data->cf_symlink_target triggers readlink() to fail with -EIO.
> +                */
> +               if (rc == -EIO)
> +                       rc = 0;
>         }
>         return rc;
>  }
> @@ -1133,6 +1139,12 @@ static int reparse_info_to_fattr(struct cifs_open_info_data *data,
>                         rc = server->ops->query_symlink(xid, tcon,
>                                                         cifs_sb, full_path,
>                                                         &data->symlink_target);
> +                       /*
> +                        * Convert -EIO to 0. This let lstat() success and
> +                        * empty data->symlink_target triggers readlink() to fail with -EIO.
> +                        */
> +                       if (rc == -EIO)
> +                               rc = 0;
>                 }
>                 if (rc == -EOPNOTSUPP)
>                         data->reparse.tag = IO_REPARSE_TAG_INTERNAL;
> @@ -1182,6 +1194,18 @@ static int reparse_info_to_fattr(struct cifs_open_info_data *data,
>                          */
>                         if (rc == -EOPNOTSUPP)
>                                 rc = 0;
> +               } else if (data->reparse.tag == IO_REPARSE_TAG_SYMLINK) {
> +                       /*
> +                        * data->reparse.tag can be set to IO_REPARSE_TAG_SYMLINK
> +                        * by STATUS_STOPPED_ON_SYMLINK error code. In this case
> +                        * we do not have a real reparse point iov buffer so
> +                        * data->reparse.buf and data->reparse.io.iov.iov_base
> +                        * are not set. And in the case symlink target location
> +                        * in the struct smb2_symlink_err_rsp is parsable then we
> +                        * even do not have data->symlink_target. So set rc to 0
> +                        * which let lstat() success and readlink() to fail.
> +                        */
> +                       rc = 0;
>                 }
>
>                 if (data->reparse.tag == IO_REPARSE_TAG_SYMLINK && !rc) {
> diff --git a/fs/smb/client/reparse.c b/fs/smb/client/reparse.c
> index d1d25f5f72ca..c70affb7b7f7 100644
> --- a/fs/smb/client/reparse.c
> +++ b/fs/smb/client/reparse.c
> @@ -739,7 +739,11 @@ static int parse_reparse_nfs(struct reparse_nfs_data_buffer *buf,
>         case NFS_SPECFILE_LNK:
>                 if (len == 0 || (len % 2)) {
>                         cifs_dbg(VFS, "srv returned malformed nfs symlink buffer\n");
> -                       return -EIO;
> +                       /*
> +                        * This is an -EIO error. Convert it to 0. This let lstat() success and
> +                        * empty data->symlink_target triggers readlink() to fail with -EIO.
> +                        */
> +                       return 0;
>                 }
>                 /*
>                  * Check that buffer does not contain UTF-16 null codepoint
> @@ -747,7 +751,11 @@ static int parse_reparse_nfs(struct reparse_nfs_data_buffer *buf,
>                  */
>                 if (UniStrnlen((wchar_t *)buf->DataBuffer, len/2) != len/2) {
>                         cifs_dbg(VFS, "srv returned null byte in nfs symlink target location\n");
> -                       return -EIO;
> +                       /*
> +                        * This is an -EIO error. Convert it to 0. This let lstat() success and
> +                        * empty data->symlink_target triggers readlink() to fail with -EIO.
> +                        */
> +                       return 0;
>                 }
>                 data->symlink_target = cifs_strndup_from_utf16(buf->DataBuffer,
>                                                                len, true,
> @@ -986,6 +994,14 @@ int smb2_parse_native_symlink(char **target, const char *buf, unsigned int len,
>         if (rc != 0)
>                 kfree(linux_target);
>         kfree(smb_target);
> +
> +       /*
> +       * Convert -EIO to 0. This let lstat() success and
> +       * empty *target triggers readlink() to fail with -EIO.
> +       */
> +       if (rc == -EIO)
> +               rc = 0;
> +
>         return rc;
>  }
>
> @@ -1004,7 +1020,11 @@ static int parse_reparse_native_symlink(struct reparse_symlink_data_buffer *sym,
>         len = le16_to_cpu(sym->SubstituteNameLength);
>         if (offs + 20 > plen || offs + len + 20 > plen) {
>                 cifs_dbg(VFS, "srv returned malformed symlink buffer\n");
> -               return -EIO;
> +               /*
> +                * This is an -EIO error. Convert it to 0. This let lstat() success and
> +                * empty data->symlink_target triggers readlink() to fail with -EIO.
> +                */
> +               return 0;
>         }
>
>         return smb2_parse_native_symlink(&data->symlink_target,
> @@ -1024,16 +1044,19 @@ static int parse_reparse_wsl_symlink(struct reparse_wsl_symlink_data_buffer *buf
>         int symname_utf8_len;
>         __le16 *symname_utf16;
>         int symname_utf16_len;
> +       int rc = 0;
>
>         if (len <= data_offset) {
>                 cifs_dbg(VFS, "srv returned malformed wsl symlink buffer\n");
> -               return -EIO;
> +               rc = -EIO;
> +               goto out;
>         }
>
>         /* MS-FSCC 2.1.2.7 defines layout of the Target field only for Version 2. */
>         if (le32_to_cpu(buf->Version) != 2) {
>                 cifs_dbg(VFS, "srv returned unsupported wsl symlink version %u\n", le32_to_cpu(buf->Version));
> -               return -EIO;
> +               rc = -EIO;
> +               goto out;
>         }
>
>         /* Target for Version 2 is in UTF-8 but without trailing null-term byte */
> @@ -1044,17 +1067,21 @@ static int parse_reparse_wsl_symlink(struct reparse_wsl_symlink_data_buffer *buf
>          */
>         if (strnlen(buf->Target, symname_utf8_len) != symname_utf8_len) {
>                 cifs_dbg(VFS, "srv returned null byte in wsl symlink target location\n");
> -               return -EIO;
> +               rc = -EIO;
> +               goto out;
>         }
>         symname_utf16 = kzalloc(symname_utf8_len * 2, GFP_KERNEL);
> -       if (!symname_utf16)
> -               return -ENOMEM;
> +       if (!symname_utf16) {
> +               rc = -ENOMEM;
> +               goto out;
> +       }
>         symname_utf16_len = utf8s_to_utf16s(buf->Target, symname_utf8_len,
>                                             UTF16_LITTLE_ENDIAN,
>                                             (wchar_t *) symname_utf16, symname_utf8_len * 2);
>         if (symname_utf16_len < 0) {
>                 kfree(symname_utf16);
> -               return symname_utf16_len;
> +               rc = symname_utf16_len;
> +               goto out;
>         }
>         symname_utf16_len *= 2; /* utf8s_to_utf16s() returns number of u16 items, not byte length */
>
> @@ -1062,10 +1089,20 @@ static int parse_reparse_wsl_symlink(struct reparse_wsl_symlink_data_buffer *buf
>                                                        symname_utf16_len, true,
>                                                        cifs_sb->local_nls);
>         kfree(symname_utf16);
> -       if (!data->symlink_target)
> -               return -ENOMEM;
> +       if (!data->symlink_target) {
> +               rc = -ENOMEM;
> +               goto out;
> +       }
>
> -       return 0;
> +out:
> +       /*
> +       * Convert -EIO to 0. This let lstat() success and
> +       * empty data->symlink_target triggers readlink() to fail with -EIO.
> +       */
> +       if (rc == -EIO)
> +               rc = 0;
> +
> +       return rc;
>  }
>
>  int parse_reparse_point(struct reparse_data_buffer *buf,
> diff --git a/fs/smb/client/smb2file.c b/fs/smb/client/smb2file.c
> index a7f629238830..9ac359f7be43 100644
> --- a/fs/smb/client/smb2file.c
> +++ b/fs/smb/client/smb2file.c
> @@ -76,11 +76,11 @@ int smb2_fix_symlink_target_type(char **target, bool directory, struct cifs_sb_i
>                 return 0;
>
>         if (!*target)
> -               return -EIO;
> +               return 0;
>
>         len = strlen(*target);
>         if (!len)
> -               return -EIO;
> +               return 0;
>
>         /*
>          * If this is directory symlink and it does not have trailing slash then
> @@ -103,8 +103,10 @@ int smb2_fix_symlink_target_type(char **target, bool directory, struct cifs_sb_i
>          * cannot contain slash character. File name with slash is invalid on
>          * both Windows and Linux systems. So return an error for such symlink.
>          */
> -       if (!directory && (*target)[len-1] == '/')
> -               return -EIO;
> +       if (!directory && (*target)[len-1] == '/') {
> +               kfree(*target);
> +               *target = NULL;
> +       }
>
>         return 0;
>  }
> --
> 2.20.1
>
>


-- 
Thanks,

Steve