[Pvfs2-developers] patches: permission/acl bug fixes
Sam Lang
slang at mcs.anl.gov
Tue Mar 20 14:26:39 EST 2007
Hi Phil,
Thanks for the patches. I went ahead and committed them. I have a
few ideas about changes to the code that are motivated by the
patches, but not really issues with the patches themselves. See
comments below.
-sam
On Mar 20, 2007, at 8:36 AM, Phil Carns wrote:
> acl-check-assert.patch:
> ------------------------
> This is a bug fix to the server side acl handling. It replaces an
> assertion with normal error handling to prevent a server from
> crashing if it encounters invalid acl information.
>
It seems like it should be possible to do that format checking of the
acl when the system.posix_acl_access extended attribute is set. Does
it make sense to add a callouts framework to set-eattr to do format
checking for specific xattrs?
> check-group.patch:
> ------------------
> This follows up on some recent fixes to the server side group
> checking. I think the getpwuid_r() has the same issue that
> getpwuid_r() did, in that you have to check if the last argument is
> NULL before using the results of the function. There was also a
> double unlock in the function.
>
> setgid-inherit-acl.patch:
> -------------------------
> This corrects setgid behavior if the "-o acl" mount option is used;
> previously the setgid bit was not being inherited by new
> subdirectories in this scenario.
>
> root-squash.patch:
> ------------------
> This is an update to the root squashing behavior. It is not a 100%
> correct fix, but I think it is an improvement. Prior to this
> patch, the root user could still write to an existing root owned
> file even when squashed. After this patch, all writes from root
> are disallowed. The basic problem is that there isn't enough
> permission information on servers at the individual write operation
> level to decide whether to allow or disallow the squashed write (I/
> O servers don't have owner or permission data). The easiest fix is
> to stop all root writes on a squashed file system. :e Normally a
> file system would allow root to write to files owned by
> "nobody" (or whatever uid the mapping points to), but this would be
> difficult to implement.
For root-squash: I've wondered why the dspace entries for datafile
handles don't carry the ownership and permissions, and it seems like
its only because we don't pass the attributes along with the create
call. The setattr does set the attrs on the metadata handle, but its
primary purpose is to set the datafile handles list in the metadata.
We already have the file's attributes -- they get passed in with the
PVFS_sys_create call. Could we possibly add an object attr field to
the create so that the attr gets set on dspace entry for datafile
handles as well? Once that's done, the credentials passed in the
write request could be checked against the attributes. I think that
would allow us to get the proper semantics for squashing.
The drawback I see in doing this would be that a chmod/chown/chgrp
would require doing setattrs to all the IO servers as well as the
metadata server. It seems like those operations are infrequent
enough that doing so wouldn't be a big deal. Also, the create state
machine on the server would have to do a trove_dspace_setattr after
the trove_dspace_create completed. We could avoid being 2x slower by
not syncing on the create though.
>
> The above patch also fixes the get_fs_intent() function; it had a
> "default: " case that made it too easy to forget to add operations
> to the list. This patch removes the default case (so warnings show
> up at build time now) and adds the listattr and smallio
> operations. The former is needed for root squashing to work as well.
>
> -Phil
> Index: pvfs2_src/src/common/misc/pint-util.c
> ===================================================================
> --- pvfs2_src/src/common/misc/pint-util.c (revision 2755)
> +++ pvfs2_src/src/common/misc/pint-util.c (revision 2756)
> @@ -635,7 +635,18 @@
>
> assert((attr->mask & perm_mask) == perm_mask);
> assert(acl_buf);
> +#if 0
> assert(acl_size % sizeof(pvfs2_acl_entry) == 0);
> +#else
> + /* if the acl format doesn't look valid, then return an error
> rather than
> + * asserting; we don't want the server to crash due to an
> invalid keyval
> + */
> + if((acl_size % sizeof(pvfs2_acl_entry)) != 0)
> + {
> + gossip_debug(GOSSIP_PERMISSIONS_DEBUG, "invalid acls on
> object\n");
> + return(-PVFS_EACCES);
> + }
> +#endif
> count = acl_size / sizeof(pvfs2_acl_entry);
>
> for (i = 0; i < count; i++)
> diff -Naur pvfs2-orig/src/common/misc/pint-util.c pvfs2/src/common/
> misc/pint-util.c
> --- pvfs2-orig/src/common/misc/pint-util.c 2006-12-14
> 00:23:55.000000000 -0500
> +++ pvfs2/src/common/misc/pint-util.c 2007-03-14 14:41:20.000000000
> -0500
> @@ -522,14 +522,18 @@
> ret = getpwuid_r(uid, &pwd, check_group_pw_buffer,
> check_group_pw_buffer_size,
> &pwd_p);
> - if(ret != 0)
> + if(ret != 0 || pwd_p == NULL)
> {
> gen_mutex_unlock(&check_group_mutex);
> return(-PVFS_EINVAL);
> }
>
> /* check primary group */
> - if(pwd.pw_gid == gid) return 0;
> + if(pwd.pw_gid == gid)
> + {
> + gen_mutex_unlock(&check_group_mutex);
> + return 0;
> + }
>
> /* get other group information */
> ret = getgrgid_r(gid, &grp, check_group_gr_buffer,
> @@ -549,9 +553,6 @@
> return(-PVFS_EACCES);
> }
>
> - gen_mutex_unlock(&check_group_mutex);
> -
> -
> for(i = 0; grp.gr_mem[i] != NULL; i++)
> {
> if(0 == strcmp(pwd.pw_name, grp.gr_mem[i]) )
> Index: pvfs2_src/src/server/prelude.sm
> ===================================================================
> --- pvfs2_src/src/server/prelude.sm (revision 3020)
> +++ pvfs2_src/src/server/prelude.sm (revision 3021)
> @@ -201,10 +201,18 @@
> *fsid = req->u.io.fs_id;
> *read_only = (req->u.io.io_type == PVFS_IO_READ) ? 1 : 0;
> break;
> + case PVFS_SERV_SMALL_IO:
> + *fsid = req->u.small_io.fs_id;
> + *read_only = (req->u.small_io.io_type ==
> PVFS_IO_READ) ? 1 : 0;
> + break;
> case PVFS_SERV_GETATTR:
> *fsid = req->u.getattr.fs_id;
> *read_only = 1;
> break;
> + case PVFS_SERV_LISTATTR:
> + *fsid = req->u.listattr.fs_id;
> + *read_only = 1;
> + break;
> case PVFS_SERV_SETATTR:
> *fsid = req->u.setattr.fs_id;
> *read_only = 0;
> @@ -293,7 +301,8 @@
> case PVFS_SERV_MGMT_NOOP:
> case PVFS_SERV_WRITE_COMPLETION:
> case PVFS_SERV_GETCONFIG:
> - default:
> + case PVFS_SERV_NUM_OPS:
> + case PVFS_SERV_INVALID:
> *fsid = PVFS_FS_ID_NULL;
> *read_only = -1;
> break;
> @@ -498,6 +508,7 @@
> js_p->error_code = 0;
> }
> #endif
> +
> get_fs_intent(s_op->req, &fsid, &rdonly);
> if (fsid != PVFS_FS_ID_NULL)
> {
> Index: pvfs2_src/src/server/prelude.sm
> ===================================================================
> --- pvfs2_src/src/server/prelude.sm (revision 3024)
> +++ pvfs2_src/src/server/prelude.sm (revision 3025)
> @@ -473,6 +473,8 @@
> PVFS_gid translated_gid = s_op->req->credentials.gid;
> PVFS_fs_id fsid;
> int rdonly = -1;
> + int squashed_flag = 0;
> + int skip_acl_flag = 0;
>
> /* moved gossip server debug output to end of state, so we can
> report
> * resulting status value.
> @@ -527,6 +529,7 @@
> if (translate_ids(fsid, s_op->req->credentials.uid,
> s_op->req->credentials.gid,
> &translated_uid, &translated_gid, s_op->addr) == 1)
> {
> + squashed_flag = 1;
> s_op->req->credentials.uid = translated_uid;
> s_op->req->credentials.gid = translated_gid;
> /* in the case of a setattr, translate the ids as
> well right here */
> @@ -629,7 +632,25 @@
> }
> break;
> case PINT_SERVER_CHECK_NONE:
> - js_p->error_code = 0;
> + if(squashed_flag && !rdonly && ((s_op->req->op ==
> PVFS_SERV_IO) ||
> + (s_op->req->op == PVFS_SERV_SMALL_IO) ||
> + (s_op->req->op == PVFS_SERV_TRUNCATE)))
> + {
> + /* special case:
> + * If we have been squashed, deny write permission
> to the
> + * file system. At the datafile level we don't
> have enough
> + * attribute information to figure out if the
> nobody/guest
> + * user has permission to write or not, so we
> disallow all
> + * writes to be safe. Not perfect semantics, but
> better
> + * than being too permissive.
> + */
> + skip_acl_flag = 1;
> + js_p->error_code = -PVFS_EACCES;
> + }
> + else
> + {
> + js_p->error_code = 0;
> + }
> break;
> case PINT_SERVER_CHECK_INVALID:
> js_p->error_code = -PVFS_EINVAL;
> @@ -647,7 +668,7 @@
> PINT_map_server_op_to_string(s_op->req->op),
> js_p->error_code);
> /* If regular checks fail, we need to run acl checks */
> - if (js_p->error_code == -PVFS_EACCES)
> + if (js_p->error_code == -PVFS_EACCES && !skip_acl_flag)
> js_p->error_code = PRELUDE_RUN_ACL_CHECKS;
> return 1;
> }
> Index: pvfs2_src/src/kernel/linux-2.6/inode.c
> ===================================================================
> --- pvfs2_src/src/kernel/linux-2.6/inode.c (revision 3014)
> +++ pvfs2_src/src/kernel/linux-2.6/inode.c (revision 3015)
> @@ -512,7 +512,22 @@
> * properly.
> */
> if (from_create)
> - inode->i_mode = mode;
> + {
> + /* the exception is when we are creating a directory
> that needs
> + * to inherit the setgid bit. That much we need to
> preserve from
> + * the getattr's view of the mode.
> + */
> + if(inode->i_mode & S_ISGID)
> + {
> + gossip_debug(GOSSIP_INODE_DEBUG,
> + "pvfs2_get_custom_inode_commmon: setting SGID
> bit.\n");
> + inode->i_mode = mode | S_ISGID;
> + }
> + else
> + {
> + inode->i_mode = mode;
> + }
> + }
> gossip_debug(GOSSIP_INODE_DEBUG,
> "pvfs2_get_custom_inode_common: inode: %p, inode-
> >i_mode %o\n",
> inode, inode->i_mode);
> _______________________________________________
> Pvfs2-developers mailing list
> Pvfs2-developers at beowulf-underground.org
> http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers
More information about the Pvfs2-developers
mailing list