4:44 a.m.
On 01/07/2017 03:05 PM, John Ferlan wrote:
On 12/19/2016 10:57 AM, Michal Privoznik wrote:
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/security/security_dac.c | 193 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 193 insertions(+)
>
Might have been nice to add some basic/sparse documentation over return
values...
BTW: I'm stopping here since I have some functionality concerns that
would probably be replicated in _selinux and the qemu implementation.
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index b6f75fe1d..0208c1751 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -68,6 +68,117 @@ struct _virSecurityDACCallbackData {
> virSecurityLabelDefPtr secdef;
> };
>
> +typedef struct _virSecurityDACChownItem virSecurityDACChownItem;
> +typedef virSecurityDACChownItem *virSecurityDACChownItemPtr;
> +struct _virSecurityDACChownItem {
> + const char *path;
> + const virStorageSource *src;
> + uid_t uid;
> + gid_t gid;
> +};
> +
> +typedef struct _virSecurityDACChownList virSecurityDACChownList;
> +typedef virSecurityDACChownList *virSecurityDACChownListPtr;
> +struct _virSecurityDACChownList {
> + virSecurityDACDataPtr priv;
> + virSecurityDACChownItemPtr *items;
> + size_t nItems;
> +};
> +
> +
> +virThreadLocal chownList;
> +
> +static int
> +virSecurityDACChownListAppend(virSecurityDACChownListPtr list,
> + const char *path,
> + const virStorageSource *src,
> + uid_t uid,
> + gid_t gid)
> +{
> + virSecurityDACChownItemPtr item;
> +
> + if (VIR_ALLOC(item) < 0)
> + return -1;
> +
> + item->path = path;
> + item->src = src;
> + item->uid = uid;
> + item->gid = gid;
> +
> + if (VIR_APPEND_ELEMENT(list->items, list->nItems, item) < 0) {
> + VIR_FREE(item);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static void
> +virSecurityDACChownListFree(void *opaque)
> +{
> + virSecurityDACChownListPtr list = opaque;
> + size_t i;
> +
> + if (!list)
> + return;
> +
> + for (i = 0; i < list->nItems; i++)
> + VIR_FREE(list->items[i]);
> + VIR_FREE(list);
> +}
> +
> +
This is one example where documenting return values would help... Since
whether 'list' is NULL or not is important from the callers viewpoint.
I also note the caller will do something different when ret > 0, but I
see no way for that to happen...
> +static int
> +virSecurityDACTransactionAppend(const char *path,
> + const virStorageSource *src,
> + uid_t uid,
> + gid_t gid)
> +{
> + virSecurityDACChownListPtr list;
> + int ret = -1;
> +
> + list = virThreadLocalGet(&chownList);
> + if (!list)
> + return 0;
> +
> + if (virSecurityDACChownListAppend(list, path, src, uid, gid) < 0)
> + goto cleanup;
> +
> + ret = 0;
Should this be ret = 1 so the caller "knows" something was put on list?
and thus returns 0 rather than running the rest of the code.
Oh right. I AM a giddy goat.
> + cleanup:
> + return ret;
> +}
> +
> +
> +static int virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv,
> + const virStorageSource *src,
> + const char *path,
> + uid_t uid,
> + gid_t gid);
> +
Some comments here would be nice.
> +static int
> +virSecurityDACTransactionRun(pid_t pid ATTRIBUTE_UNUSED,
> + void *opaque)
> +{
> + virSecurityDACChownListPtr list = opaque;
> + size_t i;
> +
> + ignore_value(virThreadLocalSet(&chownList, NULL));
Not that it should fail, but part of me wonders if a VIR_DEBUG should
issued...
This point in the process is the key step in the whole process - it's
from this point on that we do the "real" work.
Still after a bit of reading on and thinking - should this clearing be
done in Commit instead prior to calling Run
No. For couple of reasons:
1) this function is run from a separate process so unsetting this has no
affect on the parent process (libvirtd).
2) We have to unset the thread local otherwise
virSecurityDACSetOwnershipInternal() would still see the thread local
variable and think there's a transaction set up. Thus instead of doing
real chown() it would just append items to the list.
But okay, I will put this into the comment. And also check for retval of
virThreadLocalSet().
> + for (i = 0; i < list->nItems; i++) {
> + virSecurityDACChownItemPtr item = list->items[i];
> +
> + if (virSecurityDACSetOwnershipInternal(list->priv,
> + item->src,
> + item->path,
> + item->uid,
> + item->gid) < 0)
> + return -1;
If we have a failure, then should we rollback everything done to this
point and of course how would we accomplish that?
The problem being list is available only here and in Commit so how do we
really know how much has been *really* committed and of course does that
matter if our caller can "rollback" for us.
If I read things right, the caller would generate the same "list" in
rollback fashion and rollback everything rather than perhaps being
smarter and rolling back only what was changed. Although I suppose
rolling back everything would/could cause failure at the same point as
Commit.
Exactly. BTW: current code has the same limitation. If an error occurs
in SetSecurityAllLabel() (called from qemuProcessLaunch for instance),
all callers eventually call RestoreSecurityAllLabel() (called rom
qemuProcessStop).
Still we have no real way of restoring/rolling back from the point of
failure unless commit and/or this function returns that mechanism. It
would seem the rollback of RestoreAll labels would/could be overkill.
Agreed.
> + }
> +
> + return 0;
> +}
> +
> +
> /* returns -1 on error, 0 on success */
> int
> virSecurityDACSetUserAndGroup(virSecurityManagerPtr mgr,
> @@ -238,6 +349,13 @@ virSecurityDACProbe(const char *virtDriver ATTRIBUTE_UNUSED)
> static int
> virSecurityDACOpen(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED)
> {
> + if (virThreadLocalInit(&chownList,
> + virSecurityDACChownListFree) < 0) {
> + virReportSystemError(errno, "%s",
> + _("Unable to initialize thread local
variable"));
> + return -1;
> + }
> +
> return 0;
> }
>
> @@ -278,6 +396,69 @@ virSecurityDACPreFork(virSecurityManagerPtr mgr)
> return 0;
> }
>
> +static int
> +virSecurityDACTransactionStart(virSecurityManagerPtr mgr)
> +{
> + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
> + virSecurityDACChownListPtr list;
> +
> + list = virThreadLocalGet(&chownList);
> + if (list) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Another relabel transaction is already
started"));
> + return -1;
> + }
> +
> + if (VIR_ALLOC(list) < 0)
> + return -1;
> +
> + list->priv = priv;
> +
> + if (virThreadLocalSet(&chownList, list) < 0) {
> + virReportSystemError(errno, "%s",
> + _("Unable to set thread local variable"));
> + VIR_FREE(list);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static int
> +virSecurityDACTransactionCommit(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> + pid_t pid)
> +{
> + virSecurityDACChownListPtr list;
> + int ret = 0;
> +
> + list = virThreadLocalGet(&chownList);
> + if (!list)
> + return 0;
Wouldn't !list only happen if there was an error and we're calling
Commit more or less unexpectedly (either after some error that caused
Abort() to be called or a second time)? IOW: Should this be an error?
It should if we want callers to be allowed to call TransactionCommit()
only after prior TransactionStart() call. If you look at the very last
patch of mine there's the following pattern:
if (namespaceEnabled)
transactionStart();
securityDriverCall();
transactionCommit();
transactionAbort();
If the namespace is not enabled both transactionCommit() and
transactionAbort() are no-ops basically. But okay, I can make this
function return an error in this case and thus change pattern to:
if (namespaceEnabled)
transactionStart();
securityDriverCall();
if (namespaceEnabled)
transactionCommit();
transactionAbort();
I just thought that my current approach saves couple of LOC.
> +
> + if (virProcessRunInMountNamespace(pid,
> + virSecurityDACTransactionRun,
> + list) < 0)
> + ret = -1;
> +
> + ignore_value(virThreadLocalSet(&chownList, NULL));
Not that it should fail, but part of me wonders if a VIR_DEBUG should
issued... Secondary to that - should setting to NULL be done before
RunInMountNamespace and not in TransactionRun?
Ah, good point. If I do this before calling RunInMountNamespace() I
don't have to do it there either.
Also for a rollback/error type handling, rather than passing list -
should we pass a structure that includes list and a count of successful
commits. That could be compared against list.nItems and on error
returned to the caller to have some chance at successful rollback.
> + virSecurityDACChownListFree(list);
> + return ret;
> +}
> +
> +static void
> +virSecurityDACTransactionAbort(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED)
> +{
> + virSecurityDACChownListPtr list;
> +
> + list = virThreadLocalGet(&chownList);
> + if (!list)
> + return;
> +
> + ignore_value(virThreadLocalSet(&chownList, NULL));
Not that it should fail, but part of me wonders if a VIR_DEBUG should
issued...
Yep. Will change that here and in all the other lines too.
> + virSecurityDACChownListFree(list);
> +}
> +
> +
> static int
> virSecurityDACSetOwnershipInternal(const virSecurityDACData *priv,
> const virStorageSource *src,
> @@ -287,6 +468,14 @@ virSecurityDACSetOwnershipInternal(const virSecurityDACData
*priv,
> {
> int rc;
>
> + /* Be aware that this function might run in a separate process.
> + * Therefore, any driver state changes would be thrown away. */
> +
> + if ((rc = virSecurityDACTransactionAppend(path, src, uid, gid)) < 0)
> + return -1;
> + else if (rc > 0)
> + return 0;
> +
If we call virSecurityDACChownListAppend and ret = 0, then we still fall
into the code below which from my read of what this code is going to do
may not be expected...
Yeah, for some reason my code was prepared for the behaviour you
describe in the review, but my code does not implement it in the end.
Michal