smb3: create smb3 equivalent alias for cifs pseudo-xattrs

Message ID CAH2r5mtjURuj=ePMsnpkt6pUqTCQ+aoFtxxh468F1Ur6byhrRw@mail.gmail.com
State New
Headers show
Series
  • smb3: create smb3 equivalent alias for cifs pseudo-xattrs
Related show

Commit Message

Steve French Aug. 10, 2018, 11:53 p.m.
We really, really don't want to be encouraging people to use
cifs (the dialect) since it is insecure, so to avoid confusion
we want to move them to names which include 'smb3' instead of
'cifs' - so this simply creates an alias for the pseudo-xattrs

e.g. can now do:
getfattr -n user.smb3.creationtime /mnt1/file
and
getfattr -n user.smb3.dosattrib /mnt1/file
and
getfattr -n system.smb3_acl /mnt1/file

instead of forcing you to use the string 'cifs' in
these (e.g. getfattr -n system.cifs_acl /mnt1/file)

Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/xattr.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)


 enum { XATTR_USER, XATTR_CIFS_ACL, XATTR_ACL_ACCESS, XATTR_ACL_DEFAULT };
@@ -220,10 +228,12 @@ static int cifs_xattr_get(const struct
xattr_handler *handler,
     switch (handler->flags) {
     case XATTR_USER:
         cifs_dbg(FYI, "%s:querying user xattr %s\n", __func__, name);
-        if (strcmp(name, CIFS_XATTR_ATTRIB) == 0) {
+        if ((strcmp(name, CIFS_XATTR_ATTRIB) == 0) ||
+            (strcmp(name, SMB3_XATTR_ATTRIB) == 0)) {
             rc = cifs_attrib_get(dentry, inode, value, size);
             break;
-        } else if (strcmp(name, CIFS_XATTR_CREATETIME) == 0) {
+        } else if ((strcmp(name, CIFS_XATTR_CREATETIME) == 0) ||
+            (strcmp(name, SMB3_XATTR_CREATETIME) == 0)) {
             rc = cifs_creation_time_get(dentry, inode, value, size);
             break;
         }
@@ -363,6 +373,19 @@ static const struct xattr_handler
cifs_cifs_acl_xattr_handler = {
     .set = cifs_xattr_set,
 };

+/*
+ * Although this is just an alias for the above, need to move away from
+ * confusing users and using the 20 year old term 'cifs' when it is no
+ * longer secure and was replaced by SMB2/SMB3 a long time ago, and
+ * SMB3 and later are highly secure.
+ */
+static const struct xattr_handler smb3_acl_xattr_handler = {
+    .name = SMB3_XATTR_CIFS_ACL,
+    .flags = XATTR_CIFS_ACL,
+    .get = cifs_xattr_get,
+    .set = cifs_xattr_set,
+};
+
 static const struct xattr_handler cifs_posix_acl_access_xattr_handler = {
     .name = XATTR_NAME_POSIX_ACL_ACCESS,
     .flags = XATTR_ACL_ACCESS,
@@ -381,6 +404,7 @@ const struct xattr_handler *cifs_xattr_handlers[] = {
     &cifs_user_xattr_handler,
     &cifs_os2_xattr_handler,
     &cifs_cifs_acl_xattr_handler,
+    &smb3_acl_xattr_handler, /* alias for above since avoiding "cifs" */
     &cifs_posix_acl_access_xattr_handler,
     &cifs_posix_acl_default_xattr_handler,
     NULL

Comments

ronnie sahlberg Aug. 11, 2018, 4:48 a.m. | #1
I am not sure this is the way to go.

I do understand the desire to purge the word "cifs" from where we are
now but this essentially will fork the namespace between new  clients
with new kernels and old clients with old kernels.
(I am assuming the plan is to in the future WRITE the acls under the
new name at which point older kernels/clients will no longer be
compatible with the naming.




On Sat, Aug 11, 2018 at 9:53 AM, Steve French <smfrench@gmail.com> wrote:
> We really, really don't want to be encouraging people to use
> cifs (the dialect) since it is insecure, so to avoid confusion
> we want to move them to names which include 'smb3' instead of
> 'cifs' - so this simply creates an alias for the pseudo-xattrs
>
> e.g. can now do:
> getfattr -n user.smb3.creationtime /mnt1/file
> and
> getfattr -n user.smb3.dosattrib /mnt1/file
> and
> getfattr -n system.smb3_acl /mnt1/file
>
> instead of forcing you to use the string 'cifs' in
> these (e.g. getfattr -n system.cifs_acl /mnt1/file)
>
> Signed-off-by: Steve French <stfrench@microsoft.com>
> ---
>  fs/cifs/xattr.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
> index 316af84674f1..50ddb795aaeb 100644
> --- a/fs/cifs/xattr.c
> +++ b/fs/cifs/xattr.c
> @@ -35,6 +35,14 @@
>  #define CIFS_XATTR_CIFS_ACL "system.cifs_acl"
>  #define CIFS_XATTR_ATTRIB "cifs.dosattrib"  /* full name:
> user.cifs.dosattrib */
>  #define CIFS_XATTR_CREATETIME "cifs.creationtime"  /* user.cifs.creationtime */
> +/*
> + * Although these three are just aliases for the above, need to move away from
> + * confusing users and using the 20+ year old term 'cifs' when it is no longer
> + * secure, replaced by SMB2 (then even more highly secure SMB3) many years ago
> + */
> +#define SMB3_XATTR_CIFS_ACL "system.smb3_acl"
> +#define SMB3_XATTR_ATTRIB "smb3.dosattrib"  /* full name:
> user.smb3.dosattrib */
> +#define SMB3_XATTR_CREATETIME "smb3.creationtime"  /* user.smb3.creationtime */
>  /* BB need to add server (Samba e.g) support for security and trusted prefix */
>
>  enum { XATTR_USER, XATTR_CIFS_ACL, XATTR_ACL_ACCESS, XATTR_ACL_DEFAULT };
> @@ -220,10 +228,12 @@ static int cifs_xattr_get(const struct
> xattr_handler *handler,
>      switch (handler->flags) {
>      case XATTR_USER:
>          cifs_dbg(FYI, "%s:querying user xattr %s\n", __func__, name);
> -        if (strcmp(name, CIFS_XATTR_ATTRIB) == 0) {
> +        if ((strcmp(name, CIFS_XATTR_ATTRIB) == 0) ||
> +            (strcmp(name, SMB3_XATTR_ATTRIB) == 0)) {
>              rc = cifs_attrib_get(dentry, inode, value, size);
>              break;
> -        } else if (strcmp(name, CIFS_XATTR_CREATETIME) == 0) {
> +        } else if ((strcmp(name, CIFS_XATTR_CREATETIME) == 0) ||
> +            (strcmp(name, SMB3_XATTR_CREATETIME) == 0)) {
>              rc = cifs_creation_time_get(dentry, inode, value, size);
>              break;
>          }
> @@ -363,6 +373,19 @@ static const struct xattr_handler
> cifs_cifs_acl_xattr_handler = {
>      .set = cifs_xattr_set,
>  };
>
> +/*
> + * Although this is just an alias for the above, need to move away from
> + * confusing users and using the 20 year old term 'cifs' when it is no
> + * longer secure and was replaced by SMB2/SMB3 a long time ago, and
> + * SMB3 and later are highly secure.
> + */
> +static const struct xattr_handler smb3_acl_xattr_handler = {
> +    .name = SMB3_XATTR_CIFS_ACL,
> +    .flags = XATTR_CIFS_ACL,
> +    .get = cifs_xattr_get,
> +    .set = cifs_xattr_set,
> +};
> +
>  static const struct xattr_handler cifs_posix_acl_access_xattr_handler = {
>      .name = XATTR_NAME_POSIX_ACL_ACCESS,
>      .flags = XATTR_ACL_ACCESS,
> @@ -381,6 +404,7 @@ const struct xattr_handler *cifs_xattr_handlers[] = {
>      &cifs_user_xattr_handler,
>      &cifs_os2_xattr_handler,
>      &cifs_cifs_acl_xattr_handler,
> +    &smb3_acl_xattr_handler, /* alias for above since avoiding "cifs" */
>      &cifs_posix_acl_access_xattr_handler,
>      &cifs_posix_acl_default_xattr_handler,
>      NULL
>
> --
> Thanks,
>
> Steve
Steve French Aug. 11, 2018, 4:56 a.m. | #2
no - my purpose was to give instructions in the future with the new
name so people won't be confused (old apps can keep using what cifsacl
does e.g. and use the "cifs" in the name).
The patch is so small - no need to worry about breaking old apps ...
we can keep "cifs" alias around on these attributes for a long time -
but it is very confusing when cifs has been (supposedly) dead for many
years now, to keep telling people instructions with that name in it.
To avoid breaking things - I don't mind keeping a few dozen lines of
extra code ... (with "cifs" legacy names).
On Fri, Aug 10, 2018 at 11:48 PM ronnie sahlberg
<ronniesahlberg@gmail.com> wrote:
>
> I am not sure this is the way to go.
>
> I do understand the desire to purge the word "cifs" from where we are
> now but this essentially will fork the namespace between new  clients
> with new kernels and old clients with old kernels.
> (I am assuming the plan is to in the future WRITE the acls under the
> new name at which point older kernels/clients will no longer be
> compatible with the naming.
>
>
>
>
> On Sat, Aug 11, 2018 at 9:53 AM, Steve French <smfrench@gmail.com> wrote:
> > We really, really don't want to be encouraging people to use
> > cifs (the dialect) since it is insecure, so to avoid confusion
> > we want to move them to names which include 'smb3' instead of
> > 'cifs' - so this simply creates an alias for the pseudo-xattrs
> >
> > e.g. can now do:
> > getfattr -n user.smb3.creationtime /mnt1/file
> > and
> > getfattr -n user.smb3.dosattrib /mnt1/file
> > and
> > getfattr -n system.smb3_acl /mnt1/file
> >
> > instead of forcing you to use the string 'cifs' in
> > these (e.g. getfattr -n system.cifs_acl /mnt1/file)
> >
> > Signed-off-by: Steve French <stfrench@microsoft.com>
> > ---
> >  fs/cifs/xattr.c | 28 ++++++++++++++++++++++++++--
> >  1 file changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
> > index 316af84674f1..50ddb795aaeb 100644
> > --- a/fs/cifs/xattr.c
> > +++ b/fs/cifs/xattr.c
> > @@ -35,6 +35,14 @@
> >  #define CIFS_XATTR_CIFS_ACL "system.cifs_acl"
> >  #define CIFS_XATTR_ATTRIB "cifs.dosattrib"  /* full name:
> > user.cifs.dosattrib */
> >  #define CIFS_XATTR_CREATETIME "cifs.creationtime"  /* user.cifs.creationtime */
> > +/*
> > + * Although these three are just aliases for the above, need to move away from
> > + * confusing users and using the 20+ year old term 'cifs' when it is no longer
> > + * secure, replaced by SMB2 (then even more highly secure SMB3) many years ago
> > + */
> > +#define SMB3_XATTR_CIFS_ACL "system.smb3_acl"
> > +#define SMB3_XATTR_ATTRIB "smb3.dosattrib"  /* full name:
> > user.smb3.dosattrib */
> > +#define SMB3_XATTR_CREATETIME "smb3.creationtime"  /* user.smb3.creationtime */
> >  /* BB need to add server (Samba e.g) support for security and trusted prefix */
> >
> >  enum { XATTR_USER, XATTR_CIFS_ACL, XATTR_ACL_ACCESS, XATTR_ACL_DEFAULT };
> > @@ -220,10 +228,12 @@ static int cifs_xattr_get(const struct
> > xattr_handler *handler,
> >      switch (handler->flags) {
> >      case XATTR_USER:
> >          cifs_dbg(FYI, "%s:querying user xattr %s\n", __func__, name);
> > -        if (strcmp(name, CIFS_XATTR_ATTRIB) == 0) {
> > +        if ((strcmp(name, CIFS_XATTR_ATTRIB) == 0) ||
> > +            (strcmp(name, SMB3_XATTR_ATTRIB) == 0)) {
> >              rc = cifs_attrib_get(dentry, inode, value, size);
> >              break;
> > -        } else if (strcmp(name, CIFS_XATTR_CREATETIME) == 0) {
> > +        } else if ((strcmp(name, CIFS_XATTR_CREATETIME) == 0) ||
> > +            (strcmp(name, SMB3_XATTR_CREATETIME) == 0)) {
> >              rc = cifs_creation_time_get(dentry, inode, value, size);
> >              break;
> >          }
> > @@ -363,6 +373,19 @@ static const struct xattr_handler
> > cifs_cifs_acl_xattr_handler = {
> >      .set = cifs_xattr_set,
> >  };
> >
> > +/*
> > + * Although this is just an alias for the above, need to move away from
> > + * confusing users and using the 20 year old term 'cifs' when it is no
> > + * longer secure and was replaced by SMB2/SMB3 a long time ago, and
> > + * SMB3 and later are highly secure.
> > + */
> > +static const struct xattr_handler smb3_acl_xattr_handler = {
> > +    .name = SMB3_XATTR_CIFS_ACL,
> > +    .flags = XATTR_CIFS_ACL,
> > +    .get = cifs_xattr_get,
> > +    .set = cifs_xattr_set,
> > +};
> > +
> >  static const struct xattr_handler cifs_posix_acl_access_xattr_handler = {
> >      .name = XATTR_NAME_POSIX_ACL_ACCESS,
> >      .flags = XATTR_ACL_ACCESS,
> > @@ -381,6 +404,7 @@ const struct xattr_handler *cifs_xattr_handlers[] = {
> >      &cifs_user_xattr_handler,
> >      &cifs_os2_xattr_handler,
> >      &cifs_cifs_acl_xattr_handler,
> > +    &smb3_acl_xattr_handler, /* alias for above since avoiding "cifs" */
> >      &cifs_posix_acl_access_xattr_handler,
> >      &cifs_posix_acl_default_xattr_handler,
> >      NULL
> >
> > --
> > Thanks,
> >
> > Steve
Steve French Aug. 11, 2018, 4:57 a.m. | #3
This is an alias not a change to the name.   We don't want to remove
the 'cifs' names for years if there is a risk of breaking apps - but
we might as well starting moving them now.
On Fri, Aug 10, 2018 at 11:56 PM Steve French <smfrench@gmail.com> wrote:
>
> no - my purpose was to give instructions in the future with the new
> name so people won't be confused (old apps can keep using what cifsacl
> does e.g. and use the "cifs" in the name).
> The patch is so small - no need to worry about breaking old apps ...
> we can keep "cifs" alias around on these attributes for a long time -
> but it is very confusing when cifs has been (supposedly) dead for many
> years now, to keep telling people instructions with that name in it.
> To avoid breaking things - I don't mind keeping a few dozen lines of
> extra code ... (with "cifs" legacy names).
> On Fri, Aug 10, 2018 at 11:48 PM ronnie sahlberg
> <ronniesahlberg@gmail.com> wrote:
> >
> > I am not sure this is the way to go.
> >
> > I do understand the desire to purge the word "cifs" from where we are
> > now but this essentially will fork the namespace between new  clients
> > with new kernels and old clients with old kernels.
> > (I am assuming the plan is to in the future WRITE the acls under the
> > new name at which point older kernels/clients will no longer be
> > compatible with the naming.
> >
> >
> >
> >
> > On Sat, Aug 11, 2018 at 9:53 AM, Steve French <smfrench@gmail.com> wrote:
> > > We really, really don't want to be encouraging people to use
> > > cifs (the dialect) since it is insecure, so to avoid confusion
> > > we want to move them to names which include 'smb3' instead of
> > > 'cifs' - so this simply creates an alias for the pseudo-xattrs
> > >
> > > e.g. can now do:
> > > getfattr -n user.smb3.creationtime /mnt1/file
> > > and
> > > getfattr -n user.smb3.dosattrib /mnt1/file
> > > and
> > > getfattr -n system.smb3_acl /mnt1/file
> > >
> > > instead of forcing you to use the string 'cifs' in
> > > these (e.g. getfattr -n system.cifs_acl /mnt1/file)
> > >
> > > Signed-off-by: Steve French <stfrench@microsoft.com>
> > > ---
> > >  fs/cifs/xattr.c | 28 ++++++++++++++++++++++++++--
> > >  1 file changed, 26 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
> > > index 316af84674f1..50ddb795aaeb 100644
> > > --- a/fs/cifs/xattr.c
> > > +++ b/fs/cifs/xattr.c
> > > @@ -35,6 +35,14 @@
> > >  #define CIFS_XATTR_CIFS_ACL "system.cifs_acl"
> > >  #define CIFS_XATTR_ATTRIB "cifs.dosattrib"  /* full name:
> > > user.cifs.dosattrib */
> > >  #define CIFS_XATTR_CREATETIME "cifs.creationtime"  /* user.cifs.creationtime */
> > > +/*
> > > + * Although these three are just aliases for the above, need to move away from
> > > + * confusing users and using the 20+ year old term 'cifs' when it is no longer
> > > + * secure, replaced by SMB2 (then even more highly secure SMB3) many years ago
> > > + */
> > > +#define SMB3_XATTR_CIFS_ACL "system.smb3_acl"
> > > +#define SMB3_XATTR_ATTRIB "smb3.dosattrib"  /* full name:
> > > user.smb3.dosattrib */
> > > +#define SMB3_XATTR_CREATETIME "smb3.creationtime"  /* user.smb3.creationtime */
> > >  /* BB need to add server (Samba e.g) support for security and trusted prefix */
> > >
> > >  enum { XATTR_USER, XATTR_CIFS_ACL, XATTR_ACL_ACCESS, XATTR_ACL_DEFAULT };
> > > @@ -220,10 +228,12 @@ static int cifs_xattr_get(const struct
> > > xattr_handler *handler,
> > >      switch (handler->flags) {
> > >      case XATTR_USER:
> > >          cifs_dbg(FYI, "%s:querying user xattr %s\n", __func__, name);
> > > -        if (strcmp(name, CIFS_XATTR_ATTRIB) == 0) {
> > > +        if ((strcmp(name, CIFS_XATTR_ATTRIB) == 0) ||
> > > +            (strcmp(name, SMB3_XATTR_ATTRIB) == 0)) {
> > >              rc = cifs_attrib_get(dentry, inode, value, size);
> > >              break;
> > > -        } else if (strcmp(name, CIFS_XATTR_CREATETIME) == 0) {
> > > +        } else if ((strcmp(name, CIFS_XATTR_CREATETIME) == 0) ||
> > > +            (strcmp(name, SMB3_XATTR_CREATETIME) == 0)) {
> > >              rc = cifs_creation_time_get(dentry, inode, value, size);
> > >              break;
> > >          }
> > > @@ -363,6 +373,19 @@ static const struct xattr_handler
> > > cifs_cifs_acl_xattr_handler = {
> > >      .set = cifs_xattr_set,
> > >  };
> > >
> > > +/*
> > > + * Although this is just an alias for the above, need to move away from
> > > + * confusing users and using the 20 year old term 'cifs' when it is no
> > > + * longer secure and was replaced by SMB2/SMB3 a long time ago, and
> > > + * SMB3 and later are highly secure.
> > > + */
> > > +static const struct xattr_handler smb3_acl_xattr_handler = {
> > > +    .name = SMB3_XATTR_CIFS_ACL,
> > > +    .flags = XATTR_CIFS_ACL,
> > > +    .get = cifs_xattr_get,
> > > +    .set = cifs_xattr_set,
> > > +};
> > > +
> > >  static const struct xattr_handler cifs_posix_acl_access_xattr_handler = {
> > >      .name = XATTR_NAME_POSIX_ACL_ACCESS,
> > >      .flags = XATTR_ACL_ACCESS,
> > > @@ -381,6 +404,7 @@ const struct xattr_handler *cifs_xattr_handlers[] = {
> > >      &cifs_user_xattr_handler,
> > >      &cifs_os2_xattr_handler,
> > >      &cifs_cifs_acl_xattr_handler,
> > > +    &smb3_acl_xattr_handler, /* alias for above since avoiding "cifs" */
> > >      &cifs_posix_acl_access_xattr_handler,
> > >      &cifs_posix_acl_default_xattr_handler,
> > >      NULL
> > >
> > > --
> > > Thanks,
> > >
> > > Steve
>
>
>
> --
> Thanks,
>
> Steve
Aurélien Aptel Aug. 13, 2018, 7:44 a.m. | #4
Steve French <smfrench@gmail.com> writes:
> This is an alias not a change to the name.   We don't want to remove
> the 'cifs' names for years if there is a risk of breaking apps - but
> we might as well starting moving them now.

Are we going to do this for every new smb version? We could also keep it
version neutral by updating "cifs" to just "smb".

Cheers,
Steve French Aug. 13, 2018, 2:30 p.m. | #5
On Mon, Aug 13, 2018 at 2:44 AM Aurélien Aptel <aaptel@suse.com> wrote:
>
> Steve French <smfrench@gmail.com> writes:
> > This is an alias not a change to the name.   We don't want to remove
> > the 'cifs' names for years if there is a risk of breaking apps - but
> > we might as well starting moving them now.
>
> Are we going to do this for every new smb version? We could also keep it
> version neutral by updating "cifs" to just "smb".

We could add versions if that helped make it less confusing for users,
but the main motivation here is how to deal with the confusion of
users being told 'do not use cifs'  (the dialect, due to 20 year old
security) and getting confused when we tell them to use commands
with 'cifs' in them.   Using 'SMB' on the other hand may be ok, except
SMB (SMB1) was invented in 1984 and is not really very
closely related to SMB3 and later, which share common characteristics
and are highly secure (SMB3, SMB3.02 SMB3.1.1), and some admins
may think of SMB = SMB1 = 'even more insecure, don't ever, ever use
that on a public network'
while SMB3 is highly secure.
David Disseldorp Aug. 13, 2018, 2:39 p.m. | #6
On Mon, 13 Aug 2018 09:30:14 -0500, Steve French wrote:

> We could add versions if that helped make it less confusing for users,
> but the main motivation here is how to deal with the confusion of
> users being told 'do not use cifs'  (the dialect, due to 20 year old
> security) and getting confused when we tell them to use commands
> with 'cifs' in them. 

Isn't that confusion already encountered once the user needs to run
"mount -t cifs"?

> Using 'SMB' on the other hand may be ok, except
> SMB (SMB1) was invented in 1984 and is not really very
> closely related to SMB3 and later, which share common characteristics
> and are highly secure (SMB3, SMB3.02 SMB3.1.1), and some admins
> may think of SMB = SMB1 = 'even more insecure, don't ever, ever use
> that on a public network'
> while SMB3 is highly secure.

My preference would be to keep the original pseudo-xattr names until the
cifs.ko kernel module is (if ever) renamed. There are bound to be many
many other places where "cifs" is used, and renaming bit-by-bit doesn't
make much sense IMO.

Cheers, David
Steve French Aug. 13, 2018, 2:47 p.m. | #7
On Mon, Aug 13, 2018 at 9:39 AM David Disseldorp <ddiss@suse.de> wrote:
>
> On Mon, 13 Aug 2018 09:30:14 -0500, Steve French wrote:
>
> > We could add versions if that helped make it less confusing for users,
> > but the main motivation here is how to deal with the confusion of
> > users being told 'do not use cifs'  (the dialect, due to 20 year old
> > security) and getting confused when we tell them to use commands
> > with 'cifs' in them.
>
> Isn't that confusion already encountered once the user needs to run
> "mount -t cifs"?

No, some are now recommending to use smb3 for these (as vers=1.0
is disabled when you use smb3 in the mount and modprobe commands)

mount -t smb3 ... already works, as does insmod smb3

See below e.g.

root@smf-Thinkpad-P51:~/cifs-2.6# modprobe smb3
root@smf-Thinkpad-P51:~/cifs-2.6# mount -t smb3 //127.0.0.1/scratch
/mnt -o username=testuser,password=testpass
root@smf-Thinkpad-P51:~/cifs-2.6#

I do agree that we could fork cifs and smb3 modules but that is a lot more
work for less benefit (and may actually be more work to maintain two modules
rather than one with some code duplication).   Note that ext2/3/4 and
nfsv3/nfsv4 had similar issues
and used some of the same naming tricks for module names and mount.
Steve French Aug. 13, 2018, 2:56 p.m. | #8
On Mon, Aug 13, 2018 at 9:39 AM David Disseldorp <ddiss@suse.de> wrote:

> My preference would be to keep the original pseudo-xattr names until the
> cifs.ko kernel module is (if ever) renamed. There are bound to be many
> many other places where "cifs" is used, and renaming bit-by-bit doesn't
> make much sense IMO.

I think there are fewer than you might think - the remaining confusing
'cifs' names are mostly the two user space tools (cifsacl for example),
and i am less concerned about that (distros can always fix that via
a symlink if they keep getting pressure to disable everything
relating to insecure 'cifs').

For the kernel, not sure if we have any remaining cases where
you have to use 'cifs' (the name) since modprobe and mount
work as expected (using 'smb3' prevents using vers=1.0,
but is otherwise the same)

There is precedent here in kernel code with aliases.
David Disseldorp Aug. 13, 2018, 2:59 p.m. | #9
On Mon, 13 Aug 2018 09:56:12 -0500, Steve French wrote:

> There is precedent here in kernel code with aliases.

Okay, sorry for the noise. In that case I guess this change makes sense,
though I share Aurel's concern about new protocol versions requiring
yet more aliasing.

Cheers, David
Tom Talpey Aug. 13, 2018, 3:40 p.m. | #10
> -----Original Message-----
> From: linux-cifs-owner@vger.kernel.org <linux-cifs-owner@vger.kernel.org> On
> Behalf Of David Disseldorp
> Sent: Monday, August 13, 2018 10:59 AM
> To: Steve French <smfrench@gmail.com>
> Cc: Aurélien Aptel <aaptel@suse.com>; ronnie sahlberg
> <ronniesahlberg@gmail.com>; CIFS <linux-cifs@vger.kernel.org>
> Subject: Re: [PATCH] smb3: create smb3 equivalent alias for cifs pseudo-xattrs
> 
> On Mon, 13 Aug 2018 09:56:12 -0500, Steve French wrote:
> 
> > There is precedent here in kernel code with aliases.
> 
> Okay, sorry for the noise. In that case I guess this change makes sense,
> though I share Aurel's concern about new protocol versions requiring
> yet more aliasing.

The SMB3 dialect version is unlikely to change in future, because the protocol
now has negotiate and treeconnect "contexts". Since 3.1.1, these contexts have
been seamlessly extending the protocol, as opposed to the earlier approach of
dialect revision flag days.

There may be other reasons for aliasing the protocol name in the Linux utils,
but don't be too concerned about having to do it for new dialects.

Tom.

Patch

From c4f7173ac3b7e22934e51f0121833642a581d723 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Fri, 10 Aug 2018 18:46:58 -0500
Subject: [PATCH] smb3: create smb3 equivalent alias for cifs pseudo-xattrs

We really, really don't want to be encouraging people to use
cifs (the dialect) since it is insecure, so to avoid confusion
we want to move them to names which include 'smb3' instead of
'cifs' - so this simply creates an alias for the pseudo-xattrs

e.g. can now do:
getfattr -n user.smb3.creationtime /mnt1/file
and
getfattr -n user.smb3.dosattrib /mnt1/file
and
getfattr -n system.smb3_acl /mnt1/file

instead of forcing you to use the string 'cifs' in
these (e.g. getfattr -n system.cifs_acl /mnt1/file)

Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/xattr.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
index 316af84674f1..50ddb795aaeb 100644
--- a/fs/cifs/xattr.c
+++ b/fs/cifs/xattr.c
@@ -35,6 +35,14 @@ 
 #define CIFS_XATTR_CIFS_ACL "system.cifs_acl"
 #define CIFS_XATTR_ATTRIB "cifs.dosattrib"  /* full name: user.cifs.dosattrib */
 #define CIFS_XATTR_CREATETIME "cifs.creationtime"  /* user.cifs.creationtime */
+/*
+ * Although these three are just aliases for the above, need to move away from
+ * confusing users and using the 20+ year old term 'cifs' when it is no longer
+ * secure, replaced by SMB2 (then even more highly secure SMB3) many years ago
+ */
+#define SMB3_XATTR_CIFS_ACL "system.smb3_acl"
+#define SMB3_XATTR_ATTRIB "smb3.dosattrib"  /* full name: user.smb3.dosattrib */
+#define SMB3_XATTR_CREATETIME "smb3.creationtime"  /* user.smb3.creationtime */
 /* BB need to add server (Samba e.g) support for security and trusted prefix */
 
 enum { XATTR_USER, XATTR_CIFS_ACL, XATTR_ACL_ACCESS, XATTR_ACL_DEFAULT };
@@ -220,10 +228,12 @@  static int cifs_xattr_get(const struct xattr_handler *handler,
 	switch (handler->flags) {
 	case XATTR_USER:
 		cifs_dbg(FYI, "%s:querying user xattr %s\n", __func__, name);
-		if (strcmp(name, CIFS_XATTR_ATTRIB) == 0) {
+		if ((strcmp(name, CIFS_XATTR_ATTRIB) == 0) ||
+		    (strcmp(name, SMB3_XATTR_ATTRIB) == 0)) {
 			rc = cifs_attrib_get(dentry, inode, value, size);
 			break;
-		} else if (strcmp(name, CIFS_XATTR_CREATETIME) == 0) {
+		} else if ((strcmp(name, CIFS_XATTR_CREATETIME) == 0) ||
+		    (strcmp(name, SMB3_XATTR_CREATETIME) == 0)) {
 			rc = cifs_creation_time_get(dentry, inode, value, size);
 			break;
 		}
@@ -363,6 +373,19 @@  static const struct xattr_handler cifs_cifs_acl_xattr_handler = {
 	.set = cifs_xattr_set,
 };
 
+/*
+ * Although this is just an alias for the above, need to move away from
+ * confusing users and using the 20 year old term 'cifs' when it is no
+ * longer secure and was replaced by SMB2/SMB3 a long time ago, and
+ * SMB3 and later are highly secure.
+ */
+static const struct xattr_handler smb3_acl_xattr_handler = {
+	.name = SMB3_XATTR_CIFS_ACL,
+	.flags = XATTR_CIFS_ACL,
+	.get = cifs_xattr_get,
+	.set = cifs_xattr_set,
+};
+
 static const struct xattr_handler cifs_posix_acl_access_xattr_handler = {
 	.name = XATTR_NAME_POSIX_ACL_ACCESS,
 	.flags = XATTR_ACL_ACCESS,
@@ -381,6 +404,7 @@  const struct xattr_handler *cifs_xattr_handlers[] = {
 	&cifs_user_xattr_handler,
 	&cifs_os2_xattr_handler,
 	&cifs_cifs_acl_xattr_handler,
+	&smb3_acl_xattr_handler, /* alias for above since avoiding "cifs" */
 	&cifs_posix_acl_access_xattr_handler,
 	&cifs_posix_acl_default_xattr_handler,
 	NULL
-- 
2.17.1