Date: Mon, 11 Aug 2008 19:54:09 GMT From: Edward Tomasz Napierala <trasz@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 147175 for review Message-ID: <200808111954.m7BJs9QX066408@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=147175 Change 147175 by trasz@trasz_traszkan on 2008/08/11 19:53:25 With NFS4 ACLs, chmod(2) may need to add additional entries. Make sure it has room for that by limiting the acl_cnt to about half of ACL_MAX_ENTRIES in ACL setting routines. This is similar to what SunOS does. Bump ACL_MAX_ENTRIES to 202, so that "struct acl" is exactly one page long. This breaks binary compatibility, requring libc recompilation, and on-disk format, requiring one to remove nfs4.acl extattr for every file. Affected files ... .. //depot/projects/soc2008/trasz_nfs4acl/TODO#40 edit .. //depot/projects/soc2008/trasz_nfs4acl/lib/libc/posix1e/acl_support.h#8 edit .. //depot/projects/soc2008/trasz_nfs4acl/sys/kern/subr_acl_nfs4.c#20 edit .. //depot/projects/soc2008/trasz_nfs4acl/sys/kern/vfs_acl.c#9 edit .. //depot/projects/soc2008/trasz_nfs4acl/sys/sys/acl.h#18 edit Differences ... ==== //depot/projects/soc2008/trasz_nfs4acl/TODO#40 (text+ko) ==== @@ -12,11 +12,6 @@ - Add the information about correct constants to the manual pages. -- Decide what to do when chmod(2) needs to add ACL entries, but - there is no room in 'struct acl' to do that. Solaris seems to - limit the numer of user-settable entries to half of ACL_MAX_ENTRIES, - so there is no risk of running out of them in chmod(2). - - Make 'struct acl' variable size. - Benchmark things. ==== //depot/projects/soc2008/trasz_nfs4acl/lib/libc/posix1e/acl_support.h#8 (text+ko) ==== @@ -33,7 +33,7 @@ #define _ACL_SUPPORT_H #define _POSIX1E_ACL_STRING_PERM_MAXSIZE 3 /* read, write, exec */ -#define _ACL_T_ALIGNMENT_BITS 12 +#define _ACL_T_ALIGNMENT_BITS 13 int _acl_type_unold(acl_type_t type); int _acl_differs(const acl_t a, const acl_t b); ==== //depot/projects/soc2008/trasz_nfs4acl/sys/kern/subr_acl_nfs4.c#20 (text+ko) ==== @@ -98,6 +98,9 @@ if (denied_explicitly != NULL) *denied_explicitly = 0; + KASSERT(aclp->acl_cnt > 0, ("aclp->acl_cnt > 0")); + KASSERT(aclp->acl_cnt <= ACL_MAX_ENTRIES, ("aclp->acl_cnt <= ACL_MAX_ENTRIES")); + for (i = 0; i < aclp->acl_cnt; i++) { entry = &(aclp->acl_entry[i]); @@ -289,12 +292,12 @@ { struct acl_entry *entry; + KASSERT(aclp->acl_cnt + 1 <= ACL_MAX_ENTRIES, + ("aclp->acl_cnt + 1 <= ACL_MAX_ENTRIES")); + entry = &(aclp->acl_entry[aclp->acl_cnt]); aclp->acl_cnt++; - if (aclp->acl_cnt >= ACL_MAX_ENTRIES) - return (NULL); - entry->ae_tag = tag; entry->ae_id = ACL_UNDEFINED_ID; entry->ae_perm = perm; @@ -309,8 +312,8 @@ { int i; - if (aclp->acl_cnt + 1 >= ACL_MAX_ENTRIES) - return (NULL); + KASSERT(aclp->acl_cnt + 1 <= ACL_MAX_ENTRIES, + ("aclp->acl_cnt + 1 <= ACL_MAX_ENTRIES")); for (i = aclp->acl_cnt; i > entry_index; i--) aclp->acl_entry[i] = aclp->acl_entry[i - 1]; @@ -368,9 +371,6 @@ * ACE. */ copy = _acl_duplicate_entry(aclp, i); - /* XXX: Is EPERM a good choice here? */ - if (copy == NULL) - return (EPERM); /* * 1.3.2. In the first ACE, the flag @@ -472,8 +472,6 @@ */ previous = entry; entry = _acl_duplicate_entry(aclp, i); - if (entry == NULL) - return (EPERM); /* Adjust counter, as we've just extended the ACL. */ i++; @@ -840,8 +838,8 @@ ACL_ENTRY_FILE_INHERIT)) == 0) continue; - KASSERT(child_aclp->acl_cnt < ACL_MAX_ENTRIES, - ("child_aclp->acl_cnt < ACL_MAX_ENTRIES")); + KASSERT(child_aclp->acl_cnt + 1 <= ACL_MAX_ENTRIES, + ("child_aclp->acl_cnt + 1 <= ACL_MAX_ENTRIES")); child_aclp->acl_entry[child_aclp->acl_cnt] = *parent_entry; child_aclp->acl_cnt++; } @@ -901,8 +899,6 @@ * 2.D. Copy the original ACE into a second, adjacent ACE. */ copy = _acl_duplicate_entry(child_aclp, i); - if (copy == NULL) - return (EPERM); /* * 2.E. On the first ACE, ensure that ACL_ENTRY_ONLY_INHERIT @@ -970,7 +966,7 @@ * valid. There can be none of them too. Really. */ - if (acl->acl_cnt > ACL_MAX_ENTRIES || acl->acl_cnt < 0) + if (aclp->acl_cnt > ACL_MAX_ENTRIES || aclp->acl_cnt <= 0) return (EINVAL); for (i = 0; i < aclp->acl_cnt; i++) { ==== //depot/projects/soc2008/trasz_nfs4acl/sys/kern/vfs_acl.c#9 (text+ko) ==== @@ -206,6 +206,18 @@ error = copyin_acl(aclp, inkernelacl, type); if (error != 0) goto out_free; + + /* + * With NFS4 ACLs, chmod(2) may need to add additional entries. + * Make sure it has enough room for that - splitting every entry + * into two and appending "canonical six" entries at the end. + */ + if (type == ACL_TYPE_NFS4 && + inkernelacl->acl_cnt > (ACL_MAX_ENTRIES - 6) / 2) { + error = ENOSPC; + goto out_free; + } + error = vn_start_write(vp, &mp, V_WAIT | PCATCH); if (error != 0) goto out_free; @@ -302,6 +314,18 @@ error = copyin_acl(aclp, inkernelacl, type); if (error != 0) goto out_free; + + /* + * With NFS4 ACLs, chmod(2) may need to add additional entries. + * Make sure it has enough room for that - splitting every entry + * into two and appending "canonical six" entries at the end. + */ + if (type == ACL_TYPE_NFS4 && + inkernelacl->acl_cnt > (ACL_MAX_ENTRIES - 6) / 2) { + error = ENOSPC; + goto out_free; + } + error = VOP_ACLCHECK(vp, type_unold(type), inkernelacl, td->td_ucred, td); out_free: ==== //depot/projects/soc2008/trasz_nfs4acl/sys/sys/acl.h#18 (text+ko) ==== @@ -50,7 +50,12 @@ #define NFS4_ACL_EXTATTR_NAMESPACE EXTATTR_NAMESPACE_SYSTEM #define NFS4_ACL_EXTATTR_NAME "nfs4.acl" #define OLDACL_MAX_ENTRIES 32 -#define ACL_MAX_ENTRIES OLDACL_MAX_ENTRIES +/* + * With 204 entries, "struct acl" is exactly one page big. + * Note that with NFS4 ACLs, the maximum number of ACL entries one + * may set on file or directory is about half of ACL_MAX_ENTRIES. + */ +#define ACL_MAX_ENTRIES 204 /* * "struct oldacl" is used in compatibility ACL syscalls and for on-disk
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200808111954.m7BJs9QX066408>