[PATCH] move genhomedircon call out of transaction

[PATCH] move genhomedircon call out of transaction

[Chad Sellers]

[-- Attachment #1: Type: text/plain, Size: 318 bytes --]

Attached is a patch that moves the genhomedircon patch out of the libsemanage 
transaction.  This is necessary since genhomedircon now uses libsemanage, and 
enters a transaction itself.

Chad

-- 
----------------------
Chad Sellers
Tresys Technology, LLC
csellers@tresys.com
(410)290-1411 x117
http://www.tresys.com

[-- Attachment #2: genhomedircon-outside-trans.diff --]
[-- Type: text/x-diff, Size: 895 bytes --]

Index: libsemanage/src/semanage_store.c
===================================================================
RCS file: /cvsroot/selinux/nsa/selinux-usr/libsemanage/src/semanage_store.c,v
retrieving revision 1.21
diff -u -r1.21 semanage_store.c
--- libsemanage/src/semanage_store.c	9 Nov 2005 14:52:55 -0000	1.21
+++ libsemanage/src/semanage_store.c	14 Nov 2005 21:43:09 -0000
@@ -950,11 +950,6 @@
 		goto cleanup;
 	}
 
-	if ((r = semanage_exec_prog(sh, sh->conf->genhomedircon, sh->conf->store_path, "")) != 0) {
-		ERR(sh, "genhomedircon returned error code %d.", r);
-		goto cleanup;
-	}
-
 	retval = 0;
 cleanup:
 	free(storepath);
@@ -1070,6 +1065,12 @@
 		goto cleanup;
 	}
 
+	if ((retval = semanage_exec_prog(sh, sh->conf->genhomedircon, sh->conf->store_path, "")) != 0) {
+		ERR(sh, "genhomedircon returned error code %d.", retval);
+		goto cleanup;
+	}
+
+
 cleanup:
 	return retval;
 

Re: [PATCH] move genhomedircon call out of transaction

[Ivan Gyurdiev]

Chad Sellers wrote:
> Attached is a patch that moves the genhomedircon patch out of the libsemanage 
> transaction.  This is necessary since genhomedircon now uses libsemanage, and 
> enters a transaction itself.
>   
I don't think genhomedircon enters a transaction... nor should it - it's 
a read-only operation with respect to the objects that we manage 
(currently). I could be wrong, but I can't find where it enters a 
transaction in the patch.

Also, even if it did enter transaction, exiting and re-entering 
"immediatly" would create a race condition.



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] move genhomedircon call out of transaction

[Ivan Gyurdiev]

>> Attached is a patch that moves the genhomedircon patch out of the 
>> libsemanage transaction.  This is necessary since genhomedircon now 
>> uses libsemanage, and enters a transaction itself.
>>   
> I don't think genhomedircon enters a transaction... nor should it - 
> it's a read-only operation with respect to the objects that we manage 
> (currently). I could be wrong, but I can't find where it enters a 
> transaction in the patch.
Note: calling multiple semanage query functions outside a transaction is 
a bad idea, because a policy is object is created and destroyed per call 
(while it is cached when in-transaction). However, here only the list() 
function is called (once?), so that seems fine. The active lock guards 
against modification during list().


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] move genhomedircon call out of transaction

[Chad Sellers]

On 11/14/05 6:02 PM, "Ivan Gyurdiev" <ivg2@cornell.edu> wrote:

> Chad Sellers wrote:
>> Attached is a patch that moves the genhomedircon patch out of the libsemanage
>> transaction.  This is necessary since genhomedircon now uses libsemanage, and
>> enters a transaction itself.
>>   
> I don't think genhomedircon enters a transaction... nor should it - it's
> a read-only operation with respect to the objects that we manage
> (currently). I could be wrong, but I can't find where it enters a
> transaction in the patch.
Yep it does.  It calls semanage_user_list(), which calls dbase_list(), which
calls enter_ro(), which gets the lock.

> 
> Also, even if it did enter transaction, exiting and re-entering
> "immediatly" would create a race condition.
Could you expand on this race condition?  genhomedircon is only reading from
libsemanage to generate file_contexts.homedir.  What race condition are you
worried about?

> 
> 

----------------------
Chad Sellers
Tresys Technology, LLC
csellers@tresys.com
(410)290-1411 x117
http://www.tresys.com




--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] move genhomedircon call out of transaction

[Ivan Gyurdiev]

>>> Attached is a patch that moves the genhomedircon patch out of the libsemanage
>>> transaction.  This is necessary since genhomedircon now uses libsemanage, and
>>> enters a transaction itself.
>>>   
>>>       
>> I don't think genhomedircon enters a transaction... nor should it - it's
>> a read-only operation with respect to the objects that we manage
>> (currently). I could be wrong, but I can't find where it enters a
>> transaction in the patch.
>>     
> Yep it does.  It calls semanage_user_list(), which calls dbase_list(), which
> calls enter_ro(), which gets the lock.
>   
I see the problem now - you confused me, because that's the active lock, 
not the transaction lock. It is not entering a transaction - it's 
guarding against a concurrent commit, but you *are* doing a concurrent 
commit, and calling genhomedircon as part of it.
>> Also, even if it did enter transaction, exiting and re-entering
>> "immediatly" would create a race condition.
>>     
> Could you expand on this race condition?  genhomedircon is only reading from
> libsemanage to generate file_contexts.homedir.  What race condition are you
> worried about?
>   
Well... any time you're releasing a lock, and you want to re-acquire it 
before changes are made - that's a potential race condition. I guess in 
this case you don't care if changes are made in the meantime, so there 
is no race condition.

On the other hand, genhomedir called outside libsemanage has a race in 
itself, because changes can be introduced between the time you call 
user_list, and seuser_list. It seems you're moving the place it's called 
to the end of install_sandbox (hard to tell without -p flag), so if it's 
called within libsemanage, that won't be a problem (active lock 
released, transaction lock still held, concurrent commit not possible).


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] move genhomedircon call out of transaction

[Ivan Gyurdiev]

>
> On the other hand, genhomedir called outside libsemanage has a race in 
> itself, because changes can be introduced between the time you call 
> user_list, and seuser_list. It seems you're moving the place it's 
> called to the end of install_sandbox (hard to tell without -p flag), 
> so if it's called within libsemanage, that won't be a problem (active 
> lock released, transaction lock still held, concurrent commit not 
> possible).

Really, genhomedircon should be using a transaction, which is both safer 
(avoids race), and faster (requires only 1 policydb_read, as opposed to 
2). However, I don't know how you'd call it within the original 
transaction...

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] move genhomedircon call out of transaction

[Stephen Smalley]

On Mon, 2005-11-14 at 17:09 -0500, Chad Sellers wrote:
> Attached is a patch that moves the genhomedircon patch out of the libsemanage 
> transaction.  This is necessary since genhomedircon now uses libsemanage, and 
> enters a transaction itself.

Thanks, merged as of libsemanage 1.3.54.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.