2020-11-06 14:42:33

by Olga Kornievskaia

[permalink] [raw]
Subject: [PATCH v3 1/1] NFSv4.2: condition READDIR's mask for security label based on LSM state

From: Olga Kornievskaia <[email protected]>

Currently, the client will always ask for security_labels if the server
returns that it supports that feature regardless of any LSM modules
(such as Selinux) enforcing security policy. This adds performance
penalty to the READDIR operation.

Client adjusts superblock's support of the security_label based on
the server's support but also current client's configuration of the
LSM modules. Thus, prior to using the default bitmask in READDIR,
this patch checks the server's capabilities and then instructs
READDIR to remove FATTR4_WORD2_SECURITY_LABEL from the bitmask.

v3: changing label's initialization per Ondrej's comment
v2: dropping selinux hook and using the sb cap.

Suggested-by: Ondrej Mosnacek <[email protected]>
Suggested-by: Scott Mayhew <[email protected]>
Signed-off-by: Olga Kornievskaia <[email protected]>
---
fs/nfs/nfs4proc.c | 2 ++
fs/nfs/nfs4xdr.c | 3 ++-
include/linux/nfs_xdr.h | 1 +
3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 9e0ca9b2b210..d85c78657ef4 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -4968,6 +4968,8 @@ static int _nfs4_proc_readdir(struct dentry *dentry, const struct cred *cred,
.count = count,
.bitmask = NFS_SERVER(d_inode(dentry))->attr_bitmask,
.plus = plus,
+ .labels = !!(NFS_SB(dentry->d_sb)->caps &
+ NFS_CAP_SECURITY_LABEL),
};
struct nfs4_readdir_res res;
struct rpc_message msg = {
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index c6dbfcae7517..585d5b5cc3dc 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -1605,7 +1605,8 @@ static void encode_readdir(struct xdr_stream *xdr, const struct nfs4_readdir_arg
FATTR4_WORD1_OWNER_GROUP|FATTR4_WORD1_RAWDEV|
FATTR4_WORD1_SPACE_USED|FATTR4_WORD1_TIME_ACCESS|
FATTR4_WORD1_TIME_METADATA|FATTR4_WORD1_TIME_MODIFY;
- attrs[2] |= FATTR4_WORD2_SECURITY_LABEL;
+ if (readdir->labels)
+ attrs[2] |= FATTR4_WORD2_SECURITY_LABEL;
dircount >>= 1;
}
/* Use mounted_on_fileid only if the server supports it */
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index d63cb862d58e..95f648b26525 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -1119,6 +1119,7 @@ struct nfs4_readdir_arg {
unsigned int pgbase; /* zero-copy data */
const u32 * bitmask;
bool plus;
+ bool labels;
};

struct nfs4_readdir_res {
--
2.18.2


2020-11-06 15:39:19

by Trond Myklebust

[permalink] [raw]
Subject: Re: [PATCH v3 1/1] NFSv4.2: condition READDIR's mask for security label based on LSM state

On Fri, 2020-11-06 at 09:41 -0500, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <[email protected]>
>
> Currently, the client will always ask for security_labels if the
> server
> returns that it supports that feature regardless of any LSM modules
> (such as Selinux) enforcing security policy. This adds performance
> penalty to the READDIR operation.
>
> Client adjusts superblock's support of the security_label based on
> the server's support but also current client's configuration of the
> LSM modules. Thus, prior to using the default bitmask in READDIR,
> this patch checks the server's capabilities and then instructs
> READDIR to remove FATTR4_WORD2_SECURITY_LABEL from the bitmask.
>
> v3: changing label's initialization per Ondrej's comment
> v2: dropping selinux hook and using the sb cap.
>
> Suggested-by: Ondrej Mosnacek <[email protected]>
> Suggested-by: Scott Mayhew <[email protected]>
> Signed-off-by: Olga Kornievskaia <[email protected]>
> ---
>  fs/nfs/nfs4proc.c       | 2 ++
>  fs/nfs/nfs4xdr.c        | 3 ++-
>  include/linux/nfs_xdr.h | 1 +
>  3 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 9e0ca9b2b210..d85c78657ef4 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -4968,6 +4968,8 @@ static int _nfs4_proc_readdir(struct dentry
> *dentry, const struct cred *cred,
>                 .count = count,
>                 .bitmask = NFS_SERVER(d_inode(dentry))->attr_bitmask,
>                 .plus = plus,
> +               .labels = !!(NFS_SB(dentry->d_sb)->caps &
> +                               NFS_CAP_SECURITY_LABEL),

Since both bitmask and labels dereference the same 'struct nfs_server'
(although one does it through NFS_SERVER() and the other uses NFS_SB())
then why not add a 'struct nfs_server *server' variable for both to
use?

>         };
>         struct nfs4_readdir_res res;
>         struct rpc_message msg = {
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index c6dbfcae7517..585d5b5cc3dc 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -1605,7 +1605,8 @@ static void encode_readdir(struct xdr_stream
> *xdr, const struct nfs4_readdir_arg
>                         FATTR4_WORD1_OWNER_GROUP|FATTR4_WORD1_RAWDEV|
>                         FATTR4_WORD1_SPACE_USED|FATTR4_WORD1_TIME_ACC
> ESS|
>                         FATTR4_WORD1_TIME_METADATA|FATTR4_WORD1_TIME_
> MODIFY;
> -               attrs[2] |= FATTR4_WORD2_SECURITY_LABEL;
> +               if (readdir->labels)
> +                       attrs[2] |= FATTR4_WORD2_SECURITY_LABEL;
>                 dircount >>= 1;
>         }
>         /* Use mounted_on_fileid only if the server supports it */
> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> index d63cb862d58e..95f648b26525 100644
> --- a/include/linux/nfs_xdr.h
> +++ b/include/linux/nfs_xdr.h
> @@ -1119,6 +1119,7 @@ struct nfs4_readdir_arg {
>         unsigned int                    pgbase; /* zero-copy data */
>         const u32 *                     bitmask;
>         bool                            plus;
> +       bool                            labels;
>  };
>  
>  struct nfs4_readdir_res {

--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]