Make IS_POSIXACL() return false if POSIX ACL support is disabled and
ignore SB_POSIXACL/MS_POSIXACL.
Never skip applying the umask in namei.c and never bother to do any
ACL specific checks if the filesystem falsely indicates it has ACLs
enabled when the feature is completely disabled in the kernel.
This fixes a problem where the umask is always ignored in the NFS
client when compiled without CONFIG_FS_POSIX_ACL. This is a 4 year
old regression caused by commit 013cdf1088d723 which itself was not
completely wrong, but failed to consider all the side effects by
misdesigned VFS code.
Prior to that commit, there were two places where the umask could be
applied, for example when creating a directory:
1. in the VFS layer in SYSCALL_DEFINE3(mkdirat), but only if
!IS_POSIXACL()
2. again (unconditionally) in nfs3_proc_mkdir()
The first one does not apply, because even without
CONFIG_FS_POSIX_ACL, the NFS client sets MS_POSIXACL in
nfs_fill_super().
After that commit, (2.) was replaced by:
2b. in posix_acl_create(), called by nfs3_proc_mkdir()
There's one branch in posix_acl_create() which applies the umask;
however, without CONFIG_FS_POSIX_ACL, posix_acl_create() is an empty
dummy function which does not apply the umask.
The approach chosen by this patch is to make IS_POSIXACL() always
return false when POSIX ACL support is disabled, so the umask always
gets applied by the VFS layer. This is consistent with the (regular)
behavior of posix_acl_create(): that function returns early if
IS_POSIXACL() is false, before applying the umask.
Therefore, posix_acl_create() is responsible for applying the umask if
there is ACL support enabled in the file system (SB_POSIXACL), and the
VFS layer is responsible for all other cases (no SB_POSIXACL or no
CONFIG_FS_POSIX_ACL).
Signed-off-by: Max Kellermann <[email protected]>
---
include/linux/fs.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 440281f8564d..c3240c28e61b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1883,7 +1883,12 @@ static inline bool sb_rdonly(const struct super_block *sb) { return sb->s_flags
#define IS_NOQUOTA(inode) ((inode)->i_flags & S_NOQUOTA)
#define IS_APPEND(inode) ((inode)->i_flags & S_APPEND)
#define IS_IMMUTABLE(inode) ((inode)->i_flags & S_IMMUTABLE)
+
+#ifdef CONFIG_FS_POSIX_ACL
#define IS_POSIXACL(inode) __IS_FLG(inode, SB_POSIXACL)
+#else
+#define IS_POSIXACL(inode) 0
+#endif
#define IS_DEADDIR(inode) ((inode)->i_flags & S_DEAD)
#define IS_NOCMTIME(inode) ((inode)->i_flags & S_NOCMTIME)
nfs/super: set MS_POSIXACL only if ACL support is enabled
The code comment says "We will [apply the umask] ourselves", but that
happens in posix_acl_create() only if the kernel has POSIX ACL
support. Without it, posix_acl_create() is a is an empty dummy
function.
So let's not pretend we will apply the umask if we can already know
that we will never.
This fixes a problem where the umask is always ignored in the NFS
client when compiled without CONFIG_FS_POSIX_ACL. This is a 4 year
old regression caused by commit 013cdf1088d723 which itself was not
completely wrong, but failed to consider all the side effects by
misdesigned VFS code.
There are two compile-time checks and one runtime check:
- If CONFIG_FS_POSIX_ACL=n, then MS_POSIXACL is never set.
- If CONFIG_FS_POSIX_ACL=y and CONFIG_NFS_V3_ACL=n, then only NFSv4
has ACL support (and cannot be disabled), and we need to check for
"version==4".
- If CONFIG_FS_POSIX_ACL=y and CONFIG_NFS_V3_ACL=y, MS_POSIXACL is
always set, as before.
Signed-off-by: Max Kellermann <[email protected]>
---
fs/nfs/super.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 216f67d628b3..ec4e1f2775e0 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -2338,10 +2338,17 @@ void nfs_fill_super(struct super_block *sb, struct nfs_mount_info *mount_info)
sb->s_blocksize = nfs_block_size(data->bsize, &sb->s_blocksize_bits);
if (server->nfs_client->rpc_ops->version != 2) {
- /* The VFS shouldn't apply the umask to mode bits. We will do
- * so ourselves when necessary.
- */
- sb->s_flags |= MS_POSIXACL;
+#ifdef CONFIG_FS_POSIX_ACL
+#ifndef CONFIG_NFS_V3_ACL
+ if (nfss->nfs_client->rpc_ops->version == 4)
+#endif
+ /* The VFS shouldn't apply the umask to mode
+ * bits. We will do so ourselves when
+ * necessary.
+ */
+ sb->s_flags |= MS_POSIXACL;
+#endif
+
sb->s_time_gran = 1;
sb->s_export_op = &nfs_export_ops;
}
On Mon, Jan 15, 2018 at 06:30:51PM +0100, Max Kellermann wrote:
> nfs/super: set MS_POSIXACL only if ACL support is enabled
>
> The code comment says "We will [apply the umask] ourselves", but that
> happens in posix_acl_create() only if the kernel has POSIX ACL
> support. Without it, posix_acl_create() is a is an empty dummy
> function.
<snip>
Your subject line is a bit odd :(
On 2018/01/15 18:41, Greg KH <[email protected]> wrote:
> Your subject line is a bit odd :(
True, I already repaired & resent it. Sorry for the hiccup.
T24gTW9uLCAyMDE4LTAxLTE1IGF0IDE4OjMwICswMTAwLCBNYXggS2VsbGVybWFubiB3cm90ZToN
Cj4gbmZzL3N1cGVyOiBzZXQgTVNfUE9TSVhBQ0wgb25seSBpZiBBQ0wgc3VwcG9ydCBpcyBlbmFi
bGVkDQo+IA0KPiBUaGUgY29kZSBjb21tZW50IHNheXMgIldlIHdpbGwgW2FwcGx5IHRoZSB1bWFz
a10gb3Vyc2VsdmVzIiwgYnV0IHRoYXQNCj4gaGFwcGVucyBpbiBwb3NpeF9hY2xfY3JlYXRlKCkg
b25seSBpZiB0aGUga2VybmVsIGhhcyBQT1NJWCBBQ0wNCj4gc3VwcG9ydC4gIFdpdGhvdXQgaXQs
IHBvc2l4X2FjbF9jcmVhdGUoKSBpcyBhIGlzIGFuIGVtcHR5IGR1bW15DQo+IGZ1bmN0aW9uLg0K
PiANCj4gU28gbGV0J3Mgbm90IHByZXRlbmQgd2Ugd2lsbCBhcHBseSB0aGUgdW1hc2sgaWYgd2Ug
Y2FuIGFscmVhZHkga25vdw0KPiB0aGF0IHdlIHdpbGwgbmV2ZXIuDQo+IA0KPiBUaGlzIGZpeGVz
IGEgcHJvYmxlbSB3aGVyZSB0aGUgdW1hc2sgaXMgYWx3YXlzIGlnbm9yZWQgaW4gdGhlIE5GUw0K
PiBjbGllbnQgd2hlbiBjb21waWxlZCB3aXRob3V0IENPTkZJR19GU19QT1NJWF9BQ0wuICBUaGlz
IGlzIGEgNCB5ZWFyDQo+IG9sZCByZWdyZXNzaW9uIGNhdXNlZCBieSBjb21taXQgMDEzY2RmMTA4
OGQ3MjMgd2hpY2ggaXRzZWxmIHdhcyBub3QNCj4gY29tcGxldGVseSB3cm9uZywgYnV0IGZhaWxl
ZCB0byBjb25zaWRlciBhbGwgdGhlIHNpZGUgZWZmZWN0cyBieQ0KPiBtaXNkZXNpZ25lZCBWRlMg
Y29kZS4NCj4gDQo+IFRoZXJlIGFyZSB0d28gY29tcGlsZS10aW1lIGNoZWNrcyBhbmQgb25lIHJ1
bnRpbWUgY2hlY2s6DQo+IA0KPiAtIElmIENPTkZJR19GU19QT1NJWF9BQ0w9biwgdGhlbiBNU19Q
T1NJWEFDTCBpcyBuZXZlciBzZXQuDQo+IA0KPiAtIElmIENPTkZJR19GU19QT1NJWF9BQ0w9eSBh
bmQgQ09ORklHX05GU19WM19BQ0w9biwgdGhlbiBvbmx5IE5GU3Y0DQo+ICAgaGFzIEFDTCBzdXBw
b3J0IChhbmQgY2Fubm90IGJlIGRpc2FibGVkKSwgYW5kIHdlIG5lZWQgdG8gY2hlY2sgZm9yDQo+
ICAgInZlcnNpb249PTQiLg0KPiANCj4gLSBJZiBDT05GSUdfRlNfUE9TSVhfQUNMPXkgYW5kIENP
TkZJR19ORlNfVjNfQUNMPXksIE1TX1BPU0lYQUNMIGlzDQo+ICAgYWx3YXlzIHNldCwgYXMgYmVm
b3JlLg0KPiANCj4gU2lnbmVkLW9mZi1ieTogTWF4IEtlbGxlcm1hbm4gPG1rQGNtNGFsbC5jb20+
DQo+IC0tLQ0KPiAgZnMvbmZzL3N1cGVyLmMgfCAgIDE1ICsrKysrKysrKysrLS0tLQ0KPiAgMSBm
aWxlIGNoYW5nZWQsIDExIGluc2VydGlvbnMoKyksIDQgZGVsZXRpb25zKC0pDQo+IA0KPiBkaWZm
IC0tZ2l0IGEvZnMvbmZzL3N1cGVyLmMgYi9mcy9uZnMvc3VwZXIuYw0KPiBpbmRleCAyMTZmNjdk
NjI4YjMuLmVjNGUxZjI3NzVlMCAxMDA2NDQNCj4gLS0tIGEvZnMvbmZzL3N1cGVyLmMNCj4gKysr
IGIvZnMvbmZzL3N1cGVyLmMNCj4gQEAgLTIzMzgsMTAgKzIzMzgsMTcgQEAgdm9pZCBuZnNfZmls
bF9zdXBlcihzdHJ1Y3Qgc3VwZXJfYmxvY2sgKnNiLA0KPiBzdHJ1Y3QgbmZzX21vdW50X2luZm8g
Km1vdW50X2luZm8pDQo+ICAJCXNiLT5zX2Jsb2Nrc2l6ZSA9IG5mc19ibG9ja19zaXplKGRhdGEt
PmJzaXplLCAmc2ItDQo+ID5zX2Jsb2Nrc2l6ZV9iaXRzKTsNCj4gIA0KPiAgCWlmIChzZXJ2ZXIt
Pm5mc19jbGllbnQtPnJwY19vcHMtPnZlcnNpb24gIT0gMikgew0KPiAtCQkvKiBUaGUgVkZTIHNo
b3VsZG4ndCBhcHBseSB0aGUgdW1hc2sgdG8gbW9kZSBiaXRzLg0KPiBXZSB3aWxsIGRvDQo+IC0J
CSAqIHNvIG91cnNlbHZlcyB3aGVuIG5lY2Vzc2FyeS4NCj4gLQkJICovDQo+IC0JCXNiLT5zX2Zs
YWdzIHw9IE1TX1BPU0lYQUNMOw0KPiArI2lmZGVmIENPTkZJR19GU19QT1NJWF9BQ0wNCj4gKyNp
Zm5kZWYgQ09ORklHX05GU19WM19BQ0wNCj4gKwkJaWYgKG5mc3MtPm5mc19jbGllbnQtPnJwY19v
cHMtPnZlcnNpb24gPT0gNCkNCj4gKyNlbmRpZg0KPiArCQkJLyogVGhlIFZGUyBzaG91bGRuJ3Qg
YXBwbHkgdGhlIHVtYXNrIHRvIG1vZGUNCj4gKwkJCSAqIGJpdHMuIFdlIHdpbGwgZG8gc28gb3Vy
c2VsdmVzIHdoZW4NCj4gKwkJCSAqIG5lY2Vzc2FyeS4NCj4gKwkJCSAqLw0KPiArCQkJc2ItPnNf
ZmxhZ3MgfD0gTVNfUE9TSVhBQ0w7DQo+ICsjZW5kaWYNCj4gKw0KPiAgCQlzYi0+c190aW1lX2dy
YW4gPSAxOw0KPiAgCQlzYi0+c19leHBvcnRfb3AgPSAmbmZzX2V4cG9ydF9vcHM7DQo+ICAJfQ0K
DQpUaGUgYWJvdmUgaWxsdXN0cmF0ZXMgZXhhY3RseSB3aHkgSSd2ZSBhc2tlZCBwZW9wbGUgX25l
dmVyXyB0byBtYWtlDQphbnl0aGluZyBjb25kaXRpb25hbCBvbiBycGNfb3BzLT52ZXJzaW9uLiBQ
bGVhc2UgdXNlIGEgTkZTIGNhcGFiaWxpdHkNCihpLmUuIE5GU19TQihzYiktPmNhcHMpIGZvciB0
aGlzIGtpbmQgb2YgdGhpbmcuIFRoYXQgZXhwcmVzc2VzIHRoZQ0KY29uZGl0aW9uIGluIHRlcm1z
IG9mIHRoZSBmdW5jdGlvbmFsaXR5IHdlIHdhbnQgaW5zdGVhZCBvZiBhIHdoaW1zaWNhbA0KcHJv
dG9jb2wgdmVyc2lvbiBudW1iZXIuDQoNClRoYW5rcw0KICBUcm9uZA0KLS0gDQpUcm9uZCBNeWts
ZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRhaW5lciwgUHJpbWFyeURhdGENCnRyb25kLm15
a2xlYnVzdEBwcmltYXJ5ZGF0YS5jb20NCg==
Looks right to me.
Reviewed-by: J. Bruce Fields <[email protected]>
--b.
On Mon, Jan 15, 2018 at 06:30:46PM +0100, Max Kellermann wrote:
> Make IS_POSIXACL() return false if POSIX ACL support is disabled and
> ignore SB_POSIXACL/MS_POSIXACL.
>
> Never skip applying the umask in namei.c and never bother to do any
> ACL specific checks if the filesystem falsely indicates it has ACLs
> enabled when the feature is completely disabled in the kernel.
>
> This fixes a problem where the umask is always ignored in the NFS
> client when compiled without CONFIG_FS_POSIX_ACL. This is a 4 year
> old regression caused by commit 013cdf1088d723 which itself was not
> completely wrong, but failed to consider all the side effects by
> misdesigned VFS code.
>
> Prior to that commit, there were two places where the umask could be
> applied, for example when creating a directory:
>
> 1. in the VFS layer in SYSCALL_DEFINE3(mkdirat), but only if
> !IS_POSIXACL()
>
> 2. again (unconditionally) in nfs3_proc_mkdir()
>
> The first one does not apply, because even without
> CONFIG_FS_POSIX_ACL, the NFS client sets MS_POSIXACL in
> nfs_fill_super().
>
> After that commit, (2.) was replaced by:
>
> 2b. in posix_acl_create(), called by nfs3_proc_mkdir()
>
> There's one branch in posix_acl_create() which applies the umask;
> however, without CONFIG_FS_POSIX_ACL, posix_acl_create() is an empty
> dummy function which does not apply the umask.
>
> The approach chosen by this patch is to make IS_POSIXACL() always
> return false when POSIX ACL support is disabled, so the umask always
> gets applied by the VFS layer. This is consistent with the (regular)
> behavior of posix_acl_create(): that function returns early if
> IS_POSIXACL() is false, before applying the umask.
>
> Therefore, posix_acl_create() is responsible for applying the umask if
> there is ACL support enabled in the file system (SB_POSIXACL), and the
> VFS layer is responsible for all other cases (no SB_POSIXACL or no
> CONFIG_FS_POSIX_ACL).
>
> Signed-off-by: Max Kellermann <[email protected]>
> ---
> include/linux/fs.h | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 440281f8564d..c3240c28e61b 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1883,7 +1883,12 @@ static inline bool sb_rdonly(const struct super_block *sb) { return sb->s_flags
> #define IS_NOQUOTA(inode) ((inode)->i_flags & S_NOQUOTA)
> #define IS_APPEND(inode) ((inode)->i_flags & S_APPEND)
> #define IS_IMMUTABLE(inode) ((inode)->i_flags & S_IMMUTABLE)
> +
> +#ifdef CONFIG_FS_POSIX_ACL
> #define IS_POSIXACL(inode) __IS_FLG(inode, SB_POSIXACL)
> +#else
> +#define IS_POSIXACL(inode) 0
> +#endif
>
> #define IS_DEADDIR(inode) ((inode)->i_flags & S_DEAD)
> #define IS_NOCMTIME(inode) ((inode)->i_flags & S_NOCMTIME)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html