| Message ID | 20240913200204.10660-1-pali@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | cifs: Fix getting reparse points from server without WSL support | expand |
Paulo, please look at this patch as it is related to WSL attributes which you introduced in the mentioned commit. I think that the proper fix should be to change SMB2_OP_QUERY_WSL_EA code to not trigger that -EOPNOTSUPP error which is delivered to userspace. I just checked that this my patch works fine for Native NTFS symlinks and NFS-style reparse point special files. On Friday 13 September 2024 22:02:04 Pali Rohár wrote: > If SMB server does not support WSL EAs then for SMB2_OP_QUERY_WSL_EA > request it returns STATUS_EAS_NOT_SUPPORTED. Function smb2_compound_op() > translates STATUS_EAS_NOT_SUPPORTED to -EOPNOTSUPP which cause failure > during calling lstat() syscall from userspace on any reparse point, > including Native SMB symlink (which does not use any EAs). This issue > happen for example when trying to resolve Native NTFS symlink from SMB > server on Windows 8. > > Avoid this problem by calling SMB2_OP_QUERY_WSL_EA only when detected > reparse point is of WSL type. Note that this is not solve this problem > fully as WSL reparse points can be created also on other SMB server > which do not support Extended Attributes at all. > > Fixes: ea41367b2a60 ("smb: client: introduce SMB2_OP_QUERY_WSL_EA") > Signed-off-by: Pali Rohár <pali@kernel.org> > --- > fs/smb/client/smb2inode.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c > index 11a1c53c64e0..d65471aa55e6 100644 > --- a/fs/smb/client/smb2inode.c > +++ b/fs/smb/client/smb2inode.c > @@ -941,7 +941,20 @@ int smb2_query_path_info(const unsigned int xid, > if (rc || !data->reparse_point) > goto out; > > - cmds[num_cmds++] = SMB2_OP_QUERY_WSL_EA; > + /* > + * Skip SMB2_OP_QUERY_WSL_EA if reparse point is not WSL. > + * The server file system does not have to support Extended > + * Attributes and in this case returns STATUS_EAS_NOT_SUPPORTED > + * which smb2_compound_op() translate to -EOPNOTSUPP. This will > + * present failure during reading of non-WSL special files. > + */ > + if (data->reparse.tag == IO_REPARSE_TAG_LX_SYMLINK || > + data->reparse.tag == IO_REPARSE_TAG_AF_UNIX || > + data->reparse.tag == IO_REPARSE_TAG_LX_FIFO || > + data->reparse.tag == IO_REPARSE_TAG_LX_CHR || > + data->reparse.tag == IO_REPARSE_TAG_LX_BLK) > + cmds[num_cmds++] = SMB2_OP_QUERY_WSL_EA; > + > /* > * Skip SMB2_OP_GET_REPARSE if symlink already parsed in create > * response. > -- > 2.20.1 >
And seems that SMB2_OP_QUERY_WSL_EA is useful not only for reparse points, but also for any regular file or directory as it can contain UNIX mode and UID/GID ownership. On Friday 13 September 2024 22:10:41 Pali Rohár wrote: > Paulo, please look at this patch as it is related to WSL attributes > which you introduced in the mentioned commit. I think that the proper > fix should be to change SMB2_OP_QUERY_WSL_EA code to not trigger that > -EOPNOTSUPP error which is delivered to userspace. I just checked that > this my patch works fine for Native NTFS symlinks and NFS-style reparse > point special files. > > On Friday 13 September 2024 22:02:04 Pali Rohár wrote: > > If SMB server does not support WSL EAs then for SMB2_OP_QUERY_WSL_EA > > request it returns STATUS_EAS_NOT_SUPPORTED. Function smb2_compound_op() > > translates STATUS_EAS_NOT_SUPPORTED to -EOPNOTSUPP which cause failure > > during calling lstat() syscall from userspace on any reparse point, > > including Native SMB symlink (which does not use any EAs). This issue > > happen for example when trying to resolve Native NTFS symlink from SMB > > server on Windows 8. > > > > Avoid this problem by calling SMB2_OP_QUERY_WSL_EA only when detected > > reparse point is of WSL type. Note that this is not solve this problem > > fully as WSL reparse points can be created also on other SMB server > > which do not support Extended Attributes at all. > > > > Fixes: ea41367b2a60 ("smb: client: introduce SMB2_OP_QUERY_WSL_EA") > > Signed-off-by: Pali Rohár <pali@kernel.org> > > --- > > fs/smb/client/smb2inode.c | 15 ++++++++++++++- > > 1 file changed, 14 insertions(+), 1 deletion(-) > > > > diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c > > index 11a1c53c64e0..d65471aa55e6 100644 > > --- a/fs/smb/client/smb2inode.c > > +++ b/fs/smb/client/smb2inode.c > > @@ -941,7 +941,20 @@ int smb2_query_path_info(const unsigned int xid, > > if (rc || !data->reparse_point) > > goto out; > > > > - cmds[num_cmds++] = SMB2_OP_QUERY_WSL_EA; > > + /* > > + * Skip SMB2_OP_QUERY_WSL_EA if reparse point is not WSL. > > + * The server file system does not have to support Extended > > + * Attributes and in this case returns STATUS_EAS_NOT_SUPPORTED > > + * which smb2_compound_op() translate to -EOPNOTSUPP. This will > > + * present failure during reading of non-WSL special files. > > + */ > > + if (data->reparse.tag == IO_REPARSE_TAG_LX_SYMLINK || > > + data->reparse.tag == IO_REPARSE_TAG_AF_UNIX || > > + data->reparse.tag == IO_REPARSE_TAG_LX_FIFO || > > + data->reparse.tag == IO_REPARSE_TAG_LX_CHR || > > + data->reparse.tag == IO_REPARSE_TAG_LX_BLK) > > + cmds[num_cmds++] = SMB2_OP_QUERY_WSL_EA; > > + > > /* > > * Skip SMB2_OP_GET_REPARSE if symlink already parsed in create > > * response. > > -- > > 2.20.1 > >
On Tue, Sep 17, 2024 at 10:06:00PM +0200, Pali Rohár wrote: >And seems that SMB2_OP_QUERY_WSL_EA is useful not only for reparse >points, but also for any regular file or directory as it can contain >UNIX mode and UID/GID ownership. uid/gid should *never* be exposed over the wire for SMB. That way lies madness.
On Tuesday 17 September 2024 13:23:40 Jeremy Allison wrote: > On Tue, Sep 17, 2024 at 10:06:00PM +0200, Pali Rohár wrote: > > And seems that SMB2_OP_QUERY_WSL_EA is useful not only for reparse > > points, but also for any regular file or directory as it can contain > > UNIX mode and UID/GID ownership. > > uid/gid should *never* be exposed over the wire for SMB. > > That way lies madness. Hello Jeremy, if I understood wsl_to_fattr() function correctly then it is already doing it, it fills uid/gid for stat() from data which were exposed over the wire for SMB. Could you check that function if it is truth?
On Tue, Sep 17, 2024 at 10:29:21PM +0200, Pali Rohár wrote: >On Tuesday 17 September 2024 13:23:40 Jeremy Allison wrote: >> On Tue, Sep 17, 2024 at 10:06:00PM +0200, Pali Rohár wrote: >> > And seems that SMB2_OP_QUERY_WSL_EA is useful not only for reparse >> > points, but also for any regular file or directory as it can contain >> > UNIX mode and UID/GID ownership. >> >> uid/gid should *never* be exposed over the wire for SMB. >> >> That way lies madness. > >Hello Jeremy, if I understood wsl_to_fattr() function correctly then it >is already doing it, it fills uid/gid for stat() from data which were >exposed over the wire for SMB. Could you check that function if it is >truth? I'm sure the Windows implementation is doing it - however, any Linux server implementations should not do this (IMHO). It will break all SID -> uid / gid mapping that servers must carefully set up. On the wire - SIDs must be the only source of identity.
On Tuesday 17 September 2024 13:31:22 Jeremy Allison wrote: > On Tue, Sep 17, 2024 at 10:29:21PM +0200, Pali Rohár wrote: > > On Tuesday 17 September 2024 13:23:40 Jeremy Allison wrote: > > > On Tue, Sep 17, 2024 at 10:06:00PM +0200, Pali Rohár wrote: > > > > And seems that SMB2_OP_QUERY_WSL_EA is useful not only for reparse > > > > points, but also for any regular file or directory as it can contain > > > > UNIX mode and UID/GID ownership. > > > > > > uid/gid should *never* be exposed over the wire for SMB. > > > > > > That way lies madness. > > > > Hello Jeremy, if I understood wsl_to_fattr() function correctly then it > > is already doing it, it fills uid/gid for stat() from data which were > > exposed over the wire for SMB. Could you check that function if it is > > truth? > > I'm sure the Windows implementation is doing it - however, any Linux > server implementations should not do this (IMHO). > > It will break all SID -> uid / gid mapping that servers must > carefully set up. > > On the wire - SIDs must be the only source of identity. Ok. But then I do not understand why Linux client parses and uses uid and gids which are sent over the wire. If you are saying that the SIDs must be the only source of truth then Linux client should rather ignore uid and gid values?
On Tue, Sep 17, 2024 at 10:34:31PM +0200, Pali Rohár wrote: >On Tuesday 17 September 2024 13:31:22 Jeremy Allison wrote: >> It will break all SID -> uid / gid mapping that servers must >> carefully set up. >> >> On the wire - SIDs must be the only source of identity. > >Ok. But then I do not understand why Linux client parses and uses uid >and gids which are sent over the wire. If you are saying that the SIDs >must be the only source of truth then Linux client should rather ignore >uid and gid values? > Yes. I think that should be the case. It's not my code though, so take that as my free 2 cents opinion :-). Samba will never expose uids / gids on the wire over SMB2+ though (it's too late for that mistake we made in SMB1).
On Wed, 18 Sept 2024 at 06:37, Pali Rohár <pali@kernel.org> wrote: > > On Tuesday 17 September 2024 13:31:22 Jeremy Allison wrote: > > On Tue, Sep 17, 2024 at 10:29:21PM +0200, Pali Rohár wrote: > > > On Tuesday 17 September 2024 13:23:40 Jeremy Allison wrote: > > > > On Tue, Sep 17, 2024 at 10:06:00PM +0200, Pali Rohár wrote: > > > > > And seems that SMB2_OP_QUERY_WSL_EA is useful not only for reparse > > > > > points, but also for any regular file or directory as it can contain > > > > > UNIX mode and UID/GID ownership. > > > > > > > > uid/gid should *never* be exposed over the wire for SMB. > > > > > > > > That way lies madness. > > > > > > Hello Jeremy, if I understood wsl_to_fattr() function correctly then it > > > is already doing it, it fills uid/gid for stat() from data which were > > > exposed over the wire for SMB. Could you check that function if it is > > > truth? > > > > I'm sure the Windows implementation is doing it - however, any Linux > > server implementations should not do this (IMHO). > > > > It will break all SID -> uid / gid mapping that servers must > > carefully set up. > > > > On the wire - SIDs must be the only source of identity. > > Ok. But then I do not understand why Linux client parses and uses uid > and gids which are sent over the wire. If you are saying that the SIDs > must be the only source of truth then Linux client should rather ignore > uid and gid values? What I think Jeremy is refering to is that mixing uids and sids in the protocol itself is a protocol design mistake. Because this means that some PDUs in the protocol operate on SIDs but others operate on UID/GIDs and this means there is great risk of mistakes and have the sid<->uid mapping return different results depending on the actual PDU. Sometimes the sid<->uid mapping happens in the server, at other times the mapping happens in the client and it is very difficult to guarantee that the mapping is consistent across PDUs in the protocol as well as across different clients. >
On Wed, Sep 18, 2024 at 06:44:39AM +1000, ronnie sahlberg wrote: >On Wed, 18 Sept 2024 at 06:37, Pali Rohár <pali@kernel.org> wrote: >> >> Ok. But then I do not understand why Linux client parses and uses uid >> and gids which are sent over the wire. If you are saying that the SIDs >> must be the only source of truth then Linux client should rather ignore >> uid and gid values? > >What I think Jeremy is refering to is that mixing uids and sids in the >protocol itself is >a protocol design mistake. >Because this means that some PDUs in the protocol operate on SIDs but >others operate on >UID/GIDs and this means there is great risk of mistakes and have the >sid<->uid mapping return >different results depending on the actual PDU. > >Sometimes the sid<->uid mapping happens in the server, at other times >the mapping happens in the client >and it is very difficult to guarantee that the mapping is consistent >across PDUs in the protocol >as well as across different clients. Thanks Ronnie. You said that much better than I did :-) :-).
On Tuesday 17 September 2024 13:46:18 Jeremy Allison wrote: > On Wed, Sep 18, 2024 at 06:44:39AM +1000, ronnie sahlberg wrote: > > On Wed, 18 Sept 2024 at 06:37, Pali Rohár <pali@kernel.org> wrote: > > > > > > Ok. But then I do not understand why Linux client parses and uses uid > > > and gids which are sent over the wire. If you are saying that the SIDs > > > must be the only source of truth then Linux client should rather ignore > > > uid and gid values? > > > > What I think Jeremy is refering to is that mixing uids and sids in the > > protocol itself is > > a protocol design mistake. > > Because this means that some PDUs in the protocol operate on SIDs but > > others operate on > > UID/GIDs and this means there is great risk of mistakes and have the > > sid<->uid mapping return > > different results depending on the actual PDU. > > > > Sometimes the sid<->uid mapping happens in the server, at other times > > the mapping happens in the client > > and it is very difficult to guarantee that the mapping is consistent > > across PDUs in the protocol > > as well as across different clients. > > Thanks Ronnie. You said that much better than I did :-) :-). Understood, thank you! So based on this for me it looks like that for client it would be safer to ignore uid an gid for reparse points and use only SIDs. I hope that somebody will recheck that client code in wsl_to_fattr() function.
Pali Rohár <pali@kernel.org> writes: > Paulo, please look at this patch as it is related to WSL attributes > which you introduced in the mentioned commit. I think that the proper > fix should be to change SMB2_OP_QUERY_WSL_EA code to not trigger that > -EOPNOTSUPP error which is delivered to userspace. I just checked that > this my patch works fine for Native NTFS symlinks and NFS-style reparse > point special files. Thanks for the patch. The problem is that the client is considering that the entire compound request failed when the server doesn't support EA. The client should still parse the rest of the response that contains the getinfo and get reparse info data.
On Tuesday 17 September 2024 18:04:37 Paulo Alcantara wrote: > Pali Rohár <pali@kernel.org> writes: > > > Paulo, please look at this patch as it is related to WSL attributes > > which you introduced in the mentioned commit. I think that the proper > > fix should be to change SMB2_OP_QUERY_WSL_EA code to not trigger that > > -EOPNOTSUPP error which is delivered to userspace. I just checked that > > this my patch works fine for Native NTFS symlinks and NFS-style reparse > > point special files. > > Thanks for the patch. The problem is that the client is considering > that the entire compound request failed when the server doesn't support > EA. The client should still parse the rest of the response that > contains the getinfo and get reparse info data. I agree with you. This sounds like the best option. Would you be able to fix the client code to do this?
On Tue, Sep 17, 2024 at 4:04 PM Paulo Alcantara <pc@manguebit.com> wrote: > > Pali Rohár <pali@kernel.org> writes: > > > Paulo, please look at this patch as it is related to WSL attributes > > which you introduced in the mentioned commit. I think that the proper > > fix should be to change SMB2_OP_QUERY_WSL_EA code to not trigger that > > -EOPNOTSUPP error which is delivered to userspace. I just checked that > > this my patch works fine for Native NTFS symlinks and NFS-style reparse > > point special files. > > Thanks for the patch. The problem is that the client is considering > that the entire compound request failed when the server doesn't support > EA. The client should still parse the rest of the response that > contains the getinfo and get reparse info data. Yes. Agreed. And on the
Pali Rohár <pali@kernel.org> writes:
> Would you be able to fix the client code to do this?
Yep. Will send it to ML soon.
On Tue, Sep 17, 2024 at 3:45 PM ronnie sahlberg <ronniesahlberg@gmail.com> wrote: > > On Wed, 18 Sept 2024 at 06:37, Pali Rohár <pali@kernel.org> wrote: > > > > On Tuesday 17 September 2024 13:31:22 Jeremy Allison wrote: > > > On Tue, Sep 17, 2024 at 10:29:21PM +0200, Pali Rohár wrote: > > > > On Tuesday 17 September 2024 13:23:40 Jeremy Allison wrote: > > > > > On Tue, Sep 17, 2024 at 10:06:00PM +0200, Pali Rohár wrote: > > > > > > And seems that SMB2_OP_QUERY_WSL_EA is useful not only for reparse > > > > > > points, but also for any regular file or directory as it can contain > > > > > > UNIX mode and UID/GID ownership. > > > > > > > > > > uid/gid should *never* be exposed over the wire for SMB. > > > > > > > > > > That way lies madness. > > > > > > > > Hello Jeremy, if I understood wsl_to_fattr() function correctly then it > > > > is already doing it, it fills uid/gid for stat() from data which were > > > > exposed over the wire for SMB. Could you check that function if it is > > > > truth? > > > > > > I'm sure the Windows implementation is doing it - however, any Linux > > > server implementations should not do this (IMHO). > > > > > > It will break all SID -> uid / gid mapping that servers must > > > carefully set up. > > > > > > On the wire - SIDs must be the only source of identity. > > > > Ok. But then I do not understand why Linux client parses and uses uid > > and gids which are sent over the wire. If you are saying that the SIDs > > must be the only source of truth then Linux client should rather ignore > > uid and gid values? > > What I think Jeremy is refering to is that mixing uids and sids in the > protocol itself is > a protocol design mistake. > Because this means that some PDUs in the protocol operate on SIDs but > others operate on > UID/GIDs and this means there is great risk of mistakes and have the > sid<->uid mapping return > different results depending on the actual PDU. > > Sometimes the sid<->uid mapping happens in the server, at other times > the mapping happens in the client > and it is very difficult to guarantee that the mapping is consistent > across PDUs in the protocol as well as across different clients. Yes - agreed. SIDs are globally unique and should always be used/sent over the wire (never send or use the local uid/gid which is not guaranteed to be unique). Whether retrieving ownership information via the SMB ACL or via an SMB3.1.1 POSIX response, the SID is the correct thing to send/use in the protocol. For cases where the client is not domain joined, the UID/GID can be encoded in the SID, for cases that are domain joined the Linux UIDs/GIDs can be mapped consistently via the SID.
diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c index 11a1c53c64e0..d65471aa55e6 100644 --- a/fs/smb/client/smb2inode.c +++ b/fs/smb/client/smb2inode.c @@ -941,7 +941,20 @@ int smb2_query_path_info(const unsigned int xid, if (rc || !data->reparse_point) goto out; - cmds[num_cmds++] = SMB2_OP_QUERY_WSL_EA; + /* + * Skip SMB2_OP_QUERY_WSL_EA if reparse point is not WSL. + * The server file system does not have to support Extended + * Attributes and in this case returns STATUS_EAS_NOT_SUPPORTED + * which smb2_compound_op() translate to -EOPNOTSUPP. This will + * present failure during reading of non-WSL special files. + */ + if (data->reparse.tag == IO_REPARSE_TAG_LX_SYMLINK || + data->reparse.tag == IO_REPARSE_TAG_AF_UNIX || + data->reparse.tag == IO_REPARSE_TAG_LX_FIFO || + data->reparse.tag == IO_REPARSE_TAG_LX_CHR || + data->reparse.tag == IO_REPARSE_TAG_LX_BLK) + cmds[num_cmds++] = SMB2_OP_QUERY_WSL_EA; + /* * Skip SMB2_OP_GET_REPARSE if symlink already parsed in create * response.
If SMB server does not support WSL EAs then for SMB2_OP_QUERY_WSL_EA request it returns STATUS_EAS_NOT_SUPPORTED. Function smb2_compound_op() translates STATUS_EAS_NOT_SUPPORTED to -EOPNOTSUPP which cause failure during calling lstat() syscall from userspace on any reparse point, including Native SMB symlink (which does not use any EAs). This issue happen for example when trying to resolve Native NTFS symlink from SMB server on Windows 8. Avoid this problem by calling SMB2_OP_QUERY_WSL_EA only when detected reparse point is of WSL type. Note that this is not solve this problem fully as WSL reparse points can be created also on other SMB server which do not support Extended Attributes at all. Fixes: ea41367b2a60 ("smb: client: introduce SMB2_OP_QUERY_WSL_EA") Signed-off-by: Pali Rohár <pali@kernel.org> --- fs/smb/client/smb2inode.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)