[PATCH] 49 Retry setting selinux contexts incase of concurrent pkispawn/pkidestroy execution on a machine - Ticket 470
9:22 a.m.
Please review the patch which adds a retry mechanism if a semanage
transaction lock could not be acquired by a pkispawn/pkidestroy
execution. Normally, if a process does not get SELinux transaction lock
it throws an error and quits.
This patch allows pkispawn/pkidestroy to retry 10 times with a 5 second
interval between each try.
--Abhishek
1:26 p.m.
On 4/8/2013 9:22 AM, Abhishek Koneru wrote:
Please review the patch which adds a retry mechanism if a semanage
transaction lock could not be acquired by a pkispawn/pkidestroy
execution. Normally, if a process does not get SELinux transaction lock
it throws an error and quits.
This patch allows pkispawn/pkidestroy to retry 10 times with a 5 second
interval between each try.
Some comments:
1. Is there any document describing that the SELinux transaction would
throw an exception instead of blocking? Or did you already confirm this
with someone?
2. Do you have the link of the ticket that handles this issue in DS?
Please put it in ticket #470 as a reference.
3. Is there a reliable way to test this?
4. The comment for adding SELinux contexts incorrectly says:
# A maximum of 10 tries to delete the SELinux contexts
5. The code checks the type of error based on error message:
if error_message.find("Could not start semanage transaction")
The problem is that the error message might change or be translated so
it would not match. Can we check using the exception class or error code?
6. The timeOut variable is used as a counter for number of tries. It
might be better to use the following variable names for better clarity:
counter = 1
max_tries = 10
7. There's a bug in the patch. Suppose it fails to start transaction
when timeOut is 9, it will enter the exception handling code, then
timeOut is incremented to 10. Since timeOut is not bigger than 10 it
doesn't throw an exception:
if timeOut > 10:
raise
Then it goes back to the loop, but since timeOut is not less than 10 the
loop now will terminate:
while timeOut < 10:
So the code will continue without throwing an error. I think it would be
better to check the timeOut in just one location instead of in two
places to avoid bugs like this.
8. Some trailing whitespaces.
--
Endi S. Dewata
10:51 a.m.
Please review the patch with fixes for comments given by Endi.
--Abhishek
On Mon, 2013-04-08 at 13:26 -0500, Endi Sukma Dewata wrote:
On 4/8/2013 9:22 AM, Abhishek Koneru wrote:
> Please review the patch which adds a retry mechanism if a semanage
> transaction lock could not be acquired by a pkispawn/pkidestroy
> execution. Normally, if a process does not get SELinux transaction lock
> it throws an error and quits.
>
> This patch allows pkispawn/pkidestroy to retry 10 times with a 5 second
> interval between each try.
Some comments:
1. Is there any document describing that the SELinux transaction would
throw an exception instead of blocking? Or did you already confirm this
with someone?
2. Do you have the link of the ticket that handles this issue in DS?
Please put it in ticket #470 as a reference.
-- Added the reference in the comments section of #470
3. Is there a reliable way to test this?
Tested the scenario using the following script.
#! /usr/bin/python
import selinux
if selinux.is_selinux_enabled():
print 'SELinux is enabled'
import seobject
try:
trans = seobject.semanageRecords("targeted")
trans.start()
portRecords = seobject.portRecords()
portRecords.add('8492', "tcp", "s0",
'http_port_t')
trans.finish()
except ValueError as e:
s = str(e)
if s.find('Could not start the semanage transaction') != 1:
print (s)
Executed the same script simultaneously in two terminals. Only one
script completed the transaction, and the other failed throwing a
ValueError.
libsemanage.semanage_get_lock: Could not get direct transaction lock
at /etc/selinux/targeted/modules/semanage.trans.LOCK. (Resource
temporarily unavailable).
ValueError: Could not start semanage transaction
4. The comment for adding SELinux contexts incorrectly says:
# A maximum of 10 tries to delete the SELinux contexts
-- Rectified
5. The code checks the type of error based on error message:
if error_message.find("Could not start semanage transaction")
The problem is that the error message might change or be translated so
it would not match. Can we check using the exception class or error code?
The methods in classes in seobject throw just the ValueErrors if there
is an exception during execution. No error codes returned. Since the
retry has to be done only when the transaction could not begin without
getting the lock, a check on the error message is done.
6. The timeOut variable is used as a counter for number of tries. It
might be better to use the following variable names for better clarity:
counter = 1
max_tries = 10
-- Changed the variable names
7. There's a bug in the patch. Suppose it fails to start
transaction
when timeOut is 9, it will enter the exception handling code, then
timeOut is incremented to 10. Since timeOut is not bigger than 10 it
doesn't throw an exception:
if timeOut > 10:
raise
Then it goes back to the loop, but since timeOut is not less than 10 the
loop now will terminate:
while timeOut < 10:
So the code will continue without throwing an error. I think it would be
better to check the timeOut in just one location instead of in two
places to avoid bugs like this.
-- Fixed. Modified the check in catch clause to, counter == max_tries
8. Some trailing whitespaces.
Fixed.
1:14 p.m.
On discussing with Endi on IRC, done these changes before pushing to
master.
- Changed the while counter < max_tries to while True.
- Error checked as error_message.strip == "Could not start semanage
transaction" rather than doing an 'in' operation.
- Changed the condition counter == max_tries in except block to counter
= max_tries.
--Abhishek
On Tue, 2013-04-09 at 11:51 -0400, Abhishek Koneru wrote:
Please review the patch with fixes for comments given by Endi.
--Abhishek
On Mon, 2013-04-08 at 13:26 -0500, Endi Sukma Dewata wrote:
> On 4/8/2013 9:22 AM, Abhishek Koneru wrote:
> > Please review the patch which adds a retry mechanism if a semanage
> > transaction lock could not be acquired by a pkispawn/pkidestroy
> > execution. Normally, if a process does not get SELinux transaction lock
> > it throws an error and quits.
> >
> > This patch allows pkispawn/pkidestroy to retry 10 times with a 5 second
> > interval between each try.
>
> Some comments:
>
> 1. Is there any document describing that the SELinux transaction would
> throw an exception instead of blocking? Or did you already confirm this
> with someone?
>
> 2. Do you have the link of the ticket that handles this issue in DS?
> Please put it in ticket #470 as a reference.
-- Added the reference in the comments section of #470
>
> 3. Is there a reliable way to test this?
Tested the scenario using the following script.
#! /usr/bin/python
import selinux
if selinux.is_selinux_enabled():
print 'SELinux is enabled'
import seobject
try:
trans = seobject.semanageRecords("targeted")
trans.start()
portRecords = seobject.portRecords()
portRecords.add('8492', "tcp", "s0",
'http_port_t')
trans.finish()
except ValueError as e:
s = str(e)
if s.find('Could not start the semanage transaction') != 1:
print (s)
Executed the same script simultaneously in two terminals. Only one
script completed the transaction, and the other failed throwing a
ValueError.
libsemanage.semanage_get_lock: Could not get direct transaction lock
at /etc/selinux/targeted/modules/semanage.trans.LOCK. (Resource
temporarily unavailable).
ValueError: Could not start semanage transaction
>
>
> 4. The comment for adding SELinux contexts incorrectly says:
>
> # A maximum of 10 tries to delete the SELinux contexts
>
-- Rectified
> 5. The code checks the type of error based on error message:
>
> if error_message.find("Could not start semanage transaction")
>
> The problem is that the error message might change or be translated so
> it would not match. Can we check using the exception class or error code?
>
The methods in classes in seobject throw just the ValueErrors if there
is an exception during execution. No error codes returned. Since the
retry has to be done only when the transaction could not begin without
getting the lock, a check on the error message is done.
> 6. The timeOut variable is used as a counter for number of tries. It
> might be better to use the following variable names for better clarity:
>
> counter = 1
> max_tries = 10
>
-- Changed the variable names
> 7. There's a bug in the patch. Suppose it fails to start transaction
> when timeOut is 9, it will enter the exception handling code, then
> timeOut is incremented to 10. Since timeOut is not bigger than 10 it
> doesn't throw an exception:
>
> if timeOut > 10:
> raise
>
> Then it goes back to the loop, but since timeOut is not less than 10 the
> loop now will terminate:
>
> while timeOut < 10:
>
> So the code will continue without throwing an error. I think it would be
> better to check the timeOut in just one location instead of in two
> places to avoid bugs like this.
-- Fixed. Modified the check in catch clause to, counter == max_tries
>
> 8. Some trailing whitespaces.
>
Fixed.
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
4267
days inactive
4276
days old
3 comments
2 participants
participants (2)
-
Abhishek Koneru -
Endi Sukma Dewata