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.
Instead, query the LSM module to find if anything is enabled and
if not, then remove FATTR4_WORD2_SECURITY_LABEL from the bitmask.
Suggested-by: Scott Mayhew <[email protected]>
Signed-off-by: Olga Kornievskaia <[email protected]>
---
fs/nfs/nfs4proc.c | 5 +++++
fs/nfs/nfs4xdr.c | 3 ++-
include/linux/nfs_xdr.h | 1 +
3 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 9e0ca9b2b210..774bc5e63ca7 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -55,6 +55,7 @@
#include <linux/utsname.h>
#include <linux/freezer.h>
#include <linux/iversion.h>
+#include <linux/security.h>
#include "nfs4_fs.h"
#include "delegation.h"
@@ -4968,6 +4969,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 = {
@@ -4977,10 +4979,13 @@ static int _nfs4_proc_readdir(struct dentry *dentry, const struct cred *cred,
.rpc_cred = cred,
};
int status;
+ int sec_flags = LSM_FQUERY_VFS_XATTRS;
dprintk("%s: dentry = %pd2, cookie = %Lu\n", __func__,
dentry,
(unsigned long long)cookie);
+ if (!security_func_query_vfs(sec_flags))
+ 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 {
--
2.18.2
On Thu, Nov 5, 2020 at 6:33 PM Olga Kornievskaia
<[email protected]> 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.
>
> Instead, query the LSM module to find if anything is enabled and
> if not, then remove FATTR4_WORD2_SECURITY_LABEL from the bitmask.
Having spent some time staring at some of the NFS code very recently,
I can't help but suggest: Would it perhaps be enough to decide whether
to ask for labels based on (NFS_SB(dentry->d_sb)->caps &
NFS_CAP_SECURITY_LABEL)? It is set when mounting the FS iff some LSM
confirms via the security_sb_*_mnt_opts() hook that it wants the
filesystem to give it labels (or at least that's how I interpret the
cryptic name) [1]. It's just a shot in the dark, but it seems to fit
this use case.
[1] https://elixir.bootlin.com/linux/v5.10-rc2/source/fs/nfs/getroot.c#L148
>
> Suggested-by: Scott Mayhew <[email protected]>
> Signed-off-by: Olga Kornievskaia <[email protected]>
> ---
> fs/nfs/nfs4proc.c | 5 +++++
> fs/nfs/nfs4xdr.c | 3 ++-
> include/linux/nfs_xdr.h | 1 +
> 3 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 9e0ca9b2b210..774bc5e63ca7 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -55,6 +55,7 @@
> #include <linux/utsname.h>
> #include <linux/freezer.h>
> #include <linux/iversion.h>
> +#include <linux/security.h>
>
> #include "nfs4_fs.h"
> #include "delegation.h"
> @@ -4968,6 +4969,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 = {
> @@ -4977,10 +4979,13 @@ static int _nfs4_proc_readdir(struct dentry *dentry, const struct cred *cred,
> .rpc_cred = cred,
> };
> int status;
> + int sec_flags = LSM_FQUERY_VFS_XATTRS;
>
> dprintk("%s: dentry = %pd2, cookie = %Lu\n", __func__,
> dentry,
> (unsigned long long)cookie);
> + if (!security_func_query_vfs(sec_flags))
> + 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 {
> --
> 2.18.2
>
--
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.
On Thu, Nov 5, 2020 at 1:55 PM Ondrej Mosnacek <[email protected]> wrote:
>
> On Thu, Nov 5, 2020 at 6:33 PM Olga Kornievskaia
> <[email protected]> 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.
> >
> > Instead, query the LSM module to find if anything is enabled and
> > if not, then remove FATTR4_WORD2_SECURITY_LABEL from the bitmask.
>
> Having spent some time staring at some of the NFS code very recently,
> I can't help but suggest: Would it perhaps be enough to decide whether
> to ask for labels based on (NFS_SB(dentry->d_sb)->caps &
> NFS_CAP_SECURITY_LABEL)? It is set when mounting the FS iff some LSM
> confirms via the security_sb_*_mnt_opts() hook that it wants the
> filesystem to give it labels (or at least that's how I interpret the
> cryptic name) [1]. It's just a shot in the dark, but it seems to fit
> this use case.
>
> [1] https://elixir.bootlin.com/linux/v5.10-rc2/source/fs/nfs/getroot.c#L148
Very interesting. I was not aware of something like that nor was it
mentioned when I asked on the selinux mailing list. I wonder if this
is a supported feature that will always stay? In that case, NFS
wouldn't need the extra hook that was added for this series. I will
try this out and report back.
>
> >
> > Suggested-by: Scott Mayhew <[email protected]>
> > Signed-off-by: Olga Kornievskaia <[email protected]>
> > ---
> > fs/nfs/nfs4proc.c | 5 +++++
> > fs/nfs/nfs4xdr.c | 3 ++-
> > include/linux/nfs_xdr.h | 1 +
> > 3 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index 9e0ca9b2b210..774bc5e63ca7 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -55,6 +55,7 @@
> > #include <linux/utsname.h>
> > #include <linux/freezer.h>
> > #include <linux/iversion.h>
> > +#include <linux/security.h>
> >
> > #include "nfs4_fs.h"
> > #include "delegation.h"
> > @@ -4968,6 +4969,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 = {
> > @@ -4977,10 +4979,13 @@ static int _nfs4_proc_readdir(struct dentry *dentry, const struct cred *cred,
> > .rpc_cred = cred,
> > };
> > int status;
> > + int sec_flags = LSM_FQUERY_VFS_XATTRS;
> >
> > dprintk("%s: dentry = %pd2, cookie = %Lu\n", __func__,
> > dentry,
> > (unsigned long long)cookie);
> > + if (!security_func_query_vfs(sec_flags))
> > + 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 {
> > --
> > 2.18.2
> >
>
>
> --
> Ondrej Mosnacek
> Software Engineer, Platform Security - SELinux kernel
> Red Hat, Inc.
>
On Thu, Nov 5, 2020 at 8:51 PM Olga Kornievskaia
<[email protected]> wrote:
>
> On Thu, Nov 5, 2020 at 1:55 PM Ondrej Mosnacek <[email protected]> wrote:
> >
> > On Thu, Nov 5, 2020 at 6:33 PM Olga Kornievskaia
> > <[email protected]> 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.
> > >
> > > Instead, query the LSM module to find if anything is enabled and
> > > if not, then remove FATTR4_WORD2_SECURITY_LABEL from the bitmask.
> >
> > Having spent some time staring at some of the NFS code very recently,
> > I can't help but suggest: Would it perhaps be enough to decide whether
> > to ask for labels based on (NFS_SB(dentry->d_sb)->caps &
> > NFS_CAP_SECURITY_LABEL)? It is set when mounting the FS iff some LSM
> > confirms via the security_sb_*_mnt_opts() hook that it wants the
> > filesystem to give it labels (or at least that's how I interpret the
> > cryptic name) [1]. It's just a shot in the dark, but it seems to fit
> > this use case.
> >
> > [1] https://elixir.bootlin.com/linux/v5.10-rc2/source/fs/nfs/getroot.c#L148
>
> Very interesting. I was not aware of something like that nor was it
> mentioned when I asked on the selinux mailing list. I wonder if this
> is a supported feature that will always stay? In that case, NFS
> wouldn't need the extra hook that was added for this series. I will
> try this out and report back.
I wish I could have suggested it sooner, but I only learned about it
in recent days while investigating a bug that involves SELinux and NFS
:) And when I saw your patch, it immediately reminded me of that flag.
I don't think it's going away, at least as long as NFS depends on it.
If someone were to remove it, they would have to provide something
equivalent to make sure no existing users get broken, as with any
in-kernel change.
--
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.
On Thu, 2020-11-05 at 14:51 -0500, Olga Kornievskaia wrote:
> On Thu, Nov 5, 2020 at 1:55 PM Ondrej Mosnacek <[email protected]>
> wrote:
> >
> > On Thu, Nov 5, 2020 at 6:33 PM Olga Kornievskaia
> > <[email protected]> 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.
> > >
> > > Instead, query the LSM module to find if anything is enabled and
> > > if not, then remove FATTR4_WORD2_SECURITY_LABEL from the bitmask.
> >
> > Having spent some time staring at some of the NFS code very
> > recently,
> > I can't help but suggest: Would it perhaps be enough to decide
> > whether
> > to ask for labels based on (NFS_SB(dentry->d_sb)->caps &
> > NFS_CAP_SECURITY_LABEL)? It is set when mounting the FS iff some
> > LSM
> > confirms via the security_sb_*_mnt_opts() hook that it wants the
> > filesystem to give it labels (or at least that's how I interpret
> > the
> > cryptic name) [1]. It's just a shot in the dark, but it seems to
> > fit
> > this use case.
> >
> > [1]
> > https://elixir.bootlin.com/linux/v5.10-rc2/source/fs/nfs/getroot.c#L148
>
> Very interesting. I was not aware of something like that nor was it
> mentioned when I asked on the selinux mailing list. I wonder if this
> is a supported feature that will always stay? In that case, NFS
> wouldn't need the extra hook that was added for this series. I will
> try this out and report back.
NFS_CAP_SECURITY_LABEL is just the NFS server capability flag. It tells
you whether or not the client believes that the server might support
NFSv4.2 requests for labelled NFS metadata.
My understanding of Olga's requirement is that she needs to be able to
ignore that flag and simply not query for labelled NFS metadata if the
NFS client is not configured to enforce the LSM policy. The objective
is to avoid unnecessary RPC traffic to the server to query for data
that is unused.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
On Thu, Nov 5, 2020 at 4:18 PM Trond Myklebust <[email protected]> wrote:
>
> On Thu, 2020-11-05 at 14:51 -0500, Olga Kornievskaia wrote:
> > On Thu, Nov 5, 2020 at 1:55 PM Ondrej Mosnacek <[email protected]>
> > wrote:
> > >
> > > On Thu, Nov 5, 2020 at 6:33 PM Olga Kornievskaia
> > > <[email protected]> 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.
> > > >
> > > > Instead, query the LSM module to find if anything is enabled and
> > > > if not, then remove FATTR4_WORD2_SECURITY_LABEL from the bitmask.
> > >
> > > Having spent some time staring at some of the NFS code very
> > > recently,
> > > I can't help but suggest: Would it perhaps be enough to decide
> > > whether
> > > to ask for labels based on (NFS_SB(dentry->d_sb)->caps &
> > > NFS_CAP_SECURITY_LABEL)? It is set when mounting the FS iff some
> > > LSM
> > > confirms via the security_sb_*_mnt_opts() hook that it wants the
> > > filesystem to give it labels (or at least that's how I interpret
> > > the
> > > cryptic name) [1]. It's just a shot in the dark, but it seems to
> > > fit
> > > this use case.
> > >
> > > [1]
> > > https://elixir.bootlin.com/linux/v5.10-rc2/source/fs/nfs/getroot.c#L148
> >
> > Very interesting. I was not aware of something like that nor was it
> > mentioned when I asked on the selinux mailing list. I wonder if this
> > is a supported feature that will always stay? In that case, NFS
> > wouldn't need the extra hook that was added for this series. I will
> > try this out and report back.
>
> NFS_CAP_SECURITY_LABEL is just the NFS server capability flag. It tells
> you whether or not the client believes that the server might support
> NFSv4.2 requests for labelled NFS metadata.
>
> My understanding of Olga's requirement is that she needs to be able to
> ignore that flag and simply not query for labelled NFS metadata if the
> NFS client is not configured to enforce the LSM policy. The objective
> is to avoid unnecessary RPC traffic to the server to query for data
> that is unused.
Actually that seems to be working. I think it's because while the
server returns that it supports sec_labels, after calling
security_sb_set_mnt_opts() the kflags_out don't have this
SECURITY_LSM_NATIVE_LABELS set (assuming this happens because selinux
isn't enabled) then we turned server's sec_labl cap off.
I'm about to send v2 without relying on modifications to selinux.
>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> [email protected]
>
>
Hi Olga,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on nfs/linux-next]
[also build test ERROR on pcmoore-selinux/next linus/master security/next-testing v5.10-rc2 next-20201105]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Olga-Kornievskaia/introduce-a-new-hook-to-query-LSM-for-functionality/20201106-013417
base: git://git.linux-nfs.org/projects/trondmy/linux-nfs.git linux-next
config: openrisc-randconfig-r002-20201104 (attached as .config)
compiler: or1k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/d765c00ede01a334b7a3f995ab27b8d4ebd5ea38
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Olga-Kornievskaia/introduce-a-new-hook-to-query-LSM-for-functionality/20201106-013417
git checkout d765c00ede01a334b7a3f995ab27b8d4ebd5ea38
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=openrisc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
fs/nfs/nfs4proc.c: In function '_nfs4_proc_readdir':
>> fs/nfs/nfs4proc.c:4982:18: error: 'LSM_FQUERY_VFS_XATTRS' undeclared (first use in this function)
4982 | int sec_flags = LSM_FQUERY_VFS_XATTRS;
| ^~~~~~~~~~~~~~~~~~~~~
fs/nfs/nfs4proc.c:4982:18: note: each undeclared identifier is reported only once for each function it appears in
>> fs/nfs/nfs4proc.c:4987:7: error: implicit declaration of function 'security_func_query_vfs' [-Werror=implicit-function-declaration]
4987 | if (!security_func_query_vfs(sec_flags))
| ^~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/LSM_FQUERY_VFS_XATTRS +4982 fs/nfs/nfs4proc.c
4960
4961 static int _nfs4_proc_readdir(struct dentry *dentry, const struct cred *cred,
4962 u64 cookie, struct page **pages, unsigned int count, bool plus)
4963 {
4964 struct inode *dir = d_inode(dentry);
4965 struct nfs4_readdir_arg args = {
4966 .fh = NFS_FH(dir),
4967 .pages = pages,
4968 .pgbase = 0,
4969 .count = count,
4970 .bitmask = NFS_SERVER(d_inode(dentry))->attr_bitmask,
4971 .plus = plus,
4972 .labels = true,
4973 };
4974 struct nfs4_readdir_res res;
4975 struct rpc_message msg = {
4976 .rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_READDIR],
4977 .rpc_argp = &args,
4978 .rpc_resp = &res,
4979 .rpc_cred = cred,
4980 };
4981 int status;
> 4982 int sec_flags = LSM_FQUERY_VFS_XATTRS;
4983
4984 dprintk("%s: dentry = %pd2, cookie = %Lu\n", __func__,
4985 dentry,
4986 (unsigned long long)cookie);
> 4987 if (!security_func_query_vfs(sec_flags))
4988 args.labels = false;
4989 nfs4_setup_readdir(cookie, NFS_I(dir)->cookieverf, dentry, &args);
4990 res.pgbase = args.pgbase;
4991 status = nfs4_call_sync(NFS_SERVER(dir)->client, NFS_SERVER(dir), &msg, &args.seq_args, &res.seq_res, 0);
4992 if (status >= 0) {
4993 memcpy(NFS_I(dir)->cookieverf, res.verifier.data, NFS4_VERIFIER_SIZE);
4994 status += args.pgbase;
4995 }
4996
4997 nfs_invalidate_atime(dir);
4998
4999 dprintk("%s: returns %d\n", __func__, status);
5000 return status;
5001 }
5002
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Thu, Nov 5, 2020 at 10:43 PM Olga Kornievskaia
<[email protected]> wrote:
> On Thu, Nov 5, 2020 at 4:18 PM Trond Myklebust <[email protected]> wrote:
> >
> > On Thu, 2020-11-05 at 14:51 -0500, Olga Kornievskaia wrote:
> > > On Thu, Nov 5, 2020 at 1:55 PM Ondrej Mosnacek <[email protected]>
> > > wrote:
> > > >
> > > > On Thu, Nov 5, 2020 at 6:33 PM Olga Kornievskaia
> > > > <[email protected]> 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.
> > > > >
> > > > > Instead, query the LSM module to find if anything is enabled and
> > > > > if not, then remove FATTR4_WORD2_SECURITY_LABEL from the bitmask.
> > > >
> > > > Having spent some time staring at some of the NFS code very
> > > > recently,
> > > > I can't help but suggest: Would it perhaps be enough to decide
> > > > whether
> > > > to ask for labels based on (NFS_SB(dentry->d_sb)->caps &
> > > > NFS_CAP_SECURITY_LABEL)? It is set when mounting the FS iff some
> > > > LSM
> > > > confirms via the security_sb_*_mnt_opts() hook that it wants the
> > > > filesystem to give it labels (or at least that's how I interpret
> > > > the
> > > > cryptic name) [1]. It's just a shot in the dark, but it seems to
> > > > fit
> > > > this use case.
> > > >
> > > > [1]
> > > > https://elixir.bootlin.com/linux/v5.10-rc2/source/fs/nfs/getroot.c#L148
> > >
> > > Very interesting. I was not aware of something like that nor was it
> > > mentioned when I asked on the selinux mailing list. I wonder if this
> > > is a supported feature that will always stay? In that case, NFS
> > > wouldn't need the extra hook that was added for this series. I will
> > > try this out and report back.
> >
> > NFS_CAP_SECURITY_LABEL is just the NFS server capability flag. It tells
> > you whether or not the client believes that the server might support
> > NFSv4.2 requests for labelled NFS metadata.
> >
> > My understanding of Olga's requirement is that she needs to be able to
> > ignore that flag and simply not query for labelled NFS metadata if the
> > NFS client is not configured to enforce the LSM policy. The objective
> > is to avoid unnecessary RPC traffic to the server to query for data
> > that is unused.
>
> Actually that seems to be working. I think it's because while the
> server returns that it supports sec_labels, after calling
> security_sb_set_mnt_opts() the kflags_out don't have this
> SECURITY_LSM_NATIVE_LABELS set (assuming this happens because selinux
> isn't enabled) then we turned server's sec_labl cap off.
>
> I'm about to send v2 without relying on modifications to selinux.
Just to keep the LSM and SELinux lists in the loop: a v2 has been
posted to linux-nfs that uses NFS_CAP_SECURITY_LABEL instead of adding
a new hook:
https://lore.kernel.org/linux-nfs/CAFqZXNtJ2fkae7Xt-+nDR79kVs=yY=9vSoZaS-V-5UKSk22s4A@mail.gmail.com/T/
--
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.