| Message ID | 58517705.198270.1492699110308@pim.register.it (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Thu, 2017-04-20 at 16:38 +0200, Guido Trentalancia wrote: > Remove semanage read and transaction lock files upon releasing > them. Why? > > Signed-off-by: Guido Trentalancia <guido@trentalancia.net> > --- > src/semanage_store.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff -pruN a/src/semanage_store.c b/src/semanage_store.c > --- a/src/semanage_store.c 2016-10-14 17:31:26.000000000 +0200 > +++ b/src/semanage_store.c 2017-04-03 09:32:24.093627962 +0200 > @@ -1904,6 +1904,7 @@ void semanage_release_trans_lock(semanag > close(sh->u.direct.translock_file_fd); > sh->u.direct.translock_file_fd = -1; > } > + unlink(semanage_files[SEMANAGE_TRANS_LOCK]); > errno = errsv; > } > > @@ -1917,6 +1918,7 @@ void semanage_release_active_lock(semana > close(sh->u.direct.activelock_file_fd); > sh->u.direct.activelock_file_fd = -1; > } > + unlink(semanage_files[SEMANAGE_READ_LOCK]); > errno = errsv; > }
Hello Stephen. Usually, when a lock file is released, the corresponding file is removed from the filesystem for keeping it clean and tidy. I might be wrong... But why not ? If nothing is handling the semanage store, then there shouldn't be a reason for keeping it locked. The presence of a lock file, usually means that the lock is active. Regards, Guido > On the 20th of April 2017 alle 17.44 Stephen Smalley <sds@tycho.nsa.gov> wrote: > > > On Thu, 2017-04-20 at 16:38 +0200, Guido Trentalancia wrote: > > Remove semanage read and transaction lock files upon releasing > > them. > > Why? > > > > > Signed-off-by: Guido Trentalancia <guido@trentalancia.net> > > --- > > src/semanage_store.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff -pruN a/src/semanage_store.c b/src/semanage_store.c > > --- a/src/semanage_store.c 2016-10-14 17:31:26.000000000 +0200 > > +++ b/src/semanage_store.c 2017-04-03 09:32:24.093627962 +0200 > > @@ -1904,6 +1904,7 @@ void semanage_release_trans_lock(semanag > > close(sh->u.direct.translock_file_fd); > > sh->u.direct.translock_file_fd = -1; > > } > > + unlink(semanage_files[SEMANAGE_TRANS_LOCK]); > > errno = errsv; > > } > > > > @@ -1917,6 +1918,7 @@ void semanage_release_active_lock(semana > > close(sh->u.direct.activelock_file_fd); > > sh->u.direct.activelock_file_fd = -1; > > } > > + unlink(semanage_files[SEMANAGE_READ_LOCK]); > > errno = errsv; > > }
Hello and thanks for getting back. If it doesn't have any side-effect (as it should), then I think it's preferable that the filesystem is kept clean. It can be confusing too: because lock files are generally considered "active" when present in the filesystem. Well, you've heard my opinion and you have the very simple patch now. Feel free to do whatever you and the authors like with it... Regards, Guido > On the 20th of April 2017 at 17.56 Stephen Smalley <sds@tycho.nsa.gov> wrote: > > > On Thu, 2017-04-20 at 17:45 +0200, Guido Trentalancia wrote: > > Hello Stephen. > > > > Usually, when a lock file is released, the corresponding file is > > removed from the filesystem for keeping it clean and tidy. > > > > I might be wrong... But why not ? > > > > If nothing is handling the semanage store, then there shouldn't be a > > reason for keeping it locked. The presence of a lock file, usually > > means that the lock is active. > > libsemanage doesn't use the lock files that way; it just uses them as > the object for flock() operations. So the presence of the lock file > means nothing. Removing it just means it will have to be re-created on > the next operation. Not fundamentally opposed, but someone would need > to validate that it doesn't cause any issues. It's been that way > forever. Maybe the original Tresys authors of this code have an > opinion on it.
On Thu, 2017-04-20 at 17:45 +0200, Guido Trentalancia wrote: > Hello Stephen. > > Usually, when a lock file is released, the corresponding file is > removed from the filesystem for keeping it clean and tidy. > > I might be wrong... But why not ? > > If nothing is handling the semanage store, then there shouldn't be a > reason for keeping it locked. The presence of a lock file, usually > means that the lock is active. libsemanage doesn't use the lock files that way; it just uses them as the object for flock() operations. So the presence of the lock file means nothing. Removing it just means it will have to be re-created on the next operation. Not fundamentally opposed, but someone would need to validate that it doesn't cause any issues. It's been that way forever. Maybe the original Tresys authors of this code have an opinion on it.
On Thu, 2017-04-20 at 17:56 +0200, Guido Trentalancia wrote: > Hello and thanks for getting back. > > If it doesn't have any side-effect (as it should), then I think it's > preferable that the filesystem is kept clean. > > It can be confusing too: because lock files are generally considered > "active" when present in the filesystem. > > Well, you've heard my opinion and you have the very simple patch now. > Feel free to do whatever you and the authors like with it... Hmm..see for example the "DON'T unlink" section of: http://world.std.com/~swmcd/steven/tech/flock.html
Yes, I think you are right, it might lead to a race condition because it uses flock() already. It is better to leave things as they are. Please skip this patch ! Regards, Guido > On the 20th of April 2017 at 17.56 Stephen Smalley <sds@tycho.nsa.gov> wrote: > > > On Thu, 2017-04-20 at 17:45 +0200, Guido Trentalancia wrote: > > Hello Stephen. > > > > Usually, when a lock file is released, the corresponding file is > > removed from the filesystem for keeping it clean and tidy. > > > > I might be wrong... But why not ? > > > > If nothing is handling the semanage store, then there shouldn't be a > > reason for keeping it locked. The presence of a lock file, usually > > means that the lock is active. > > libsemanage doesn't use the lock files that way; it just uses them as > the object for flock() operations. So the presence of the lock file > means nothing. Removing it just means it will have to be re-created on > the next operation. Not fundamentally opposed, but someone would need > to validate that it doesn't cause any issues. It's been that way > forever. Maybe the original Tresys authors of this code have an > opinion on it.
On 20/04/17 15:38, Guido Trentalancia wrote: > Remove semanage read and transaction lock files upon releasing > them. What prevents this sequence? A release lock B acquire lock A unlink lock file C create lock file C acquire lock > Signed-off-by: Guido Trentalancia <guido-D1bseh+SzQhuxeB9wqlrNw@public.gmane.org> > --- > src/semanage_store.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff -pruN a/src/semanage_store.c b/src/semanage_store.c > --- a/src/semanage_store.c 2016-10-14 17:31:26.000000000 +0200 > +++ b/src/semanage_store.c 2017-04-03 09:32:24.093627962 +0200 > @@ -1904,6 +1904,7 @@ void semanage_release_trans_lock(semanag > close(sh->u.direct.translock_file_fd); > sh->u.direct.translock_file_fd = -1; > } > + unlink(semanage_files[SEMANAGE_TRANS_LOCK]); > errno = errsv; > } > > @@ -1917,6 +1918,7 @@ void semanage_release_active_lock(semana > close(sh->u.direct.activelock_file_fd); > sh->u.direct.activelock_file_fd = -1; > } > + unlink(semanage_files[SEMANAGE_READ_LOCK]); > errno = errsv; > }
*expands thread Sorry, I see this has already been addressed. On 24/04/17 13:06, Alan Jenkins wrote: > On 20/04/17 15:38, Guido Trentalancia wrote: >> Remove semanage read and transaction lock files upon releasing >> them. > > What prevents this sequence? > > A release lock > B acquire lock > A unlink lock file > C create lock file > C acquire lock > >> Signed-off-by: Guido Trentalancia >> <guido-D1bseh+SzQhuxeB9wqlrNw@public.gmane.org> >> --- >> src/semanage_store.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff -pruN a/src/semanage_store.c b/src/semanage_store.c >> --- a/src/semanage_store.c 2016-10-14 17:31:26.000000000 +0200 >> +++ b/src/semanage_store.c 2017-04-03 09:32:24.093627962 +0200 >> @@ -1904,6 +1904,7 @@ void semanage_release_trans_lock(semanag >> close(sh->u.direct.translock_file_fd); >> sh->u.direct.translock_file_fd = -1; >> } >> + unlink(semanage_files[SEMANAGE_TRANS_LOCK]); >> errno = errsv; >> } >> @@ -1917,6 +1918,7 @@ void semanage_release_active_lock(semana >> close(sh->u.direct.activelock_file_fd); >> sh->u.direct.activelock_file_fd = -1; >> } >> + unlink(semanage_files[SEMANAGE_READ_LOCK]); >> errno = errsv; >> } > > >
Yes, we already discussed this possibile race condition. Usually there is only one system administrator operating on the semanage store, nevertheless it's worth having a robust locking mechanism... This patch either needs further work to avoid using flock() and instead using a simpler file lock mechanism with the added benefit of having a cleaner filesystem without confusing stale files around or we just drop the patch given it is not essential to keep things working. Regards, Guido On the 24th of April 2017 14:08:22 CEST, Alan Jenkins <alan.christopher.jenkins@gmail.com> wrote: >*expands thread > >Sorry, I see this has already been addressed. > > >On 24/04/17 13:06, Alan Jenkins wrote: >> On 20/04/17 15:38, Guido Trentalancia wrote: >>> Remove semanage read and transaction lock files upon releasing >>> them. >> >> What prevents this sequence? >> >> A release lock >> B acquire lock >> A unlink lock file >> C create lock file >> C acquire lock >> >>> Signed-off-by: Guido Trentalancia >>> <guido-D1bseh+SzQhuxeB9wqlrNw@public.gmane.org> >>> --- >>> src/semanage_store.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff -pruN a/src/semanage_store.c b/src/semanage_store.c >>> --- a/src/semanage_store.c 2016-10-14 17:31:26.000000000 +0200 >>> +++ b/src/semanage_store.c 2017-04-03 09:32:24.093627962 +0200 >>> @@ -1904,6 +1904,7 @@ void semanage_release_trans_lock(semanag >>> close(sh->u.direct.translock_file_fd); >>> sh->u.direct.translock_file_fd = -1; >>> } >>> + unlink(semanage_files[SEMANAGE_TRANS_LOCK]); >>> errno = errsv; >>> } >>> @@ -1917,6 +1918,7 @@ void semanage_release_active_lock(semana >>> close(sh->u.direct.activelock_file_fd); >>> sh->u.direct.activelock_file_fd = -1; >>> } >>> + unlink(semanage_files[SEMANAGE_READ_LOCK]); >>> errno = errsv; >>> } >> >> >>
Also, another major benefit of not using flock() comes when using NFS (probably a very rare circumstance, but not entirely impossibile). It is possible to use the presence of a file (with the same name) to indicate an "active" lock: such file should store the PID of the process that is requiring the lock. If a lock is found with a PID that does not exist, then such lock is considered invalid and it is removed. That is it really... Regards, Guido On the 24th of April 2017 19:51:27 CEST, Guido Trentalancia <guido@trentalancia.net> wrote: >Yes, we already discussed this possibile race condition. > >Usually there is only one system administrator operating on the >semanage store, nevertheless it's worth having a robust locking >mechanism... > >This patch either needs further work to avoid using flock() and instead >using a simpler file lock mechanism with the added benefit of having a >cleaner filesystem without confusing stale files around or we just drop >the patch given it is not essential to keep things working. > >Regards, > >Guido > >On the 24th of April 2017 14:08:22 CEST, Alan Jenkins ><alan.christopher.jenkins@gmail.com> wrote: >>*expands thread >> >>Sorry, I see this has already been addressed. >> >> >>On 24/04/17 13:06, Alan Jenkins wrote: >>> On 20/04/17 15:38, Guido Trentalancia wrote: >>>> Remove semanage read and transaction lock files upon releasing >>>> them. >>> >>> What prevents this sequence? >>> >>> A release lock >>> B acquire lock >>> A unlink lock file >>> C create lock file >>> C acquire lock >>> >>>> Signed-off-by: Guido Trentalancia >>>> <guido-D1bseh+SzQhuxeB9wqlrNw@public.gmane.org> >>>> --- >>>> src/semanage_store.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff -pruN a/src/semanage_store.c b/src/semanage_store.c >>>> --- a/src/semanage_store.c 2016-10-14 17:31:26.000000000 +0200 >>>> +++ b/src/semanage_store.c 2017-04-03 09:32:24.093627962 +0200 >>>> @@ -1904,6 +1904,7 @@ void semanage_release_trans_lock(semanag >>>> close(sh->u.direct.translock_file_fd); >>>> sh->u.direct.translock_file_fd = -1; >>>> } >>>> + unlink(semanage_files[SEMANAGE_TRANS_LOCK]); >>>> errno = errsv; >>>> } >>>> @@ -1917,6 +1918,7 @@ void semanage_release_active_lock(semana >>>> close(sh->u.direct.activelock_file_fd); >>>> sh->u.direct.activelock_file_fd = -1; >>>> } >>>> + unlink(semanage_files[SEMANAGE_READ_LOCK]); >>>> errno = errsv; >>>> } >>> >>> >>>
On Tue, 25 Apr 2017 04:38:40 AM Guido Trentalancia wrote: > Also, another major benefit of not using flock() comes when using NFS > (probably a very rare circumstance, but not entirely impossibile). > > It is possible to use the presence of a file (with the same name) to > indicate an "active" lock: such file should store the PID of the process > that is requiring the lock. > > If a lock is found with a PID that does not exist, then such lock is > considered invalid and it is removed. That is it really... Pidfile locking doesn't work well as pids are not unique, you can have a process die and be replaced by another process with the same pid. Also a reboot is expected to have pid conflicts as pids are allocated sequentially and most daemons end up with low numbers. Using a tmpfs for /run solves some of these problems as it's reliably cleared out at boot. Things get even more exciting if you use systemd-nspawn and have multiple pid namespaces on the same system with bind mounts of directories that have pidfiles. Pidfile locking also never works across network filesystems as pids are local to a system. You could have some combination of pid and hostname (as done by some web browsers) but that gets ugly. Really pidfiles are so horrible that one of the noteworthy features of systemd is removing the need for them. Having multiple systems operate with NFS root and a shared /etc/selinux is never going to work well. Even if everything works well (and it probably won't) you will end up with systems that have the policy in /etc/selinux not matching what is running.
diff -pruN a/src/semanage_store.c b/src/semanage_store.c --- a/src/semanage_store.c 2016-10-14 17:31:26.000000000 +0200 +++ b/src/semanage_store.c 2017-04-03 09:32:24.093627962 +0200 @@ -1904,6 +1904,7 @@ void semanage_release_trans_lock(semanag close(sh->u.direct.translock_file_fd); sh->u.direct.translock_file_fd = -1; } + unlink(semanage_files[SEMANAGE_TRANS_LOCK]); errno = errsv; } @@ -1917,6 +1918,7 @@ void semanage_release_active_lock(semana close(sh->u.direct.activelock_file_fd); sh->u.direct.activelock_file_fd = -1; } + unlink(semanage_files[SEMANAGE_READ_LOCK]); errno = errsv; }
Remove semanage read and transaction lock files upon releasing them. Signed-off-by: Guido Trentalancia <guido@trentalancia.net> --- src/semanage_store.c | 2 ++ 1 file changed, 2 insertions(+)