diff mbox series

[v2,1/1] NFSv4.2: condition READDIR's mask for security label based on LSM state

Message ID 20201105221245.2838-1-olga.kornievskaia@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v2,1/1] NFSv4.2: condition READDIR's mask for security label based on LSM state | expand

Commit Message

Olga Kornievskaia Nov. 5, 2020, 10:12 p.m. UTC 
From: Olga Kornievskaia <kolga@netapp.com>

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 the superblock's support of the security_label based on
the server's support but also on the current client's configuration of
the LSM module(s). 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.

v2: dropping selinux hook and using the sb cap.

Suggested-by: Ondrej Mosnacek <omosnace@redhat.com
Suggested-by: Scott Mayhew <smayhew@redhat.com>
Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
---
 fs/nfs/nfs4proc.c       | 3 +++
 fs/nfs/nfs4xdr.c        | 3 ++-
 include/linux/nfs_xdr.h | 1 +
 3 files changed, 6 insertions(+), 1 deletion(-)

Comments

Ondrej Mosnacek Nov. 5, 2020, 10:37 p.m. UTC | #1 
On Thu, Nov 5, 2020 at 11:12 PM Olga Kornievskaia
<olga.kornievskaia@gmail.com> wrote:
> From: Olga Kornievskaia <kolga@netapp.com>
>
> 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 the superblock's support of the security_label based on
> the server's support but also on the current client's configuration of
> the LSM module(s). 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.
>
> v2: dropping selinux hook and using the sb cap.

Thanks! I'm happy that we managed to avoid adding yet another LSM hook
in the end :)

>
> Suggested-by: Ondrej Mosnacek <omosnace@redhat.com
> Suggested-by: Scott Mayhew <smayhew@redhat.com>
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfs/nfs4proc.c       | 3 +++
>  fs/nfs/nfs4xdr.c        | 3 ++-
>  include/linux/nfs_xdr.h | 1 +
>  3 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 9e0ca9b2b210..9c9d9aa2a8f8 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -4968,6 +4968,7 @@ 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 = true,
>         };
>         struct nfs4_readdir_res res;
>         struct rpc_message msg = {
> @@ -4981,6 +4982,8 @@ static int _nfs4_proc_readdir(struct dentry *dentry, const struct cred *cred,
>         dprintk("%s: dentry = %pd2, cookie = %Lu\n", __func__,
>                         dentry,
>                         (unsigned long long)cookie);
> +       if (!(NFS_SB(dentry->d_sb)->caps & NFS_CAP_SECURITY_LABEL))
> +               args.labels = false;

Minor nit: you could initialize .labels directly in the initializer of
"args" like this:

.labels = !!(NFS_SB(dentry->d_sb)->caps & NFS_CAP_SECURITY_LABEL),

>         nfs4_setup_readdir(cookie, NFS_I(dir)->cookieverf, dentry, &args);
>         res.pgbase = args.pgbase;
>         status = nfs4_call_sync(NFS_SERVER(dir)->client, NFS_SERVER(dir), &msg, &args.seq_args, &res.seq_res, 0);
> 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
>

--
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.
Olga Kornievskaia Nov. 6, 2020, 2:41 p.m. UTC | #2 
On Thu, Nov 5, 2020 at 5:37 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Thu, Nov 5, 2020 at 11:12 PM Olga Kornievskaia
> <olga.kornievskaia@gmail.com> wrote:
> > From: Olga Kornievskaia <kolga@netapp.com>
> >
> > 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 the superblock's support of the security_label based on
> > the server's support but also on the current client's configuration of
> > the LSM module(s). 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.
> >
> > v2: dropping selinux hook and using the sb cap.
>
> Thanks! I'm happy that we managed to avoid adding yet another LSM hook
> in the end :)
>
> >
> > Suggested-by: Ondrej Mosnacek <omosnace@redhat.com
> > Suggested-by: Scott Mayhew <smayhew@redhat.com>
> > Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> > ---
> >  fs/nfs/nfs4proc.c       | 3 +++
> >  fs/nfs/nfs4xdr.c        | 3 ++-
> >  include/linux/nfs_xdr.h | 1 +
> >  3 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index 9e0ca9b2b210..9c9d9aa2a8f8 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -4968,6 +4968,7 @@ 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 = true,
> >         };
> >         struct nfs4_readdir_res res;
> >         struct rpc_message msg = {
> > @@ -4981,6 +4982,8 @@ static int _nfs4_proc_readdir(struct dentry *dentry, const struct cred *cred,
> >         dprintk("%s: dentry = %pd2, cookie = %Lu\n", __func__,
> >                         dentry,
> >                         (unsigned long long)cookie);
> > +       if (!(NFS_SB(dentry->d_sb)->caps & NFS_CAP_SECURITY_LABEL))
> > +               args.labels = false;
>
> Minor nit: you could initialize .labels directly in the initializer of
> "args" like this:
>
> .labels = !!(NFS_SB(dentry->d_sb)->caps & NFS_CAP_SECURITY_LABEL),

Thank you. v3 will have it.

>
> >         nfs4_setup_readdir(cookie, NFS_I(dir)->cookieverf, dentry, &args);
> >         res.pgbase = args.pgbase;
> >         status = nfs4_call_sync(NFS_SERVER(dir)->client, NFS_SERVER(dir), &msg, &args.seq_args, &res.seq_res, 0);
> > 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
> >
>
> --
> Ondrej Mosnacek
> Software Engineer, Platform Security - SELinux kernel
> Red Hat, Inc.
>
diff mbox series

Patch

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 9e0ca9b2b210..9c9d9aa2a8f8 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -4968,6 +4968,7 @@  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 = true,
 	};
 	struct nfs4_readdir_res res;
 	struct rpc_message msg = {
@@ -4981,6 +4982,8 @@  static int _nfs4_proc_readdir(struct dentry *dentry, const struct cred *cred,
 	dprintk("%s: dentry = %pd2, cookie = %Lu\n", __func__,
 			dentry,
 			(unsigned long long)cookie);
+	if (!(NFS_SB(dentry->d_sb)->caps & NFS_CAP_SECURITY_LABEL))
+		args.labels = false;
 	nfs4_setup_readdir(cookie, NFS_I(dir)->cookieverf, dentry, &args);
 	res.pgbase = args.pgbase;
 	status = nfs4_call_sync(NFS_SERVER(dir)->client, NFS_SERVER(dir), &msg, &args.seq_args, &res.seq_res, 0);
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 {