Chao reported the following oops when testing labeled NFS:
BUG: unable to handle kernel NULL pointer dereference at (null)
IP: [<ffffffffa0568703>] nfs4_xdr_enc_setattr+0x43/0x110 [nfsv4]
PGD 277bbd067 PUD 2777ea067 PMD 0
Oops: 0000 [#1] SMP
Modules linked in: rpcsec_gss_krb5 nfsv4 dns_resolver nfs fscache sg coretemp kvm_intel kvm crc32_pclmul crc32c_intel ghash_clmulni_intel aesni_intel lrw gf128mul iTCO_wdt glue_helper ablk_helper cryptd iTCO_vendor_support bnx2 pcspkr serio_raw i7core_edac cdc_ether microcode usbnet edac_core mii lpc_ich i2c_i801 mfd_core shpchp ioatdma dca acpi_cpufreq mperf nfsd auth_rpcgss nfs_acl lockd sunrpc xfs libcrc32c sr_mod sd_mod cdrom crc_t10dif mgag200 syscopyarea sysfillrect sysimgblt i2c_algo_bit drm_kms_helper ata_generic ttm pata_acpi drm ata_piix libata megaraid_sas i2c_core dm_mirror dm_region_hash dm_log dm_mod
CPU: 4 PID: 25657 Comm: chcon Not tainted 3.10.0-33.el7.x86_64 #1
Hardware name: IBM System x3550 M3 -[7944OEJ]-/90Y4784 , BIOS -[D6E150CUS-1.11]- 02/08/2011
task: ffff880178397220 ti: ffff8801595d2000 task.ti: ffff8801595d2000
RIP: 0010:[<ffffffffa0568703>] [<ffffffffa0568703>] nfs4_xdr_enc_setattr+0x43/0x110 [nfsv4]
RSP: 0018:ffff8801595d3888 EFLAGS: 00010296
RAX: 0000000000000000 RBX: ffff8801595d3b30 RCX: 0000000000000b4c
RDX: ffff8801595d3b30 RSI: ffff8801595d38e0 RDI: ffff880278b6ec00
RBP: ffff8801595d38c8 R08: ffff8801595d3b30 R09: 0000000000000001
R10: 0000000000000000 R11: 0000000000000000 R12: ffff8801595d38e0
R13: ffff880277a4a780 R14: ffffffffa05686c0 R15: ffff8802765f206c
FS: 00007f2c68486800(0000) GS:ffff88027fc00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 000000027651a000 CR4: 00000000000007e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Stack:
0000000000000000 0000000000000000 0000000000000000 0000000000000000
0000000000000000 ffff880277865800 ffff880278b6ec00 ffff880277a4a780
ffff8801595d3948 ffffffffa02ad926 ffff8801595d3b30 ffff8802765f206c
Call Trace:
[<ffffffffa02ad926>] rpcauth_wrap_req+0x86/0xd0 [sunrpc]
[<ffffffffa02a1d40>] ? call_connect+0xb0/0xb0 [sunrpc]
[<ffffffffa02a1d40>] ? call_connect+0xb0/0xb0 [sunrpc]
[<ffffffffa02a1ecb>] call_transmit+0x18b/0x290 [sunrpc]
[<ffffffffa02a1d40>] ? call_connect+0xb0/0xb0 [sunrpc]
[<ffffffffa02aae14>] __rpc_execute+0x84/0x400 [sunrpc]
[<ffffffffa02ac40e>] rpc_execute+0x5e/0xa0 [sunrpc]
[<ffffffffa02a2ea0>] rpc_run_task+0x70/0x90 [sunrpc]
[<ffffffffa02a2f03>] rpc_call_sync+0x43/0xa0 [sunrpc]
[<ffffffffa055284d>] _nfs4_do_set_security_label+0x11d/0x170 [nfsv4]
[<ffffffffa0558861>] nfs4_set_security_label.isra.69+0xf1/0x1d0 [nfsv4]
[<ffffffff815fca8b>] ? avc_alloc_node+0x24/0x125
[<ffffffff815fcd2f>] ? avc_compute_av+0x1a3/0x1b5
[<ffffffffa055897b>] nfs4_xattr_set_nfs4_label+0x3b/0x50 [nfsv4]
[<ffffffff811bc772>] generic_setxattr+0x62/0x80
[<ffffffff811bcfc3>] __vfs_setxattr_noperm+0x63/0x1b0
[<ffffffff811bd1c5>] vfs_setxattr+0xb5/0xc0
[<ffffffff811bd2fe>] setxattr+0x12e/0x1c0
[<ffffffff811a4d22>] ? final_putname+0x22/0x50
[<ffffffff811a4f2b>] ? putname+0x2b/0x40
[<ffffffff811aa1cf>] ? user_path_at_empty+0x5f/0x90
[<ffffffff8119bc29>] ? __sb_start_write+0x49/0x100
[<ffffffff811bd66f>] SyS_lsetxattr+0x8f/0xd0
[<ffffffff8160cf99>] system_call_fastpath+0x16/0x1b
Code: 48 8b 02 48 c7 45 c0 00 00 00 00 48 c7 45 c8 00 00 00 00 48 c7 45 d0 00 00 00 00 48 c7 45 d8 00 00 00 00 48 c7 45 e0 00 00 00 00 <48> 8b 00 48 8b 00 48 85 c0 0f 84 ae 00 00 00 48 8b 80 b8 03 00
RIP [<ffffffffa0568703>] nfs4_xdr_enc_setattr+0x43/0x110 [nfsv4]
RSP <ffff8801595d3888>
CR2: 0000000000000000
The problem is that _nfs4_do_set_security_label calls rpc_call_sync()
directly which fails to do any setup of the SEQUENCE call. Have it use
nfs4_call_sync() instead which does the right thing. While we're at it
change the name of "args" to "arg" to better match the pattern in
_nfs4_do_setattr.
Reported-by: Chao Ye <[email protected]>
Cc: David Quigley <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfs/nfs4proc.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index b02c4cc..d436c57 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -4657,7 +4657,7 @@ static int _nfs4_do_set_security_label(struct inode *inode,
struct iattr sattr = {0};
struct nfs_server *server = NFS_SERVER(inode);
const u32 bitmask[3] = { 0, 0, FATTR4_WORD2_SECURITY_LABEL };
- struct nfs_setattrargs args = {
+ struct nfs_setattrargs arg = {
.fh = NFS_FH(inode),
.iap = &sattr,
.server = server,
@@ -4671,14 +4671,14 @@ static int _nfs4_do_set_security_label(struct inode *inode,
};
struct rpc_message msg = {
.rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_SETATTR],
- .rpc_argp = &args,
+ .rpc_argp = &arg,
.rpc_resp = &res,
};
int status;
- nfs4_stateid_copy(&args.stateid, &zero_stateid);
+ nfs4_stateid_copy(&arg.stateid, &zero_stateid);
- status = rpc_call_sync(server->client, &msg, 0);
+ status = nfs4_call_sync(server->client, server, &msg, &arg.seq_args, &res.seq_res, 1);
if (status)
dprintk("%s failed: %d\n", __func__, status);
--
1.8.3.1
On Fri, 2013-11-01 at 12:02 -0400, Jeff Layton wrote:
+AD4- It looks like +AF8-nfs4+AF8-get+AF8-security+AF8-label() has the same problem, but I've
+AD4- so far been unable to get it to be called, so I didn't patch it. It
+AD4- seems like getxattr does some special stuff for SELinux labels that
+AD4- cause them only to ever be fetched once.
+AD4-
+AD4- Is there some trick to it?
+AD4-
Doesn't 'ls -Z' cause them to security label to be read again?
Either way, the fix is clearly needed, so I've added the patch, and Cced
stable.
Cheers
Trond
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust+AEA-netapp.com
http://www.netapp.com
On Fri, 1 Nov 2013 16:50:00 +0000
"Myklebust, Trond" <[email protected]> wrote:
> On Fri, 2013-11-01 at 12:02 -0400, Jeff Layton wrote:
> > It looks like _nfs4_get_security_label() has the same problem, but I've
> > so far been unable to get it to be called, so I didn't patch it. It
> > seems like getxattr does some special stuff for SELinux labels that
> > cause them only to ever be fetched once.
> >
> > Is there some trick to it?
> >
>
> Doesn't 'ls -Z' cause them to security label to be read again?
>
As best I can tell, security labels are set on the inode when the inode
is instantiated, and then are reset on changes (i.e. setxattr). If
another client changes the label though, it's not clear to me how your
client would ever notice it until the inode is dropped from the cache.
ISTR Eric Paris explaining to me that they do that for performance
reasons but it seems like something that needs to be reconsidered in
light of labeled NFS. Not picking up a security label change seems like
a bug, IMO...
> Either way, the fix is clearly needed, so I've added the patch, and Cced
> stable.
>
Thanks!
--
Jeff Layton <[email protected]>
On Fri, 1 Nov 2013 17:47:46 +-0000
+ACI-Myklebust, Trond+ACI +ADw-Trond.Myklebust+AEA-netapp.com+AD4 wrote:
+AD4 On Fri, 2013-11-01 at 13:18 -0400, Jeff Layton wrote:
+AD4 +AD4 On Fri, 1 Nov 2013 17:05:08 +-0000
+AD4 +AD4 +ACI-Myklebust, Trond+ACI +ADw-Trond.Myklebust+AEA-netapp.com+AD4 wrote:
+AD4 +AD4
+AD4 +AD4 +AD4
+AD4 +AD4 +AD4 On Nov 1, 2013, at 12:57, Jeff Layton +ADw-jlayton+AEA-redhat.com+AD4 wrote:
+AD4 +AD4 +AD4
+AD4 +AD4 +AD4 +AD4 On Fri, 1 Nov 2013 16:50:00 +-0000
+AD4 +AD4 +AD4 +AD4 +ACI-Myklebust, Trond+ACI +ADw-Trond.Myklebust+AEA-netapp.com+AD4 wrote:
+AD4 +AD4 +AD4 +AD4
+AD4 +AD4 +AD4 +AD4APg On Fri, 2013-11-01 at 12:02 -0400, Jeff Layton wrote:
+AD4 +AD4 +AD4 +AD4APgA+ It looks like +AF8-nfs4+AF8-get+AF8-security+AF8-label() has the same problem, but I've
+AD4 +AD4 +AD4 +AD4APgA+ so far been unable to get it to be called, so I didn't patch it. It
+AD4 +AD4 +AD4 +AD4APgA+ seems like getxattr does some special stuff for SELinux labels that
+AD4 +AD4 +AD4 +AD4APgA+ cause them only to ever be fetched once.
+AD4 +AD4 +AD4 +AD4APgA+
+AD4 +AD4 +AD4 +AD4APgA+ Is there some trick to it?
+AD4 +AD4 +AD4 +AD4APgA+
+AD4 +AD4 +AD4 +AD4APg
+AD4 +AD4 +AD4 +AD4APg Doesn't 'ls -Z' cause them to security label to be read again?
+AD4 +AD4 +AD4 +AD4APg
+AD4 +AD4 +AD4 +AD4
+AD4 +AD4 +AD4 +AD4 As best I can tell, security labels are set on the inode when the inode
+AD4 +AD4 +AD4 +AD4 is instantiated, and then are reset on changes (i.e. setxattr). If
+AD4 +AD4 +AD4
+AD4 +AD4 +AD4 +ICY-and on getxattr, afaics.
+AD4 +AD4 +AD4
+AD4 +AD4
+AD4 +AD4 I don't see that. The call chain is something like this:
+AD4 +AD4
+AD4 +AD4 vfs+AF8-getxattr
+AD4 +AD4 xattr+AF8-getsecurity
+AD4 +AD4 security+AF8-inode+AF8-getsecurity
+AD4 +AD4 selinux+AF8-inode+AF8-getsecurity
+AD4 +AD4
+AD4 +AD4 ...and that function looks like it just converts the current security
+AD4 +AD4 context on the inode to text and plops that into the buffer.
+AD4
+AD4 Ah, you're right. You have to turn off SELinux in order to hit
+AD4 nfs4+AF8-xattr+AF8-get+AF8-nfs4+AF8-label.
+AD4
+AD4 +AD4 +AD4 +AD4 another client changes the label though, it's not clear to me how your
+AD4 +AD4 +AD4 +AD4 client would ever notice it until the inode is dropped from the cache.
+AD4 +AD4 +AD4 +AD4
+AD4 +AD4 +AD4 +AD4 ISTR Eric Paris explaining to me that they do that for performance
+AD4 +AD4 +AD4 +AD4 reasons but it seems like something that needs to be reconsidered in
+AD4 +AD4 +AD4 +AD4 light of labeled NFS. Not picking up a security label change seems like
+AD4 +AD4 +AD4 +AD4 a bug, IMO...
+AD4 +AD4 +AD4
+AD4 +AD4 +AD4 To be effective, the security label should normally be set at file creation time. It should rarely, if ever, change. Why would you need to change it from a different client?
+AD4 +AD4 +AD4
+AD4 +AD4
+AD4 +AD4 At least in Fedora, there are SELinux policy changes all the time.
+AD4 +AD4 Sometimes that involves changing how files are labeled. I don't think
+AD4 +AD4 it's reasonable to assume that they only get set at creation time.
+AD4 +AD4
+AD4
+AD4 That doesn't answer the question. Again, why would you need to do this
+AD4 from another client? If you don't have a real-life use case, then it's
+AD4 just another 'doctor it hurts' problem. Stop doing it...
+AD4
Ok, how about this then: +ADs)
NFS doesn't have O+AF8-TMPFILE, so you really +ACo-can't+ACo set the context at
creation time, at least with normal syscalls. There will always be a
race window between creating a file and setting the SELinux context on
it.
FWIW, I just started playing with this stuff and this behavior just
gave me pause. This is the sort of thing that will give people fits
since it's unexpected behavior.
When I change something on the server, I typically expect the client to
see that change in a timely fashion. As it stands now, it won't -- at
least until the inode gets purged from the cache.
--
Jeff Layton +ADw-jlayton+AEA-redhat.com+AD4
On Nov 1, 2013, at 12:57, Jeff Layton <[email protected]> wrote:
> On Fri, 1 Nov 2013 16:50:00 +0000
> "Myklebust, Trond" <[email protected]> wrote:
>
>> On Fri, 2013-11-01 at 12:02 -0400, Jeff Layton wrote:
>>> It looks like _nfs4_get_security_label() has the same problem, but I've
>>> so far been unable to get it to be called, so I didn't patch it. It
>>> seems like getxattr does some special stuff for SELinux labels that
>>> cause them only to ever be fetched once.
>>>
>>> Is there some trick to it?
>>>
>>
>> Doesn't 'ls -Z' cause them to security label to be read again?
>>
>
> As best I can tell, security labels are set on the inode when the inode
> is instantiated, and then are reset on changes (i.e. setxattr). If
?and on getxattr, afaics.
> another client changes the label though, it's not clear to me how your
> client would ever notice it until the inode is dropped from the cache.
>
> ISTR Eric Paris explaining to me that they do that for performance
> reasons but it seems like something that needs to be reconsidered in
> light of labeled NFS. Not picking up a security label change seems like
> a bug, IMO...
To be effective, the security label should normally be set at file creation time. It should rarely, if ever, change. Why would you need to change it from a different client?
Cheers,
Trond
On Fri, 1 Nov 2013 17:05:08 +0000
"Myklebust, Trond" <[email protected]> wrote:
>
> On Nov 1, 2013, at 12:57, Jeff Layton <[email protected]> wrote:
>
> > On Fri, 1 Nov 2013 16:50:00 +0000
> > "Myklebust, Trond" <[email protected]> wrote:
> >
> >> On Fri, 2013-11-01 at 12:02 -0400, Jeff Layton wrote:
> >>> It looks like _nfs4_get_security_label() has the same problem, but I've
> >>> so far been unable to get it to be called, so I didn't patch it. It
> >>> seems like getxattr does some special stuff for SELinux labels that
> >>> cause them only to ever be fetched once.
> >>>
> >>> Is there some trick to it?
> >>>
> >>
> >> Doesn't 'ls -Z' cause them to security label to be read again?
> >>
> >
> > As best I can tell, security labels are set on the inode when the inode
> > is instantiated, and then are reset on changes (i.e. setxattr). If
>
> ?and on getxattr, afaics.
>
I don't see that. The call chain is something like this:
vfs_getxattr
xattr_getsecurity
security_inode_getsecurity
selinux_inode_getsecurity
...and that function looks like it just converts the current security
context on the inode to text and plops that into the buffer.
> > another client changes the label though, it's not clear to me how your
> > client would ever notice it until the inode is dropped from the cache.
> >
> > ISTR Eric Paris explaining to me that they do that for performance
> > reasons but it seems like something that needs to be reconsidered in
> > light of labeled NFS. Not picking up a security label change seems like
> > a bug, IMO...
>
> To be effective, the security label should normally be set at file creation time. It should rarely, if ever, change. Why would you need to change it from a different client?
>
At least in Fedora, there are SELinux policy changes all the time.
Sometimes that involves changing how files are labeled. I don't think
it's reasonable to assume that they only get set at creation time.
--
Jeff Layton <[email protected]>
On 11/1/2013 1:05 PM, Myklebust, Trond wrote:
>
> On Nov 1, 2013, at 12:57, Jeff Layton <[email protected]> wrote:
>
>> On Fri, 1 Nov 2013 16:50:00 +0000
>> "Myklebust, Trond" <[email protected]> wrote:
>>
>>> On Fri, 2013-11-01 at 12:02 -0400, Jeff Layton wrote:
>>>> It looks like _nfs4_get_security_label() has the same problem, but I've
>>>> so far been unable to get it to be called, so I didn't patch it. It
>>>> seems like getxattr does some special stuff for SELinux labels that
>>>> cause them only to ever be fetched once.
>>>>
>>>> Is there some trick to it?
>>>>
>>>
>>> Doesn't 'ls -Z' cause them to security label to be read again?
>>>
>>
>> As best I can tell, security labels are set on the inode when the inode
>> is instantiated, and then are reset on changes (i.e. setxattr). If
>
> ?and on getxattr, afaics.
>
>> another client changes the label though, it's not clear to me how your
>> client would ever notice it until the inode is dropped from the cache.
>>
>> ISTR Eric Paris explaining to me that they do that for performance
>> reasons but it seems like something that needs to be reconsidered in
>> light of labeled NFS. Not picking up a security label change seems like
>> a bug, IMO...
>
> To be effective, the security label should normally be set at file creation time. It should rarely, if ever, change. Why would you need to change it from a different client?
>
> Cheers,
> Trond
>
That isn't completely true. Security labels should be set on creation
but they may change. An SELinux label consists of
user:role:type:level/categories. In RHEL and Fedora level/categories
implements something called multi category security. One level multiple
categories with the constraint on categories being that all must match
for access to be allowed. This is how sVirt work. It picks two
categories and setups the qemu process to start with these categories
and then manipulates the files on disk to have these categories such
that even though the qemu_t proess can access the files type enforcement
wise the categories deny it. Its what allows for SELinux based per
process separation of a type.
Dave
On 11/1/2013 12:50 PM, Myklebust, Trond wrote:
> On Fri, 2013-11-01 at 12:02 -0400, Jeff Layton wrote:
>> It looks like _nfs4_get_security_label() has the same problem, but I've
>> so far been unable to get it to be called, so I didn't patch it. It
>> seems like getxattr does some special stuff for SELinux labels that
>> cause them only to ever be fetched once.
>>
>> Is there some trick to it?
>>
>
> Doesn't 'ls -Z' cause them to security label to be read again?
>
> Either way, the fix is clearly needed, so I've added the patch, and Cced
> stable.
>
> Cheers
> Trond
>
ls -Z will only cause the label to be reread if its no longer valid in
the NFSv4 cache. Otherwise it will use the in-core inode value and
return that.
On Fri, 2013-11-01 at 13:58 -0400, Jeff Layton wrote:
+AD4- On Fri, 1 Nov 2013 17:47:46 +-0000
+AD4- +ACI-Myklebust, Trond+ACI- +ADw-Trond.Myklebust+AEA-netapp.com+AD4- wrote:
+AD4-
+AD4- +AD4- On Fri, 2013-11-01 at 13:18 -0400, Jeff Layton wrote:
+AD4- +AD4- +AD4- On Fri, 1 Nov 2013 17:05:08 +-0000
+AD4- +AD4- +AD4- +ACI-Myklebust, Trond+ACI- +ADw-Trond.Myklebust+AEA-netapp.com+AD4- wrote:
+AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- +AD4- On Nov 1, 2013, at 12:57, Jeff Layton +ADw-jlayton+AEA-redhat.com+AD4- wrote:
+AD4- +AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- +AD4- +AD4- On Fri, 1 Nov 2013 16:50:00 +-0000
+AD4- +AD4- +AD4- +AD4- +AD4- +ACI-Myklebust, Trond+ACI- +ADw-Trond.Myklebust+AEA-netapp.com+AD4- wrote:
+AD4- +AD4- +AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- +AD4- +AD4APg- On Fri, 2013-11-01 at 12:02 -0400, Jeff Layton wrote:
+AD4- +AD4- +AD4- +AD4- +AD4APgA+- It looks like +AF8-nfs4+AF8-get+AF8-security+AF8-label() has the same problem, but I've
+AD4- +AD4- +AD4- +AD4- +AD4APgA+- so far been unable to get it to be called, so I didn't patch it. It
+AD4- +AD4- +AD4- +AD4- +AD4APgA+- seems like getxattr does some special stuff for SELinux labels that
+AD4- +AD4- +AD4- +AD4- +AD4APgA+- cause them only to ever be fetched once.
+AD4- +AD4- +AD4- +AD4- +AD4APgA+-
+AD4- +AD4- +AD4- +AD4- +AD4APgA+- Is there some trick to it?
+AD4- +AD4- +AD4- +AD4- +AD4APgA+-
+AD4- +AD4- +AD4- +AD4- +AD4APg-
+AD4- +AD4- +AD4- +AD4- +AD4APg- Doesn't 'ls -Z' cause them to security label to be read again?
+AD4- +AD4- +AD4- +AD4- +AD4APg-
+AD4- +AD4- +AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- +AD4- +AD4- As best I can tell, security labels are set on the inode when the inode
+AD4- +AD4- +AD4- +AD4- +AD4- is instantiated, and then are reset on changes (i.e. setxattr). If
+AD4- +AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- +AD4- +ICY-and on getxattr, afaics.
+AD4- +AD4- +AD4- +AD4-
+AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- I don't see that. The call chain is something like this:
+AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- vfs+AF8-getxattr
+AD4- +AD4- +AD4- xattr+AF8-getsecurity
+AD4- +AD4- +AD4- security+AF8-inode+AF8-getsecurity
+AD4- +AD4- +AD4- selinux+AF8-inode+AF8-getsecurity
+AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- ...and that function looks like it just converts the current security
+AD4- +AD4- +AD4- context on the inode to text and plops that into the buffer.
+AD4- +AD4-
+AD4- +AD4- Ah, you're right. You have to turn off SELinux in order to hit
+AD4- +AD4- nfs4+AF8-xattr+AF8-get+AF8-nfs4+AF8-label.
+AD4- +AD4-
+AD4- +AD4- +AD4- +AD4- +AD4- another client changes the label though, it's not clear to me how your
+AD4- +AD4- +AD4- +AD4- +AD4- client would ever notice it until the inode is dropped from the cache.
+AD4- +AD4- +AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- +AD4- +AD4- ISTR Eric Paris explaining to me that they do that for performance
+AD4- +AD4- +AD4- +AD4- +AD4- reasons but it seems like something that needs to be reconsidered in
+AD4- +AD4- +AD4- +AD4- +AD4- light of labeled NFS. Not picking up a security label change seems like
+AD4- +AD4- +AD4- +AD4- +AD4- a bug, IMO...
+AD4- +AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- +AD4- To be effective, the security label should normally be set at file creation time. It should rarely, if ever, change. Why would you need to change it from a different client?
+AD4- +AD4- +AD4- +AD4-
+AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- At least in Fedora, there are SELinux policy changes all the time.
+AD4- +AD4- +AD4- Sometimes that involves changing how files are labeled. I don't think
+AD4- +AD4- +AD4- it's reasonable to assume that they only get set at creation time.
+AD4- +AD4- +AD4-
+AD4- +AD4-
+AD4- +AD4- That doesn't answer the question. Again, why would you need to do this
+AD4- +AD4- from another client? If you don't have a real-life use case, then it's
+AD4- +AD4- just another 'doctor it hurts' problem. Stop doing it...
+AD4- +AD4-
+AD4-
+AD4- Ok, how about this then: +ADs-)
+AD4-
+AD4- NFS doesn't have O+AF8-TMPFILE, so you really +ACo-can't+ACo- set the context at
+AD4- creation time, at least with normal syscalls. There will always be a
+AD4- race window between creating a file and setting the SELinux context on
+AD4- it.
Ummm. This is supposed to be a security feature. How can you tolerate race windows?
IOW: If I get the security label +ACI-wrong+ACI- on lookup due to such a race,
then how am I supposed to know not to act on it?
+AD4- FWIW, I just started playing with this stuff and this behavior just
+AD4- gave me pause. This is the sort of thing that will give people fits
+AD4- since it's unexpected behavior.
+AD4-
+AD4- When I change something on the server, I typically expect the client to
+AD4- see that change in a timely fashion. As it stands now, it won't -- at
+AD4- least until the inode gets purged from the cache.
See my previous answer: don't do that... If you want a different answer,
then feel free to propose a caching model, but note that it's going to
be very hard to deal sensibly with your race condition above. That's the
reason why we went with the current caching model.
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust+AEA-netapp.com
http://www.netapp.com
On 11/1/2013 12:57 PM, Jeff Layton wrote:
> On Fri, 1 Nov 2013 16:50:00 +0000
> "Myklebust, Trond" <[email protected]> wrote:
>
>> On Fri, 2013-11-01 at 12:02 -0400, Jeff Layton wrote:
>>> It looks like _nfs4_get_security_label() has the same problem, but I've
>>> so far been unable to get it to be called, so I didn't patch it. It
>>> seems like getxattr does some special stuff for SELinux labels that
>>> cause them only to ever be fetched once.
>>>
>>> Is there some trick to it?
>>>
>>
>> Doesn't 'ls -Z' cause them to security label to be read again?
>>
>
> As best I can tell, security labels are set on the inode when the inode
> is instantiated, and then are reset on changes (i.e. setxattr). If
> another client changes the label though, it's not clear to me how your
> client would ever notice it until the inode is dropped from the cache.
>
> ISTR Eric Paris explaining to me that they do that for performance
> reasons but it seems like something that needs to be reconsidered in
> light of labeled NFS. Not picking up a security label change seems like
> a bug, IMO...
>
>> Either way, the fix is clearly needed, so I've added the patch, and Cced
>> stable.
>>
>
> Thanks!
>
This was the point of the original requirement for a label change
notification. Its important for us to pick up that label change when it
gets changed from another client or on the server directly. As of when
we implemented the prototype the cache invalidation for the security
attribute in NFSv4 was 5 seconds and we questioned whether or not it was
necessary to have an explicit change notification or if waiting 5
seconds was sufficient.
Dave
On Fri, 1 Nov 2013 18:29:43 +-0000
+ACI-Myklebust, Trond+ACI +ADw-Trond.Myklebust+AEA-netapp.com+AD4 wrote:
+AD4 On Fri, 2013-11-01 at 13:58 -0400, Jeff Layton wrote:
+AD4 +AD4 On Fri, 1 Nov 2013 17:47:46 +-0000
+AD4 +AD4 +ACI-Myklebust, Trond+ACI +ADw-Trond.Myklebust+AEA-netapp.com+AD4 wrote:
+AD4 +AD4
+AD4 +AD4 +AD4 On Fri, 2013-11-01 at 13:18 -0400, Jeff Layton wrote:
+AD4 +AD4 +AD4 +AD4 On Fri, 1 Nov 2013 17:05:08 +-0000
+AD4 +AD4 +AD4 +AD4 +ACI-Myklebust, Trond+ACI +ADw-Trond.Myklebust+AEA-netapp.com+AD4 wrote:
+AD4 +AD4 +AD4 +AD4
+AD4 +AD4 +AD4 +AD4 +AD4
+AD4 +AD4 +AD4 +AD4 +AD4 On Nov 1, 2013, at 12:57, Jeff Layton +ADw-jlayton+AEA-redhat.com+AD4 wrote:
+AD4 +AD4 +AD4 +AD4 +AD4
+AD4 +AD4 +AD4 +AD4 +AD4 +AD4 On Fri, 1 Nov 2013 16:50:00 +-0000
+AD4 +AD4 +AD4 +AD4 +AD4 +AD4 +ACI-Myklebust, Trond+ACI +ADw-Trond.Myklebust+AEA-netapp.com+AD4 wrote:
+AD4 +AD4 +AD4 +AD4 +AD4 +AD4
+AD4 +AD4 +AD4 +AD4 +AD4 +AD4APg On Fri, 2013-11-01 at 12:02 -0400, Jeff Layton wrote:
+AD4 +AD4 +AD4 +AD4 +AD4 +AD4APgA+ It looks like +AF8-nfs4+AF8-get+AF8-security+AF8-label() has the same problem, but I've
+AD4 +AD4 +AD4 +AD4 +AD4 +AD4APgA+ so far been unable to get it to be called, so I didn't patch it. It
+AD4 +AD4 +AD4 +AD4 +AD4 +AD4APgA+ seems like getxattr does some special stuff for SELinux labels that
+AD4 +AD4 +AD4 +AD4 +AD4 +AD4APgA+ cause them only to ever be fetched once.
+AD4 +AD4 +AD4 +AD4 +AD4 +AD4APgA+
+AD4 +AD4 +AD4 +AD4 +AD4 +AD4APgA+ Is there some trick to it?
+AD4 +AD4 +AD4 +AD4 +AD4 +AD4APgA+
+AD4 +AD4 +AD4 +AD4 +AD4 +AD4APg
+AD4 +AD4 +AD4 +AD4 +AD4 +AD4APg Doesn't 'ls -Z' cause them to security label to be read again?
+AD4 +AD4 +AD4 +AD4 +AD4 +AD4APg
+AD4 +AD4 +AD4 +AD4 +AD4 +AD4
+AD4 +AD4 +AD4 +AD4 +AD4 +AD4 As best I can tell, security labels are set on the inode when the inode
+AD4 +AD4 +AD4 +AD4 +AD4 +AD4 is instantiated, and then are reset on changes (i.e. setxattr). If
+AD4 +AD4 +AD4 +AD4 +AD4
+AD4 +AD4 +AD4 +AD4 +AD4 +ICY-and on getxattr, afaics.
+AD4 +AD4 +AD4 +AD4 +AD4
+AD4 +AD4 +AD4 +AD4
+AD4 +AD4 +AD4 +AD4 I don't see that. The call chain is something like this:
+AD4 +AD4 +AD4 +AD4
+AD4 +AD4 +AD4 +AD4 vfs+AF8-getxattr
+AD4 +AD4 +AD4 +AD4 xattr+AF8-getsecurity
+AD4 +AD4 +AD4 +AD4 security+AF8-inode+AF8-getsecurity
+AD4 +AD4 +AD4 +AD4 selinux+AF8-inode+AF8-getsecurity
+AD4 +AD4 +AD4 +AD4
+AD4 +AD4 +AD4 +AD4 ...and that function looks like it just converts the current security
+AD4 +AD4 +AD4 +AD4 context on the inode to text and plops that into the buffer.
+AD4 +AD4 +AD4
+AD4 +AD4 +AD4 Ah, you're right. You have to turn off SELinux in order to hit
+AD4 +AD4 +AD4 nfs4+AF8-xattr+AF8-get+AF8-nfs4+AF8-label.
+AD4 +AD4 +AD4
+AD4 +AD4 +AD4 +AD4 +AD4 +AD4 another client changes the label though, it's not clear to me how your
+AD4 +AD4 +AD4 +AD4 +AD4 +AD4 client would ever notice it until the inode is dropped from the cache.
+AD4 +AD4 +AD4 +AD4 +AD4 +AD4
+AD4 +AD4 +AD4 +AD4 +AD4 +AD4 ISTR Eric Paris explaining to me that they do that for performance
+AD4 +AD4 +AD4 +AD4 +AD4 +AD4 reasons but it seems like something that needs to be reconsidered in
+AD4 +AD4 +AD4 +AD4 +AD4 +AD4 light of labeled NFS. Not picking up a security label change seems like
+AD4 +AD4 +AD4 +AD4 +AD4 +AD4 a bug, IMO...
+AD4 +AD4 +AD4 +AD4 +AD4
+AD4 +AD4 +AD4 +AD4 +AD4 To be effective, the security label should normally be set at file creation time. It should rarely, if ever, change. Why would you need to change it from a different client?
+AD4 +AD4 +AD4 +AD4 +AD4
+AD4 +AD4 +AD4 +AD4
+AD4 +AD4 +AD4 +AD4 At least in Fedora, there are SELinux policy changes all the time.
+AD4 +AD4 +AD4 +AD4 Sometimes that involves changing how files are labeled. I don't think
+AD4 +AD4 +AD4 +AD4 it's reasonable to assume that they only get set at creation time.
+AD4 +AD4 +AD4 +AD4
+AD4 +AD4 +AD4
+AD4 +AD4 +AD4 That doesn't answer the question. Again, why would you need to do this
+AD4 +AD4 +AD4 from another client? If you don't have a real-life use case, then it's
+AD4 +AD4 +AD4 just another 'doctor it hurts' problem. Stop doing it...
+AD4 +AD4 +AD4
+AD4 +AD4
+AD4 +AD4 Ok, how about this then: +ADs)
+AD4 +AD4
+AD4 +AD4 NFS doesn't have O+AF8-TMPFILE, so you really +ACo-can't+ACo set the context at
+AD4 +AD4 creation time, at least with normal syscalls. There will always be a
+AD4 +AD4 race window between creating a file and setting the SELinux context on
+AD4 +AD4 it.
+AD4
+AD4 Ummm. This is supposed to be a security feature. How can you tolerate race windows?
+AD4
+AD4 IOW: If I get the security label +ACI-wrong+ACI on lookup due to such a race,
+AD4 then how am I supposed to know not to act on it?
+AD4
+AD4 +AD4 FWIW, I just started playing with this stuff and this behavior just
+AD4 +AD4 gave me pause. This is the sort of thing that will give people fits
+AD4 +AD4 since it's unexpected behavior.
+AD4 +AD4
+AD4 +AD4 When I change something on the server, I typically expect the client to
+AD4 +AD4 see that change in a timely fashion. As it stands now, it won't -- at
+AD4 +AD4 least until the inode gets purged from the cache.
+AD4
+AD4 See my previous answer: don't do that... If you want a different answer,
+AD4 then feel free to propose a caching model, but note that it's going to
+AD4 be very hard to deal sensibly with your race condition above. That's the
+AD4 reason why we went with the current caching model.
+AD4
I think we may be misunderstanding one another.
I'm not suggesting that we need to always fetch the label before any
use of an inode. The problem I was seeing is similar to what Dave
mentioned a month or so ago. Even in the case where we are fetching a
security label, we aren't always applying it to the inode.
The red flag for me is that we are calling nfs+AF8-setsecurity on existing
inodes in nfs+AF8-prime+AF8-dcache, but aren't calling it in
+AF8AXw-nfs+AF8-revalidate+AF8-inode. That's why 'ls -Z' works to make the client
pick up the label change when the parent directory changes, but 'stat'
doesn't.
I've just sent a patch that seems to fix the behavior for me in most
cases. Does it look reasonable?
--
Jeff Layton +ADw-jlayton+AEA-redhat.com+AD4
On Fri, 1 Nov 2013 10:49:32 -0400
Jeff Layton <[email protected]> wrote:
> Chao reported the following oops when testing labeled NFS:
>
> BUG: unable to handle kernel NULL pointer dereference at (null)
> IP: [<ffffffffa0568703>] nfs4_xdr_enc_setattr+0x43/0x110 [nfsv4]
> PGD 277bbd067 PUD 2777ea067 PMD 0
> Oops: 0000 [#1] SMP
> Modules linked in: rpcsec_gss_krb5 nfsv4 dns_resolver nfs fscache sg coretemp kvm_intel kvm crc32_pclmul crc32c_intel ghash_clmulni_intel aesni_intel lrw gf128mul iTCO_wdt glue_helper ablk_helper cryptd iTCO_vendor_support bnx2 pcspkr serio_raw i7core_edac cdc_ether microcode usbnet edac_core mii lpc_ich i2c_i801 mfd_core shpchp ioatdma dca acpi_cpufreq mperf nfsd auth_rpcgss nfs_acl lockd sunrpc xfs libcrc32c sr_mod sd_mod cdrom crc_t10dif mgag200 syscopyarea sysfillrect sysimgblt i2c_algo_bit drm_kms_helper ata_generic ttm pata_acpi drm ata_piix libata megaraid_sas i2c_core dm_mirror dm_region_hash dm_log dm_mod
> CPU: 4 PID: 25657 Comm: chcon Not tainted 3.10.0-33.el7.x86_64 #1
> Hardware name: IBM System x3550 M3 -[7944OEJ]-/90Y4784 , BIOS -[D6E150CUS-1.11]- 02/08/2011
> task: ffff880178397220 ti: ffff8801595d2000 task.ti: ffff8801595d2000
> RIP: 0010:[<ffffffffa0568703>] [<ffffffffa0568703>] nfs4_xdr_enc_setattr+0x43/0x110 [nfsv4]
> RSP: 0018:ffff8801595d3888 EFLAGS: 00010296
> RAX: 0000000000000000 RBX: ffff8801595d3b30 RCX: 0000000000000b4c
> RDX: ffff8801595d3b30 RSI: ffff8801595d38e0 RDI: ffff880278b6ec00
> RBP: ffff8801595d38c8 R08: ffff8801595d3b30 R09: 0000000000000001
> R10: 0000000000000000 R11: 0000000000000000 R12: ffff8801595d38e0
> R13: ffff880277a4a780 R14: ffffffffa05686c0 R15: ffff8802765f206c
> FS: 00007f2c68486800(0000) GS:ffff88027fc00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000000 CR3: 000000027651a000 CR4: 00000000000007e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> Stack:
> 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> 0000000000000000 ffff880277865800 ffff880278b6ec00 ffff880277a4a780
> ffff8801595d3948 ffffffffa02ad926 ffff8801595d3b30 ffff8802765f206c
> Call Trace:
> [<ffffffffa02ad926>] rpcauth_wrap_req+0x86/0xd0 [sunrpc]
> [<ffffffffa02a1d40>] ? call_connect+0xb0/0xb0 [sunrpc]
> [<ffffffffa02a1d40>] ? call_connect+0xb0/0xb0 [sunrpc]
> [<ffffffffa02a1ecb>] call_transmit+0x18b/0x290 [sunrpc]
> [<ffffffffa02a1d40>] ? call_connect+0xb0/0xb0 [sunrpc]
> [<ffffffffa02aae14>] __rpc_execute+0x84/0x400 [sunrpc]
> [<ffffffffa02ac40e>] rpc_execute+0x5e/0xa0 [sunrpc]
> [<ffffffffa02a2ea0>] rpc_run_task+0x70/0x90 [sunrpc]
> [<ffffffffa02a2f03>] rpc_call_sync+0x43/0xa0 [sunrpc]
> [<ffffffffa055284d>] _nfs4_do_set_security_label+0x11d/0x170 [nfsv4]
> [<ffffffffa0558861>] nfs4_set_security_label.isra.69+0xf1/0x1d0 [nfsv4]
> [<ffffffff815fca8b>] ? avc_alloc_node+0x24/0x125
> [<ffffffff815fcd2f>] ? avc_compute_av+0x1a3/0x1b5
> [<ffffffffa055897b>] nfs4_xattr_set_nfs4_label+0x3b/0x50 [nfsv4]
> [<ffffffff811bc772>] generic_setxattr+0x62/0x80
> [<ffffffff811bcfc3>] __vfs_setxattr_noperm+0x63/0x1b0
> [<ffffffff811bd1c5>] vfs_setxattr+0xb5/0xc0
> [<ffffffff811bd2fe>] setxattr+0x12e/0x1c0
> [<ffffffff811a4d22>] ? final_putname+0x22/0x50
> [<ffffffff811a4f2b>] ? putname+0x2b/0x40
> [<ffffffff811aa1cf>] ? user_path_at_empty+0x5f/0x90
> [<ffffffff8119bc29>] ? __sb_start_write+0x49/0x100
> [<ffffffff811bd66f>] SyS_lsetxattr+0x8f/0xd0
> [<ffffffff8160cf99>] system_call_fastpath+0x16/0x1b
> Code: 48 8b 02 48 c7 45 c0 00 00 00 00 48 c7 45 c8 00 00 00 00 48 c7 45 d0 00 00 00 00 48 c7 45 d8 00 00 00 00 48 c7 45 e0 00 00 00 00 <48> 8b 00 48 8b 00 48 85 c0 0f 84 ae 00 00 00 48 8b 80 b8 03 00
> RIP [<ffffffffa0568703>] nfs4_xdr_enc_setattr+0x43/0x110 [nfsv4]
> RSP <ffff8801595d3888>
> CR2: 0000000000000000
>
> The problem is that _nfs4_do_set_security_label calls rpc_call_sync()
> directly which fails to do any setup of the SEQUENCE call. Have it use
> nfs4_call_sync() instead which does the right thing. While we're at it
> change the name of "args" to "arg" to better match the pattern in
> _nfs4_do_setattr.
>
> Reported-by: Chao Ye <[email protected]>
> Cc: David Quigley <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> fs/nfs/nfs4proc.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index b02c4cc..d436c57 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -4657,7 +4657,7 @@ static int _nfs4_do_set_security_label(struct inode *inode,
> struct iattr sattr = {0};
> struct nfs_server *server = NFS_SERVER(inode);
> const u32 bitmask[3] = { 0, 0, FATTR4_WORD2_SECURITY_LABEL };
> - struct nfs_setattrargs args = {
> + struct nfs_setattrargs arg = {
> .fh = NFS_FH(inode),
> .iap = &sattr,
> .server = server,
> @@ -4671,14 +4671,14 @@ static int _nfs4_do_set_security_label(struct inode *inode,
> };
> struct rpc_message msg = {
> .rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_SETATTR],
> - .rpc_argp = &args,
> + .rpc_argp = &arg,
> .rpc_resp = &res,
> };
> int status;
>
> - nfs4_stateid_copy(&args.stateid, &zero_stateid);
> + nfs4_stateid_copy(&arg.stateid, &zero_stateid);
>
> - status = rpc_call_sync(server->client, &msg, 0);
> + status = nfs4_call_sync(server->client, server, &msg, &arg.seq_args, &res.seq_res, 1);
> if (status)
> dprintk("%s failed: %d\n", __func__, status);
>
Oh and btw...
It looks like _nfs4_get_security_label() has the same problem, but I've
so far been unable to get it to be called, so I didn't patch it. It
seems like getxattr does some special stuff for SELinux labels that
cause them only to ever be fetched once.
Is there some trick to it?
--
Jeff Layton <[email protected]>
On Fri, 2013-11-01 at 13:18 -0400, Jeff Layton wrote:
+AD4- On Fri, 1 Nov 2013 17:05:08 +-0000
+AD4- +ACI-Myklebust, Trond+ACI- +ADw-Trond.Myklebust+AEA-netapp.com+AD4- wrote:
+AD4-
+AD4- +AD4-
+AD4- +AD4- On Nov 1, 2013, at 12:57, Jeff Layton +ADw-jlayton+AEA-redhat.com+AD4- wrote:
+AD4- +AD4-
+AD4- +AD4- +AD4- On Fri, 1 Nov 2013 16:50:00 +-0000
+AD4- +AD4- +AD4- +ACI-Myklebust, Trond+ACI- +ADw-Trond.Myklebust+AEA-netapp.com+AD4- wrote:
+AD4- +AD4- +AD4-
+AD4- +AD4- +AD4APg- On Fri, 2013-11-01 at 12:02 -0400, Jeff Layton wrote:
+AD4- +AD4- +AD4APgA+- It looks like +AF8-nfs4+AF8-get+AF8-security+AF8-label() has the same problem, but I've
+AD4- +AD4- +AD4APgA+- so far been unable to get it to be called, so I didn't patch it. It
+AD4- +AD4- +AD4APgA+- seems like getxattr does some special stuff for SELinux labels that
+AD4- +AD4- +AD4APgA+- cause them only to ever be fetched once.
+AD4- +AD4- +AD4APgA+-
+AD4- +AD4- +AD4APgA+- Is there some trick to it?
+AD4- +AD4- +AD4APgA+-
+AD4- +AD4- +AD4APg-
+AD4- +AD4- +AD4APg- Doesn't 'ls -Z' cause them to security label to be read again?
+AD4- +AD4- +AD4APg-
+AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- As best I can tell, security labels are set on the inode when the inode
+AD4- +AD4- +AD4- is instantiated, and then are reset on changes (i.e. setxattr). If
+AD4- +AD4-
+AD4- +AD4- +ICY-and on getxattr, afaics.
+AD4- +AD4-
+AD4-
+AD4- I don't see that. The call chain is something like this:
+AD4-
+AD4- vfs+AF8-getxattr
+AD4- xattr+AF8-getsecurity
+AD4- security+AF8-inode+AF8-getsecurity
+AD4- selinux+AF8-inode+AF8-getsecurity
+AD4-
+AD4- ...and that function looks like it just converts the current security
+AD4- context on the inode to text and plops that into the buffer.
Ah, you're right. You have to turn off SELinux in order to hit
nfs4+AF8-xattr+AF8-get+AF8-nfs4+AF8-label.
+AD4- +AD4- +AD4- another client changes the label though, it's not clear to me how your
+AD4- +AD4- +AD4- client would ever notice it until the inode is dropped from the cache.
+AD4- +AD4- +AD4-
+AD4- +AD4- +AD4- ISTR Eric Paris explaining to me that they do that for performance
+AD4- +AD4- +AD4- reasons but it seems like something that needs to be reconsidered in
+AD4- +AD4- +AD4- light of labeled NFS. Not picking up a security label change seems like
+AD4- +AD4- +AD4- a bug, IMO...
+AD4- +AD4-
+AD4- +AD4- To be effective, the security label should normally be set at file creation time. It should rarely, if ever, change. Why would you need to change it from a different client?
+AD4- +AD4-
+AD4-
+AD4- At least in Fedora, there are SELinux policy changes all the time.
+AD4- Sometimes that involves changing how files are labeled. I don't think
+AD4- it's reasonable to assume that they only get set at creation time.
+AD4-
That doesn't answer the question. Again, why would you need to do this
from another client? If you don't have a real-life use case, then it's
just another 'doctor it hurts' problem. Stop doing it...
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust+AEA-netapp.com
http://www.netapp.com