* [PATCH] libsemanage: remove lock files @ 2017-04-20 14:38 Guido Trentalancia 2017-04-20 15:44 ` Stephen Smalley 2017-04-24 12:06 ` Alan Jenkins 0 siblings, 2 replies; 19+ messages in thread From: Guido Trentalancia @ 2017-04-20 14:38 UTC (permalink / raw) To: selinux 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(+) 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; } ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] libsemanage: remove lock files 2017-04-20 14:38 [PATCH] libsemanage: remove lock files Guido Trentalancia @ 2017-04-20 15:44 ` Stephen Smalley 2017-04-20 15:45 ` Guido Trentalancia 2017-04-24 12:06 ` Alan Jenkins 1 sibling, 1 reply; 19+ messages in thread From: Stephen Smalley @ 2017-04-20 15:44 UTC (permalink / raw) To: Guido Trentalancia, selinux 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; > } ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] libsemanage: remove lock files 2017-04-20 15:44 ` Stephen Smalley @ 2017-04-20 15:45 ` Guido Trentalancia 2017-04-20 15:56 ` Stephen Smalley 0 siblings, 1 reply; 19+ messages in thread From: Guido Trentalancia @ 2017-04-20 15:45 UTC (permalink / raw) To: selinux 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; > > } ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] libsemanage: remove lock files 2017-04-20 15:45 ` Guido Trentalancia @ 2017-04-20 15:56 ` Stephen Smalley 2017-04-20 15:56 ` Guido Trentalancia 2017-04-20 16:09 ` Guido Trentalancia 0 siblings, 2 replies; 19+ messages in thread From: Stephen Smalley @ 2017-04-20 15:56 UTC (permalink / raw) To: Guido Trentalancia, selinux 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] libsemanage: remove lock files 2017-04-20 15:56 ` Stephen Smalley @ 2017-04-20 15:56 ` Guido Trentalancia 2017-04-20 16:08 ` Stephen Smalley 2017-04-20 16:09 ` Guido Trentalancia 1 sibling, 1 reply; 19+ messages in thread From: Guido Trentalancia @ 2017-04-20 15:56 UTC (permalink / raw) To: selinux 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] libsemanage: remove lock files 2017-04-20 15:56 ` Guido Trentalancia @ 2017-04-20 16:08 ` Stephen Smalley 0 siblings, 0 replies; 19+ messages in thread From: Stephen Smalley @ 2017-04-20 16:08 UTC (permalink / raw) To: Guido Trentalancia, selinux 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] libsemanage: remove lock files 2017-04-20 15:56 ` Stephen Smalley 2017-04-20 15:56 ` Guido Trentalancia @ 2017-04-20 16:09 ` Guido Trentalancia 1 sibling, 0 replies; 19+ messages in thread From: Guido Trentalancia @ 2017-04-20 16:09 UTC (permalink / raw) To: selinux 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] libsemanage: remove lock files 2017-04-20 14:38 [PATCH] libsemanage: remove lock files Guido Trentalancia 2017-04-20 15:44 ` Stephen Smalley @ 2017-04-24 12:06 ` Alan Jenkins 2017-04-24 12:08 ` Alan Jenkins 1 sibling, 1 reply; 19+ messages in thread From: Alan Jenkins @ 2017-04-24 12:06 UTC (permalink / raw) To: Guido Trentalancia, selinux 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; > } ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] libsemanage: remove lock files 2017-04-24 12:06 ` Alan Jenkins @ 2017-04-24 12:08 ` Alan Jenkins 2017-04-24 17:51 ` Guido Trentalancia 0 siblings, 1 reply; 19+ messages in thread From: Alan Jenkins @ 2017-04-24 12:08 UTC (permalink / raw) To: Guido Trentalancia, selinux *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; >> } > > > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] libsemanage: remove lock files 2017-04-24 12:08 ` Alan Jenkins @ 2017-04-24 17:51 ` Guido Trentalancia 2017-04-24 18:38 ` Guido Trentalancia 0 siblings, 1 reply; 19+ messages in thread From: Guido Trentalancia @ 2017-04-24 17:51 UTC (permalink / raw) To: selinux 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; >>> } >> >> >> ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] libsemanage: remove lock files 2017-04-24 17:51 ` Guido Trentalancia @ 2017-04-24 18:38 ` Guido Trentalancia 2017-04-25 6:30 ` Russell Coker 0 siblings, 1 reply; 19+ messages in thread From: Guido Trentalancia @ 2017-04-24 18:38 UTC (permalink / raw) To: selinux 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; >>>> } >>> >>> >>> ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] libsemanage: remove lock files 2017-04-24 18:38 ` Guido Trentalancia @ 2017-04-25 6:30 ` Russell Coker 2017-04-25 20:06 ` [PATCH v2] " Guido Trentalancia 0 siblings, 1 reply; 19+ messages in thread From: Russell Coker @ 2017-04-25 6:30 UTC (permalink / raw) To: selinux 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. -- My Main Blog http://etbe.coker.com.au/ My Documents Blog http://doc.coker.com.au/ ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2] libsemanage: remove lock files 2017-04-25 6:30 ` Russell Coker @ 2017-04-25 20:06 ` Guido Trentalancia 2017-04-25 20:35 ` [PATCH v3] " Guido Trentalancia 2017-04-26 0:31 ` [PATCH v2] " Russell Coker 0 siblings, 2 replies; 19+ messages in thread From: Guido Trentalancia @ 2017-04-25 20:06 UTC (permalink / raw) To: selinux Hello. What follows is a reply to Russell comments and a new, hopefully better, version of the original patch. On Tue, 25/04/2017 at 16.30 +1000, Russell Coker wrote: > 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. No, I didn't mean the complicate circumstance of NFS shared amongst multiple systems. I only meant the simpler (and most common) situation where the NFS is mounted for only one system, therefore PIDs are unique and there is no need to include the hostname. > Really pidfiles are so horrible that one of the noteworthy features > of systemd > is removing the need for them. I am not that pessimistic about locking with aid of PIDs. To be honest, the current situation has more drawbacks in my opinion. In particular, I don't like the fact that it leaves unused lock files around the filesystem. > 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. Please forget sharing NFS. I meant dedicated NFS. Anyway, I have created a new version of the patch that should probably improve the previous race condition. If you have a way of testing a dedicated store over NFS, then I'd like to hear back from you the result of such testing ! But, if you are not interested in testing it, then never mind... --- Do not use flock() for file locking, but instead use generic text files that keep track of the process ID (PID) of the locking process. Remove semanage read and transaction lock files upon releasing them. Signed-off-by: Guido Trentalancia <guido@trentalancia.net> --- src/Makefile | 2 src/semanage_store.c | 205 ++++++++++++++++++++++++++++++++++++++------------- 2 files changed, 154 insertions(+), 53 deletions(-) --- a/src/Makefile 2016-10-14 17:31:26.000000000 +0200 +++ b/src/Makefile 2017-04-25 00:54:58.916827639 +0200 @@ -91,7 +91,7 @@ $(LIBA): $(OBJS) $(RANLIB) $@ $(LIBSO): $(LOBJS) - $(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lsepol -laudit -lselinux -lbz2 -lustr -L$(LIBDIR) -Wl,-soname,$(LIBSO),--version-script=libsemanage.map,-z,defs + $(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lm -lsepol -laudit -lselinux -lbz2 -lustr -L$(LIBDIR) -Wl,-soname,$(LIBSO),--version-script=libsemanage.map,-z,defs ln -sf $@ $(TARGET) $(LIBPC): $(LIBPC).in ../VERSION --- a/src/semanage_store.c 2016-10-14 17:31:26.000000000 +0200 +++ b/src/semanage_store.c 2017-04-25 21:52:51.977563955 +0200 @@ -45,6 +45,8 @@ typedef struct dbase_policydb dbase_t; #include <dirent.h> #include <errno.h> #include <fcntl.h> +#include <math.h> +#include <signal.h> #include <stdio.h> #include <stdio_ext.h> #include <stdlib.h> @@ -52,11 +54,24 @@ typedef struct dbase_policydb dbase_t; #include <unistd.h> #include <sys/file.h> #include <sys/stat.h> +#include <sys/sysctl.h> #include <sys/types.h> #include <sys/wait.h> #include <limits.h> #include <libgen.h> +#include <linux/sysctl.h> + +#ifndef CONFIG_BASE_SMALL +#define CONFIG_BASE_SMALL 0 +#endif + +#include <linux/threads.h> + +#ifndef PID_MAX_DEFAULT +#define PID_MAX_DEFAULT 32768 +#endif + #include "debug.h" #include "utilities.h" @@ -76,6 +91,8 @@ enum semanage_file_defs { static char *semanage_paths[SEMANAGE_NUM_STORES][SEMANAGE_STORE_NUM_PATHS]; static char *semanage_files[SEMANAGE_NUM_FILES] = { NULL }; static int semanage_paths_initialized = 0; +static int pid_max; +static ssize_t pid_max_length; /* These are paths relative to the bottom of the module store */ static const char *semanage_relative_files[SEMANAGE_NUM_FILES] = { @@ -425,8 +442,23 @@ cleanup: int semanage_check_init(semanage_handle_t *sh, const char *prefix) { int rc; + int fd; + char root[PATH_MAX]; + ssize_t amount_read; + if (semanage_paths_initialized == 0) { - char root[PATH_MAX]; + pid_max = PID_MAX_DEFAULT; + pid_max_length = ceil(log10(PID_MAX_DEFAULT + 1)); + + fd = open("/proc/sys/kernel/pid_max", O_RDONLY); + if (fd > 0) { + char sysctlstring[pid_max_length]; + amount_read = read(fd, sysctlstring, pid_max_length); + if (amount_read > 0) { + pid_max = atoi(sysctlstring); + pid_max_length = ceil(log10(pid_max + 1)); + } + } rc = snprintf(root, sizeof(root), @@ -526,16 +558,23 @@ char *semanage_conf_path(void) /**************** functions that create module store ***************/ -/* Check that the semanage store exists. If 'create' is non-zero then - * create the directories. Returns 0 if module store exists (either - * already or just created), -1 if does not exist or could not be - * read, or -2 if it could not create the store. */ +/* Check that the semanage store exists and that the read lock can be + * taken. If 'create' is non-zero then it creates the directories + * and the lock file. Returns 0 if the module store exists (either + * already or just created) and the read lock can be taken, -1 if it + * does not exist or it is not possible to read from it, or -2 if it + * could not create the store or it could not take the lock file. */ int semanage_create_store(semanage_handle_t * sh, int create) { struct stat sb; int mode_mask = R_OK | W_OK | X_OK; const char *path = semanage_files[SEMANAGE_ROOT]; int fd; + pid_t pid, lock_pid; + char *pid_string, *lock_pid_string; + size_t pid_length; + ssize_t pid_bytes; + int invalid_lock = 0; if (stat(path, &sb) == -1) { if (errno == ENOENT && create) { @@ -605,26 +644,82 @@ int semanage_create_store(semanage_handl return -1; } } + pid = getpid(); + pid_string = malloc(pid_max_length * sizeof(char)); + sprintf(pid_string, "%d", pid); + pid_length = strlen(pid_string); path = semanage_files[SEMANAGE_READ_LOCK]; if (stat(path, &sb) == -1) { if (errno == ENOENT && create) { if ((fd = creat(path, S_IRUSR | S_IWUSR)) == -1) { ERR(sh, "Could not create lock file at %s.", path); + free(pid_string); return -2; } + pid_bytes = write(fd, pid_string, pid_length); close(fd); + free(pid_string); + if (pid_bytes == (ssize_t) pid_length) + return 0; + else { + ERR(sh, "Could not create lock file at %s.", path); + return -2; + } } else { ERR(sh, "Could not read lock file at %s.", path); + free(pid_string); return -1; } } else { if (!S_ISREG(sb.st_mode) || access(path, R_OK | W_OK) == -1) { ERR(sh, "Could not access lock file at %s.", path); + free(pid_string); return -1; - } + } else { + fd = open(path, O_RDWR); + if (fd > 0) { + lock_pid_string = malloc(pid_max_length * sizeof(char)); + pid_bytes = read(fd, lock_pid_string, pid_max_length); + if (pid_bytes > 0) { + lock_pid = (pid_t) atoi(lock_pid_string); + if (lock_pid > pid_max) + invalid_lock = 1; + else if (kill(lock_pid, 0) != 0) + if (errno == ESRCH) + invalid_lock = 1; + + if (!invalid_lock) { + ERR(sh, "Could not create lock file at %s.", path); + close(fd); + free(pid_string); + free(lock_pid_string); + return -2; + } + } else + invalid_lock = 1; + + if (invalid_lock) { + pid_bytes = pwrite(fd, pid_string, pid_length, 0); + close(fd); + free(pid_string); + free(lock_pid_string); + if (pid_bytes == (ssize_t) pid_length) + return 0; + else { + ERR(sh, "Could not create lock file at %s.", path); + return -2; + } + } else { + ERR(sh, "Could not create lock file at %s.", path); + close(fd); + free(pid_string); + free(lock_pid_string); + return -2; + } + } + } } - return 0; } /* returns <0 if the active store cannot be read or doesn't exist @@ -1788,64 +1883,74 @@ int semanage_install_sandbox(semanage_ha static int semanage_get_lock(semanage_handle_t * sh, const char *lock_name, const char *lock_file) { + pid_t pid, lock_pid; + char *pid_string, *lock_pid_string; int fd; - struct timeval origtime, curtime; + size_t pid_length; + ssize_t pid_bytes; int got_lock = 0; + int overwritten_lock = 0; - if ((fd = open(lock_file, O_RDONLY)) == -1) { - if ((fd = - open(lock_file, O_RDWR | O_CREAT | O_TRUNC, - S_IRUSR | S_IWUSR)) == -1) { + pid = getpid(); + pid_string = malloc(pid_max_length * sizeof(char)); + sprintf(pid_string, "%d", pid); + pid_length = strlen(pid_string); + if ((fd = open(lock_file, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR)) == -1) { ERR(sh, "Could not open direct %s at %s.", lock_name, lock_file); + free(pid_string); return -1; + } else { + lock_pid_string = malloc(pid_max_length * sizeof(char)); + pid_bytes = pread(fd, lock_pid_string, pid_max_length, 0); + if (pid_bytes == 0) { + overwritten_lock = 1; + pid_bytes = write(fd, pid_string, pid_length); + if (pid_bytes == (ssize_t) pid_length) + got_lock = 1; + } else { + lock_pid = (pid_t) atoi(lock_pid_string); + if (lock_pid > pid_max) { + overwritten_lock = 1; + pid_bytes = pwrite(fd, pid_string, pid_length, 0); + if (pid_bytes == (ssize_t) pid_length) + got_lock = 1; + } else { + if (lock_pid = pid) + got_lock = 1; + else if (kill(lock_pid, 0) != 0) + if (errno == ESRCH) { + overwritten_lock = 1; + pid_bytes = pwrite(fd, pid_string, pid_length, 0); + if (pid_bytes == (ssize_t) pid_length) + got_lock = 1; + } + } } } - if (fcntl(fd, F_SETFD, FD_CLOEXEC) < 0) { - ERR(sh, "Could not set close-on-exec for %s at %s.", lock_name, - lock_file); + + if (!got_lock) { close(fd); + free(pid_string); + free(lock_pid_string); + if (overwritten_lock) + unlink(lock_file); + ERR(sh, "Could not get direct %s at %s.", lock_name, lock_file); return -1; } - if (sh->timeout == 0) { - /* return immediately */ - origtime.tv_sec = 0; - } else { - origtime.tv_sec = sh->timeout; - } - origtime.tv_usec = 0; - do { - curtime.tv_sec = 1; - curtime.tv_usec = 0; - if (flock(fd, LOCK_EX | LOCK_NB) == 0) { - got_lock = 1; - break; - } else if (errno != EAGAIN) { - ERR(sh, "Error obtaining direct %s at %s.", lock_name, - lock_file); - close(fd); - return -1; - } - if (origtime.tv_sec > 0 || sh->timeout == -1) { - if (select(0, NULL, NULL, NULL, &curtime) == -1) { - if (errno == EINTR) { - continue; - } - ERR(sh, - "Error while waiting to get direct %s at %s.", - lock_name, lock_file); - close(fd); - return -1; - } - origtime.tv_sec--; - } - } while (origtime.tv_sec > 0 || sh->timeout == -1); - if (!got_lock) { - ERR(sh, "Could not get direct %s at %s.", lock_name, lock_file); + if (fcntl(fd, F_SETFD, FD_CLOEXEC) < 0) { + ERR(sh, "Could not set close-on-exec for %s at %s.", lock_name, + lock_file); close(fd); + free(pid_string); + free(lock_pid_string); + unlink(lock_file); return -1; } + + free(pid_string); + free(lock_pid_string); return fd; } @@ -1894,29 +1999,27 @@ int semanage_get_active_lock(semanage_ha } } -/* Releases the transaction lock. Does nothing if there was not one already - * there. */ +/* Releases the transaction lock: the lock file is removed */ void semanage_release_trans_lock(semanage_handle_t * sh) { int errsv = errno; if (sh->u.direct.translock_file_fd >= 0) { - flock(sh->u.direct.translock_file_fd, LOCK_UN); close(sh->u.direct.translock_file_fd); sh->u.direct.translock_file_fd = -1; } + unlink(semanage_files[SEMANAGE_TRANS_LOCK]); errno = errsv; } -/* Releases the read lock. Does nothing if there was not one already - * there. */ +/* Releases the read lock: the lock file is removed */ void semanage_release_active_lock(semanage_handle_t * sh) { int errsv = errno; if (sh->u.direct.activelock_file_fd >= 0) { - flock(sh->u.direct.activelock_file_fd, LOCK_UN); close(sh->u.direct.activelock_file_fd); sh->u.direct.activelock_file_fd = -1; } + unlink(semanage_files[SEMANAGE_READ_LOCK]); errno = errsv; } ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3] libsemanage: remove lock files 2017-04-25 20:06 ` [PATCH v2] " Guido Trentalancia @ 2017-04-25 20:35 ` Guido Trentalancia 2017-04-26 12:03 ` Jason Zaman 2017-04-26 0:31 ` [PATCH v2] " Russell Coker 1 sibling, 1 reply; 19+ messages in thread From: Guido Trentalancia @ 2017-04-25 20:35 UTC (permalink / raw) To: selinux Do not use flock() for file locking, but instead use generic text files that keep track of the process ID (PID) of the locking process. Remove semanage read and transaction lock files upon releasing them. This third version fixes a bug in the previous version and also applies cleanly to the latest git tree. Signed-off-by: Guido Trentalancia <guido@trentalancia.net> --- src/Makefile | 2 src/semanage_store.c | 214 +++++++++++++++++++++++++++++++++++++-------------- 2 files changed, 160 insertions(+), 56 deletions(-) --- a/src/Makefile 2017-04-25 22:27:38.105555427 +0200 +++ b/src/Makefile 2017-04-25 22:28:58.512555098 +0200 @@ -91,7 +91,7 @@ $(LIBA): $(OBJS) $(RANLIB) $@ $(LIBSO): $(LOBJS) - $(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lsepol -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version-script=libsemanage.map,-z,defs + $(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lm -lsepol -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version-script=libsemanage.map,-z,defs ln -sf $@ $(TARGET) $(LIBPC): $(LIBPC).in ../VERSION --- a/src/semanage_store.c 2017-04-20 16:30:21.218209972 +0200 +++ b/src/semanage_store.c 2017-04-25 22:24:35.883556172 +0200 @@ -45,6 +45,8 @@ typedef struct dbase_policydb dbase_t; #include <dirent.h> #include <errno.h> #include <fcntl.h> +#include <math.h> +#include <signal.h> #include <stdio.h> #include <stdio_ext.h> #include <stdlib.h> @@ -52,11 +54,24 @@ typedef struct dbase_policydb dbase_t; #include <unistd.h> #include <sys/file.h> #include <sys/stat.h> +#include <sys/sysctl.h> #include <sys/types.h> #include <sys/wait.h> #include <limits.h> #include <libgen.h> +#include <linux/sysctl.h> + +#ifndef CONFIG_BASE_SMALL +#define CONFIG_BASE_SMALL 0 +#endif + +#include <linux/threads.h> + +#ifndef PID_MAX_DEFAULT +#define PID_MAX_DEFAULT 32768 +#endif + #include "debug.h" #include "utilities.h" @@ -76,6 +91,8 @@ enum semanage_file_defs { static char *semanage_paths[SEMANAGE_NUM_STORES][SEMANAGE_STORE_NUM_PATHS]; static char *semanage_files[SEMANAGE_NUM_FILES] = { NULL }; static int semanage_paths_initialized = 0; +static int pid_max; +static ssize_t pid_max_length; /* These are paths relative to the bottom of the module store */ static const char *semanage_relative_files[SEMANAGE_NUM_FILES] = { @@ -427,8 +442,23 @@ cleanup: int semanage_check_init(semanage_handle_t *sh, const char *prefix) { int rc; + int fd; + char root[PATH_MAX]; + ssize_t amount_read; + if (semanage_paths_initialized == 0) { - char root[PATH_MAX]; + pid_max = PID_MAX_DEFAULT; + pid_max_length = ceil(log10(PID_MAX_DEFAULT + 1)); + + fd = open("/proc/sys/kernel/pid_max", O_RDONLY); + if (fd > 0) { + char sysctlstring[pid_max_length]; + amount_read = read(fd, sysctlstring, pid_max_length); + if (amount_read > 0) { + pid_max = atoi(sysctlstring); + pid_max_length = ceil(log10(pid_max + 1)); + } + } rc = snprintf(root, sizeof(root), @@ -528,16 +558,23 @@ char *semanage_conf_path(void) /**************** functions that create module store ***************/ -/* Check that the semanage store exists. If 'create' is non-zero then - * create the directories. Returns 0 if module store exists (either - * already or just created), -1 if does not exist or could not be - * read, or -2 if it could not create the store. */ +/* Check that the semanage store exists and that the read lock can be + * taken. If 'create' is non-zero then it creates the directories + * and the lock file. Returns 0 if the module store exists (either + * already or just created) and the read lock can be taken, -1 if it + * does not exist or it is not possible to read from it, or -2 if it + * could not create the store or it could not take the lock file. */ int semanage_create_store(semanage_handle_t * sh, int create) { struct stat sb; int mode_mask = R_OK | W_OK | X_OK; const char *path = semanage_files[SEMANAGE_ROOT]; int fd; + pid_t pid, lock_pid; + char *pid_string, *lock_pid_string; + size_t pid_length; + ssize_t pid_bytes; + int invalid_lock = 0; if (stat(path, &sb) == -1) { if (errno == ENOENT && create) { @@ -607,24 +644,81 @@ int semanage_create_store(semanage_handl return -1; } } + pid = getpid(); + pid_string = malloc(pid_max_length * sizeof(char)); + sprintf(pid_string, "%d", pid); + pid_length = strlen(pid_string); path = semanage_files[SEMANAGE_READ_LOCK]; if (stat(path, &sb) == -1) { if (errno == ENOENT && create) { if ((fd = creat(path, S_IRUSR | S_IWUSR)) == -1) { ERR(sh, "Could not create lock file at %s.", path); + free(pid_string); return -2; } + pid_bytes = write(fd, pid_string, pid_length); close(fd); + free(pid_string); + if (pid_bytes == (ssize_t) pid_length) + return 0; + else { + ERR(sh, "Could not create lock file at %s.", path); + return -2; + } } else { ERR(sh, "Could not read lock file at %s.", path); + free(pid_string); return -1; } } else { if (!S_ISREG(sb.st_mode) || access(path, R_OK | W_OK) == -1) { ERR(sh, "Could not access lock file at %s.", path); + free(pid_string); return -1; - } + } else { + fd = open(path, O_RDWR); + if (fd > 0) { + lock_pid_string = malloc(pid_max_length * sizeof(char)); + pid_bytes = read(fd, lock_pid_string, pid_max_length); + if (pid_bytes > 0) { + lock_pid = (pid_t) atoi(lock_pid_string); + if (lock_pid > pid_max) + invalid_lock = 1; + else if (kill(lock_pid, 0) != 0) + if (errno == ESRCH) + invalid_lock = 1; + + if (!invalid_lock) { + ERR(sh, "Could not create lock file at %s.", path); + close(fd); + free(pid_string); + free(lock_pid_string); + return -2; + } + } else + invalid_lock = 1; + + if (invalid_lock) { + pid_bytes = pwrite(fd, pid_string, pid_length, 0); + close(fd); + free(pid_string); + free(lock_pid_string); + if (pid_bytes == (ssize_t) pid_length) + return 0; + else { + ERR(sh, "Could not create lock file at %s.", path); + return -2; + } + } else { + ERR(sh, "Could not create lock file at %s.", path); + close(fd); + free(pid_string); + free(lock_pid_string); + return -2; + } + } + } } return 0; } @@ -1795,64 +1884,74 @@ int semanage_install_sandbox(semanage_ha static int semanage_get_lock(semanage_handle_t * sh, const char *lock_name, const char *lock_file) { + pid_t pid, lock_pid; + char *pid_string, *lock_pid_string; int fd; - struct timeval origtime, curtime; + size_t pid_length; + ssize_t pid_bytes; int got_lock = 0; + int overwritten_lock = 0; - if ((fd = open(lock_file, O_RDONLY)) == -1) { - if ((fd = - open(lock_file, O_RDWR | O_CREAT | O_TRUNC, - S_IRUSR | S_IWUSR)) == -1) { + pid = getpid(); + pid_string = malloc(pid_max_length * sizeof(char)); + sprintf(pid_string, "%d", pid); + pid_length = strlen(pid_string); + if ((fd = open(lock_file, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR)) == -1) { ERR(sh, "Could not open direct %s at %s.", lock_name, lock_file); + free(pid_string); return -1; + } else { + lock_pid_string = malloc(pid_max_length * sizeof(char)); + pid_bytes = pread(fd, lock_pid_string, pid_max_length, 0); + if (pid_bytes == 0) { + overwritten_lock = 1; + pid_bytes = write(fd, pid_string, pid_length); + if (pid_bytes == (ssize_t) pid_length) + got_lock = 1; + } else { + lock_pid = (pid_t) atoi(lock_pid_string); + if (lock_pid > pid_max) { + overwritten_lock = 1; + pid_bytes = pwrite(fd, pid_string, pid_length, 0); + if (pid_bytes == (ssize_t) pid_length) + got_lock = 1; + } else { + if (lock_pid == pid) + got_lock = 1; + else if (kill(lock_pid, 0) != 0) + if (errno == ESRCH) { + overwritten_lock = 1; + pid_bytes = pwrite(fd, pid_string, pid_length, 0); + if (pid_bytes == (ssize_t) pid_length) + got_lock = 1; + } + } } } - if (fcntl(fd, F_SETFD, FD_CLOEXEC) < 0) { - ERR(sh, "Could not set close-on-exec for %s at %s.", lock_name, - lock_file); + + if (!got_lock) { close(fd); + free(pid_string); + free(lock_pid_string); + if (overwritten_lock) + unlink(lock_file); + ERR(sh, "Could not get direct %s at %s.", lock_name, lock_file); return -1; } - if (sh->timeout == 0) { - /* return immediately */ - origtime.tv_sec = 0; - } else { - origtime.tv_sec = sh->timeout; - } - origtime.tv_usec = 0; - do { - curtime.tv_sec = 1; - curtime.tv_usec = 0; - if (flock(fd, LOCK_EX | LOCK_NB) == 0) { - got_lock = 1; - break; - } else if (errno != EAGAIN) { - ERR(sh, "Error obtaining direct %s at %s.", lock_name, - lock_file); - close(fd); - return -1; - } - if (origtime.tv_sec > 0 || sh->timeout == -1) { - if (select(0, NULL, NULL, NULL, &curtime) == -1) { - if (errno == EINTR) { - continue; - } - ERR(sh, - "Error while waiting to get direct %s at %s.", - lock_name, lock_file); - close(fd); - return -1; - } - origtime.tv_sec--; - } - } while (origtime.tv_sec > 0 || sh->timeout == -1); - if (!got_lock) { - ERR(sh, "Could not get direct %s at %s.", lock_name, lock_file); + if (fcntl(fd, F_SETFD, FD_CLOEXEC) < 0) { + ERR(sh, "Could not set close-on-exec for %s at %s.", lock_name, + lock_file); close(fd); + free(pid_string); + free(lock_pid_string); + unlink(lock_file); return -1; } + + free(pid_string); + free(lock_pid_string); return fd; } @@ -1901,29 +2000,27 @@ int semanage_get_active_lock(semanage_ha } } -/* Releases the transaction lock. Does nothing if there was not one already - * there. */ +/* Releases the transaction lock: the lock file is removed */ void semanage_release_trans_lock(semanage_handle_t * sh) { int errsv = errno; if (sh->u.direct.translock_file_fd >= 0) { - flock(sh->u.direct.translock_file_fd, LOCK_UN); close(sh->u.direct.translock_file_fd); sh->u.direct.translock_file_fd = -1; } + unlink(semanage_files[SEMANAGE_TRANS_LOCK]); errno = errsv; } -/* Releases the read lock. Does nothing if there was not one already - * there. */ +/* Releases the read lock: the lock file is removed */ void semanage_release_active_lock(semanage_handle_t * sh) { int errsv = errno; if (sh->u.direct.activelock_file_fd >= 0) { - flock(sh->u.direct.activelock_file_fd, LOCK_UN); close(sh->u.direct.activelock_file_fd); sh->u.direct.activelock_file_fd = -1; } + unlink(semanage_files[SEMANAGE_READ_LOCK]); errno = errsv; } ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] libsemanage: remove lock files 2017-04-25 20:35 ` [PATCH v3] " Guido Trentalancia @ 2017-04-26 12:03 ` Jason Zaman 2017-04-26 12:56 ` Stephen Smalley 0 siblings, 1 reply; 19+ messages in thread From: Jason Zaman @ 2017-04-26 12:03 UTC (permalink / raw) To: Guido Trentalancia; +Cc: selinux On Tue, Apr 25, 2017 at 10:35:17PM +0200, Guido Trentalancia wrote: > Do not use flock() for file locking, but instead use generic text files > that keep track of the process ID (PID) of the locking process. > > Remove semanage read and transaction lock files upon releasing > them. > > This third version fixes a bug in the previous version and also applies > cleanly to the latest git tree. > > Signed-off-by: Guido Trentalancia <guido@trentalancia.net> > --- > src/Makefile | 2 > src/semanage_store.c | 214 +++++++++++++++++++++++++++++++++++++-------------- > 2 files changed, 160 insertions(+), 56 deletions(-) > > --- a/src/Makefile 2017-04-25 22:27:38.105555427 +0200 > +++ b/src/Makefile 2017-04-25 22:28:58.512555098 +0200 > @@ -91,7 +91,7 @@ $(LIBA): $(OBJS) > $(RANLIB) $@ > > $(LIBSO): $(LOBJS) > - $(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lsepol -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version-script=libsemanage.map,-z,defs > + $(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lm -lsepol -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version-script=libsemanage.map,-z,defs > ln -sf $@ $(TARGET) > > $(LIBPC): $(LIBPC).in ../VERSION > --- a/src/semanage_store.c 2017-04-20 16:30:21.218209972 +0200 > +++ b/src/semanage_store.c 2017-04-25 22:24:35.883556172 +0200 > @@ -45,6 +45,8 @@ typedef struct dbase_policydb dbase_t; > #include <dirent.h> > #include <errno.h> > #include <fcntl.h> > +#include <math.h> > +#include <signal.h> > #include <stdio.h> > #include <stdio_ext.h> > #include <stdlib.h> > @@ -52,11 +54,24 @@ typedef struct dbase_policydb dbase_t; > #include <unistd.h> > #include <sys/file.h> > #include <sys/stat.h> > +#include <sys/sysctl.h> > #include <sys/types.h> > #include <sys/wait.h> > #include <limits.h> > #include <libgen.h> > > +#include <linux/sysctl.h> > + > +#ifndef CONFIG_BASE_SMALL > +#define CONFIG_BASE_SMALL 0 > +#endif > + > +#include <linux/threads.h> > + > +#ifndef PID_MAX_DEFAULT > +#define PID_MAX_DEFAULT 32768 > +#endif > + > #include "debug.h" > #include "utilities.h" > > @@ -76,6 +91,8 @@ enum semanage_file_defs { > static char *semanage_paths[SEMANAGE_NUM_STORES][SEMANAGE_STORE_NUM_PATHS]; > static char *semanage_files[SEMANAGE_NUM_FILES] = { NULL }; > static int semanage_paths_initialized = 0; > +static int pid_max; > +static ssize_t pid_max_length; > > /* These are paths relative to the bottom of the module store */ > static const char *semanage_relative_files[SEMANAGE_NUM_FILES] = { > @@ -427,8 +442,23 @@ cleanup: > int semanage_check_init(semanage_handle_t *sh, const char *prefix) > { > int rc; > + int fd; > + char root[PATH_MAX]; > + ssize_t amount_read; > + > if (semanage_paths_initialized == 0) { > - char root[PATH_MAX]; > + pid_max = PID_MAX_DEFAULT; > + pid_max_length = ceil(log10(PID_MAX_DEFAULT + 1)); > + > + fd = open("/proc/sys/kernel/pid_max", O_RDONLY); > + if (fd > 0) { > + char sysctlstring[pid_max_length]; > + amount_read = read(fd, sysctlstring, pid_max_length); > + if (amount_read > 0) { > + pid_max = atoi(sysctlstring); > + pid_max_length = ceil(log10(pid_max + 1)); > + } > + } > > rc = snprintf(root, > sizeof(root), > @@ -528,16 +558,23 @@ char *semanage_conf_path(void) > > /**************** functions that create module store ***************/ > > -/* Check that the semanage store exists. If 'create' is non-zero then > - * create the directories. Returns 0 if module store exists (either > - * already or just created), -1 if does not exist or could not be > - * read, or -2 if it could not create the store. */ > +/* Check that the semanage store exists and that the read lock can be > + * taken. If 'create' is non-zero then it creates the directories > + * and the lock file. Returns 0 if the module store exists (either > + * already or just created) and the read lock can be taken, -1 if it > + * does not exist or it is not possible to read from it, or -2 if it > + * could not create the store or it could not take the lock file. */ > int semanage_create_store(semanage_handle_t * sh, int create) > { > struct stat sb; > int mode_mask = R_OK | W_OK | X_OK; > const char *path = semanage_files[SEMANAGE_ROOT]; > int fd; > + pid_t pid, lock_pid; > + char *pid_string, *lock_pid_string; > + size_t pid_length; > + ssize_t pid_bytes; > + int invalid_lock = 0; > > if (stat(path, &sb) == -1) { > if (errno == ENOENT && create) { > @@ -607,24 +644,81 @@ int semanage_create_store(semanage_handl > return -1; > } > } > + pid = getpid(); > + pid_string = malloc(pid_max_length * sizeof(char)); > + sprintf(pid_string, "%d", pid); > + pid_length = strlen(pid_string); > path = semanage_files[SEMANAGE_READ_LOCK]; > if (stat(path, &sb) == -1) { > if (errno == ENOENT && create) { > if ((fd = creat(path, S_IRUSR | S_IWUSR)) == -1) { > ERR(sh, "Could not create lock file at %s.", > path); > + free(pid_string); > return -2; > } > + pid_bytes = write(fd, pid_string, pid_length); I'm pretty sure there is a race here. and another one between stat() and creat(). you can have two processes create and truncate the file then one write on top of the other. leaving lock files around is definitely the safest option. and are 0 bytes so its not like you're wasting any disk space. Overall I disagree with changing this just because a random file is left around. NFS may be a legit reason to change but as russel said, I think locks work somewhat over NFS too. Also, NFS mounting /home or /usr is common but /var is pretty commonly not NFS mounted. -- Jason > close(fd); > + free(pid_string); > + if (pid_bytes == (ssize_t) pid_length) > + return 0; > + else { > + ERR(sh, "Could not create lock file at %s.", path); > + return -2; > + } > } else { > ERR(sh, "Could not read lock file at %s.", path); > + free(pid_string); > return -1; > } > } else { > if (!S_ISREG(sb.st_mode) || access(path, R_OK | W_OK) == -1) { > ERR(sh, "Could not access lock file at %s.", path); > + free(pid_string); > return -1; > - } > + } else { > + fd = open(path, O_RDWR); > + if (fd > 0) { > + lock_pid_string = malloc(pid_max_length * sizeof(char)); > + pid_bytes = read(fd, lock_pid_string, pid_max_length); > + if (pid_bytes > 0) { > + lock_pid = (pid_t) atoi(lock_pid_string); > + if (lock_pid > pid_max) > + invalid_lock = 1; > + else if (kill(lock_pid, 0) != 0) > + if (errno == ESRCH) > + invalid_lock = 1; > + > + if (!invalid_lock) { > + ERR(sh, "Could not create lock file at %s.", path); > + close(fd); > + free(pid_string); > + free(lock_pid_string); > + return -2; > + } > + } else > + invalid_lock = 1; > + > + if (invalid_lock) { > + pid_bytes = pwrite(fd, pid_string, pid_length, 0); > + close(fd); > + free(pid_string); > + free(lock_pid_string); > + if (pid_bytes == (ssize_t) pid_length) > + return 0; > + else { > + ERR(sh, "Could not create lock file at %s.", path); > + return -2; > + } > + } else { > + ERR(sh, "Could not create lock file at %s.", path); > + close(fd); > + free(pid_string); > + free(lock_pid_string); > + return -2; > + } > + } > + } > } > return 0; > } > @@ -1795,64 +1884,74 @@ int semanage_install_sandbox(semanage_ha > static int semanage_get_lock(semanage_handle_t * sh, > const char *lock_name, const char *lock_file) > { > + pid_t pid, lock_pid; > + char *pid_string, *lock_pid_string; > int fd; > - struct timeval origtime, curtime; > + size_t pid_length; > + ssize_t pid_bytes; > int got_lock = 0; > + int overwritten_lock = 0; > > - if ((fd = open(lock_file, O_RDONLY)) == -1) { > - if ((fd = > - open(lock_file, O_RDWR | O_CREAT | O_TRUNC, > - S_IRUSR | S_IWUSR)) == -1) { > + pid = getpid(); > + pid_string = malloc(pid_max_length * sizeof(char)); > + sprintf(pid_string, "%d", pid); > + pid_length = strlen(pid_string); > + if ((fd = open(lock_file, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR)) == -1) { > ERR(sh, "Could not open direct %s at %s.", lock_name, > lock_file); > + free(pid_string); > return -1; > + } else { > + lock_pid_string = malloc(pid_max_length * sizeof(char)); > + pid_bytes = pread(fd, lock_pid_string, pid_max_length, 0); > + if (pid_bytes == 0) { > + overwritten_lock = 1; > + pid_bytes = write(fd, pid_string, pid_length); > + if (pid_bytes == (ssize_t) pid_length) > + got_lock = 1; > + } else { > + lock_pid = (pid_t) atoi(lock_pid_string); > + if (lock_pid > pid_max) { > + overwritten_lock = 1; > + pid_bytes = pwrite(fd, pid_string, pid_length, 0); > + if (pid_bytes == (ssize_t) pid_length) > + got_lock = 1; > + } else { > + if (lock_pid == pid) > + got_lock = 1; > + else if (kill(lock_pid, 0) != 0) > + if (errno == ESRCH) { > + overwritten_lock = 1; > + pid_bytes = pwrite(fd, pid_string, pid_length, 0); > + if (pid_bytes == (ssize_t) pid_length) > + got_lock = 1; > + } > + } > } > } > - if (fcntl(fd, F_SETFD, FD_CLOEXEC) < 0) { > - ERR(sh, "Could not set close-on-exec for %s at %s.", lock_name, > - lock_file); > + > + if (!got_lock) { > close(fd); > + free(pid_string); > + free(lock_pid_string); > + if (overwritten_lock) > + unlink(lock_file); > + ERR(sh, "Could not get direct %s at %s.", lock_name, lock_file); > return -1; > } > > - if (sh->timeout == 0) { > - /* return immediately */ > - origtime.tv_sec = 0; > - } else { > - origtime.tv_sec = sh->timeout; > - } > - origtime.tv_usec = 0; > - do { > - curtime.tv_sec = 1; > - curtime.tv_usec = 0; > - if (flock(fd, LOCK_EX | LOCK_NB) == 0) { > - got_lock = 1; > - break; > - } else if (errno != EAGAIN) { > - ERR(sh, "Error obtaining direct %s at %s.", lock_name, > - lock_file); > - close(fd); > - return -1; > - } > - if (origtime.tv_sec > 0 || sh->timeout == -1) { > - if (select(0, NULL, NULL, NULL, &curtime) == -1) { > - if (errno == EINTR) { > - continue; > - } > - ERR(sh, > - "Error while waiting to get direct %s at %s.", > - lock_name, lock_file); > - close(fd); > - return -1; > - } > - origtime.tv_sec--; > - } > - } while (origtime.tv_sec > 0 || sh->timeout == -1); > - if (!got_lock) { > - ERR(sh, "Could not get direct %s at %s.", lock_name, lock_file); > + if (fcntl(fd, F_SETFD, FD_CLOEXEC) < 0) { > + ERR(sh, "Could not set close-on-exec for %s at %s.", lock_name, > + lock_file); > close(fd); > + free(pid_string); > + free(lock_pid_string); > + unlink(lock_file); > return -1; > } > + > + free(pid_string); > + free(lock_pid_string); > return fd; > } > > @@ -1901,29 +2000,27 @@ int semanage_get_active_lock(semanage_ha > } > } > > -/* Releases the transaction lock. Does nothing if there was not one already > - * there. */ > +/* Releases the transaction lock: the lock file is removed */ > void semanage_release_trans_lock(semanage_handle_t * sh) > { > int errsv = errno; > if (sh->u.direct.translock_file_fd >= 0) { > - flock(sh->u.direct.translock_file_fd, LOCK_UN); > close(sh->u.direct.translock_file_fd); > sh->u.direct.translock_file_fd = -1; > } > + unlink(semanage_files[SEMANAGE_TRANS_LOCK]); > errno = errsv; > } > > -/* Releases the read lock. Does nothing if there was not one already > - * there. */ > +/* Releases the read lock: the lock file is removed */ > void semanage_release_active_lock(semanage_handle_t * sh) > { > int errsv = errno; > if (sh->u.direct.activelock_file_fd >= 0) { > - flock(sh->u.direct.activelock_file_fd, LOCK_UN); > close(sh->u.direct.activelock_file_fd); > sh->u.direct.activelock_file_fd = -1; > } > + unlink(semanage_files[SEMANAGE_READ_LOCK]); > errno = errsv; > } > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] libsemanage: remove lock files 2017-04-26 12:03 ` Jason Zaman @ 2017-04-26 12:56 ` Stephen Smalley 2017-04-26 18:13 ` Guido Trentalancia 0 siblings, 1 reply; 19+ messages in thread From: Stephen Smalley @ 2017-04-26 12:56 UTC (permalink / raw) To: Jason Zaman, Guido Trentalancia; +Cc: selinux On Wed, 2017-04-26 at 20:03 +0800, Jason Zaman wrote: > On Tue, Apr 25, 2017 at 10:35:17PM +0200, Guido Trentalancia wrote: > > Do not use flock() for file locking, but instead use generic text > > files > > that keep track of the process ID (PID) of the locking process. > > > > Remove semanage read and transaction lock files upon releasing > > them. > > > > This third version fixes a bug in the previous version and also > > applies > > cleanly to the latest git tree. > > > > Signed-off-by: Guido Trentalancia <guido@trentalancia.net> > > --- > > src/Makefile | 2 > > src/semanage_store.c | 214 +++++++++++++++++++++++++++++++++++++- > > ------------- > > 2 files changed, 160 insertions(+), 56 deletions(-) > > > > --- a/src/Makefile 2017-04-25 22:27:38.105555427 +0200 > > +++ b/src/Makefile 2017-04-25 22:28:58.512555098 +0200 > > @@ -91,7 +91,7 @@ $(LIBA): $(OBJS) > > $(RANLIB) $@ > > > > $(LIBSO): $(LOBJS) > > - $(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lsepol > > -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version- > > script=libsemanage.map,-z,defs > > + $(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lm -lsepol > > -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version- > > script=libsemanage.map,-z,defs > > ln -sf $@ $(TARGET) > > > > $(LIBPC): $(LIBPC).in ../VERSION > > --- a/src/semanage_store.c 2017-04-20 16:30:21.218209972 > > +0200 > > +++ b/src/semanage_store.c 2017-04-25 22:24:35.883556172 > > +0200 > > @@ -45,6 +45,8 @@ typedef struct dbase_policydb dbase_t; > > #include <dirent.h> > > #include <errno.h> > > #include <fcntl.h> > > +#include <math.h> > > +#include <signal.h> > > #include <stdio.h> > > #include <stdio_ext.h> > > #include <stdlib.h> > > @@ -52,11 +54,24 @@ typedef struct dbase_policydb dbase_t; > > #include <unistd.h> > > #include <sys/file.h> > > #include <sys/stat.h> > > +#include <sys/sysctl.h> > > #include <sys/types.h> > > #include <sys/wait.h> > > #include <limits.h> > > #include <libgen.h> > > > > +#include <linux/sysctl.h> > > + > > +#ifndef CONFIG_BASE_SMALL > > +#define CONFIG_BASE_SMALL 0 > > +#endif > > + > > +#include <linux/threads.h> > > + > > +#ifndef PID_MAX_DEFAULT > > +#define PID_MAX_DEFAULT 32768 > > +#endif > > + > > #include "debug.h" > > #include "utilities.h" > > > > @@ -76,6 +91,8 @@ enum semanage_file_defs { > > static char > > *semanage_paths[SEMANAGE_NUM_STORES][SEMANAGE_STORE_NUM_PATHS]; > > static char *semanage_files[SEMANAGE_NUM_FILES] = { NULL }; > > static int semanage_paths_initialized = 0; > > +static int pid_max; > > +static ssize_t pid_max_length; > > > > /* These are paths relative to the bottom of the module store */ > > static const char *semanage_relative_files[SEMANAGE_NUM_FILES] = { > > @@ -427,8 +442,23 @@ cleanup: > > int semanage_check_init(semanage_handle_t *sh, const char *prefix) > > { > > int rc; > > + int fd; > > + char root[PATH_MAX]; > > + ssize_t amount_read; > > + > > if (semanage_paths_initialized == 0) { > > - char root[PATH_MAX]; > > + pid_max = PID_MAX_DEFAULT; > > + pid_max_length = ceil(log10(PID_MAX_DEFAULT + 1)); > > + > > + fd = open("/proc/sys/kernel/pid_max", O_RDONLY); > > + if (fd > 0) { > > + char sysctlstring[pid_max_length]; > > + amount_read = read(fd, sysctlstring, > > pid_max_length); > > + if (amount_read > 0) { > > + pid_max = atoi(sysctlstring); > > + pid_max_length = > > ceil(log10(pid_max + 1)); > > + } > > + } > > > > rc = snprintf(root, > > sizeof(root), > > @@ -528,16 +558,23 @@ char *semanage_conf_path(void) > > > > /**************** functions that create module store > > ***************/ > > > > -/* Check that the semanage store exists. If 'create' is non-zero > > then > > - * create the directories. Returns 0 if module store exists > > (either > > - * already or just created), -1 if does not exist or could not be > > - * read, or -2 if it could not create the store. */ > > +/* Check that the semanage store exists and that the read lock can > > be > > + * taken. If 'create' is non-zero then it creates the directories > > + * and the lock file. Returns 0 if the module store exists > > (either > > + * already or just created) and the read lock can be taken, -1 if > > it > > + * does not exist or it is not possible to read from it, or -2 if > > it > > + * could not create the store or it could not take the lock file. > > */ > > int semanage_create_store(semanage_handle_t * sh, int create) > > { > > struct stat sb; > > int mode_mask = R_OK | W_OK | X_OK; > > const char *path = semanage_files[SEMANAGE_ROOT]; > > int fd; > > + pid_t pid, lock_pid; > > + char *pid_string, *lock_pid_string; > > + size_t pid_length; > > + ssize_t pid_bytes; > > + int invalid_lock = 0; > > > > if (stat(path, &sb) == -1) { > > if (errno == ENOENT && create) { > > @@ -607,24 +644,81 @@ int semanage_create_store(semanage_handl > > return -1; > > } > > } > > + pid = getpid(); > > + pid_string = malloc(pid_max_length * sizeof(char)); > > + sprintf(pid_string, "%d", pid); > > + pid_length = strlen(pid_string); > > path = semanage_files[SEMANAGE_READ_LOCK]; > > if (stat(path, &sb) == -1) { > > if (errno == ENOENT && create) { > > if ((fd = creat(path, S_IRUSR | S_IWUSR)) > > == -1) { > > ERR(sh, "Could not create lock > > file at %s.", > > path); > > + free(pid_string); > > return -2; > > } > > + pid_bytes = write(fd, pid_string, > > pid_length); > > I'm pretty sure there is a race here. and another one between stat() > and > creat(). you can have two processes create and truncate the file then > one write on top of the other. leaving lock files around is > definitely > the safest option. and are 0 bytes so its not like you're wasting any > disk space. > > Overall I disagree with changing this just because a random file is > left > around. NFS may be a legit reason to change but as russel said, I > think > locks work somewhat over NFS too. Also, NFS mounting /home or /usr is > common but /var is pretty commonly not NFS mounted. I agree; absent a demonstration of an actual problem with the current libsemanage locking scheme, let's leave it alone. Keeping around two empty files is not a problem and this is a fairly well-established paradigm for locking on Linux. flock(2) works over NFS since Linux 2.6.12. > > -- Jason > > > close(fd); > > + free(pid_string); > > + if (pid_bytes == (ssize_t) pid_length) > > + return 0; > > + else { > > + ERR(sh, "Could not create lock > > file at %s.", path); > > + return -2; > > + } > > } else { > > ERR(sh, "Could not read lock file at %s.", > > path); > > + free(pid_string); > > return -1; > > } > > } else { > > if (!S_ISREG(sb.st_mode) || access(path, R_OK | > > W_OK) == -1) { > > ERR(sh, "Could not access lock file at > > %s.", path); > > + free(pid_string); > > return -1; > > - } > > + } else { > > + fd = open(path, O_RDWR); > > + if (fd > 0) { > > + lock_pid_string = > > malloc(pid_max_length * sizeof(char)); > > + pid_bytes = read(fd, > > lock_pid_string, pid_max_length); > > + if (pid_bytes > 0) { > > + lock_pid = (pid_t) > > atoi(lock_pid_string); > > + if (lock_pid > pid_max) > > + invalid_lock = 1; > > + else if (kill(lock_pid, 0) > > != 0) > > + if (errno == > > ESRCH) > > + invalid_lo > > ck = 1; > > + > > + if (!invalid_lock) { > > + ERR(sh, "Could not > > create lock file at %s.", path); > > + close(fd); > > + free(pid_string); > > + free(lock_pid_stri > > ng); > > + return -2; > > + } > > + } else > > + invalid_lock = 1; > > + > > + if (invalid_lock) { > > + pid_bytes = pwrite(fd, > > pid_string, pid_length, 0); > > + close(fd); > > + free(pid_string); > > + free(lock_pid_string); > > + if (pid_bytes == (ssize_t) > > pid_length) > > + return 0; > > + else { > > + ERR(sh, "Could not > > create lock file at %s.", path); > > + return -2; > > + } > > + } else { > > + ERR(sh, "Could not create > > lock file at %s.", path); > > + close(fd); > > + free(pid_string); > > + free(lock_pid_string); > > + return -2; > > + } > > + } > > + } > > } > > return 0; > > } > > @@ -1795,64 +1884,74 @@ int semanage_install_sandbox(semanage_ha > > static int semanage_get_lock(semanage_handle_t * sh, > > const char *lock_name, const char > > *lock_file) > > { > > + pid_t pid, lock_pid; > > + char *pid_string, *lock_pid_string; > > int fd; > > - struct timeval origtime, curtime; > > + size_t pid_length; > > + ssize_t pid_bytes; > > int got_lock = 0; > > + int overwritten_lock = 0; > > > > - if ((fd = open(lock_file, O_RDONLY)) == -1) { > > - if ((fd = > > - open(lock_file, O_RDWR | O_CREAT | O_TRUNC, > > - S_IRUSR | S_IWUSR)) == -1) { > > + pid = getpid(); > > + pid_string = malloc(pid_max_length * sizeof(char)); > > + sprintf(pid_string, "%d", pid); > > + pid_length = strlen(pid_string); > > + if ((fd = open(lock_file, O_RDWR | O_CREAT, S_IRUSR | > > S_IWUSR)) == -1) { > > ERR(sh, "Could not open direct %s at %s.", > > lock_name, > > lock_file); > > + free(pid_string); > > return -1; > > + } else { > > + lock_pid_string = malloc(pid_max_length * > > sizeof(char)); > > + pid_bytes = pread(fd, lock_pid_string, > > pid_max_length, 0); > > + if (pid_bytes == 0) { > > + overwritten_lock = 1; > > + pid_bytes = write(fd, pid_string, > > pid_length); > > + if (pid_bytes == (ssize_t) pid_length) > > + got_lock = 1; > > + } else { > > + lock_pid = (pid_t) atoi(lock_pid_string); > > + if (lock_pid > pid_max) { > > + overwritten_lock = 1; > > + pid_bytes = pwrite(fd, pid_string, > > pid_length, 0); > > + if (pid_bytes == (ssize_t) > > pid_length) > > + got_lock = 1; > > + } else { > > + if (lock_pid == pid) > > + got_lock = 1; > > + else if (kill(lock_pid, 0) != 0) > > + if (errno == ESRCH) { > > + overwritten_lock = > > 1; > > + pid_bytes = > > pwrite(fd, pid_string, pid_length, 0); > > + if (pid_bytes == > > (ssize_t) pid_length) > > + got_lock = > > 1; > > + } > > + } > > } > > } > > - if (fcntl(fd, F_SETFD, FD_CLOEXEC) < 0) { > > - ERR(sh, "Could not set close-on-exec for %s at > > %s.", lock_name, > > - lock_file); > > + > > + if (!got_lock) { > > close(fd); > > + free(pid_string); > > + free(lock_pid_string); > > + if (overwritten_lock) > > + unlink(lock_file); > > + ERR(sh, "Could not get direct %s at %s.", > > lock_name, lock_file); > > return -1; > > } > > > > - if (sh->timeout == 0) { > > - /* return immediately */ > > - origtime.tv_sec = 0; > > - } else { > > - origtime.tv_sec = sh->timeout; > > - } > > - origtime.tv_usec = 0; > > - do { > > - curtime.tv_sec = 1; > > - curtime.tv_usec = 0; > > - if (flock(fd, LOCK_EX | LOCK_NB) == 0) { > > - got_lock = 1; > > - break; > > - } else if (errno != EAGAIN) { > > - ERR(sh, "Error obtaining direct %s at > > %s.", lock_name, > > - lock_file); > > - close(fd); > > - return -1; > > - } > > - if (origtime.tv_sec > 0 || sh->timeout == -1) { > > - if (select(0, NULL, NULL, NULL, &curtime) > > == -1) { > > - if (errno == EINTR) { > > - continue; > > - } > > - ERR(sh, > > - "Error while waiting to get > > direct %s at %s.", > > - lock_name, lock_file); > > - close(fd); > > - return -1; > > - } > > - origtime.tv_sec--; > > - } > > - } while (origtime.tv_sec > 0 || sh->timeout == -1); > > - if (!got_lock) { > > - ERR(sh, "Could not get direct %s at %s.", > > lock_name, lock_file); > > + if (fcntl(fd, F_SETFD, FD_CLOEXEC) < 0) { > > + ERR(sh, "Could not set close-on-exec for %s at > > %s.", lock_name, > > + lock_file); > > close(fd); > > + free(pid_string); > > + free(lock_pid_string); > > + unlink(lock_file); > > return -1; > > } > > + > > + free(pid_string); > > + free(lock_pid_string); > > return fd; > > } > > > > @@ -1901,29 +2000,27 @@ int semanage_get_active_lock(semanage_ha > > } > > } > > > > -/* Releases the transaction lock. Does nothing if there was not > > one already > > - * there. */ > > +/* Releases the transaction lock: the lock file is removed */ > > void semanage_release_trans_lock(semanage_handle_t * sh) > > { > > int errsv = errno; > > if (sh->u.direct.translock_file_fd >= 0) { > > - flock(sh->u.direct.translock_file_fd, LOCK_UN); > > close(sh->u.direct.translock_file_fd); > > sh->u.direct.translock_file_fd = -1; > > } > > + unlink(semanage_files[SEMANAGE_TRANS_LOCK]); > > errno = errsv; > > } > > > > -/* Releases the read lock. Does nothing if there was not one > > already > > - * there. */ > > +/* Releases the read lock: the lock file is removed */ > > void semanage_release_active_lock(semanage_handle_t * sh) > > { > > int errsv = errno; > > if (sh->u.direct.activelock_file_fd >= 0) { > > - flock(sh->u.direct.activelock_file_fd, LOCK_UN); > > close(sh->u.direct.activelock_file_fd); > > sh->u.direct.activelock_file_fd = -1; > > } > > + unlink(semanage_files[SEMANAGE_READ_LOCK]); > > errno = errsv; > > } > > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] libsemanage: remove lock files 2017-04-26 12:56 ` Stephen Smalley @ 2017-04-26 18:13 ` Guido Trentalancia 2017-04-26 18:25 ` Stephen Smalley 0 siblings, 1 reply; 19+ messages in thread From: Guido Trentalancia @ 2017-04-26 18:13 UTC (permalink / raw) To: selinux Hello. On Wed, 26/04/2017 at 08.56 -0400, Stephen Smalley wrote: > On Wed, 2017-04-26 at 20:03 +0800, Jason Zaman wrote: > > On Tue, Apr 25, 2017 at 10:35:17PM +0200, Guido Trentalancia wrote: > > > Do not use flock() for file locking, but instead use generic text > > > files > > > that keep track of the process ID (PID) of the locking process. > > > > > > Remove semanage read and transaction lock files upon releasing > > > them. > > > > > > This third version fixes a bug in the previous version and also > > > applies > > > cleanly to the latest git tree. > > > > > > Signed-off-by: Guido Trentalancia <guido@trentalancia.net> > > > --- > > > src/Makefile | 2 > > > src/semanage_store.c | 214 > > > +++++++++++++++++++++++++++++++++++++- > > > ------------- > > > 2 files changed, 160 insertions(+), 56 deletions(-) > > > > > > --- a/src/Makefile 2017-04-25 22:27:38.105555427 +0200 > > > +++ b/src/Makefile 2017-04-25 22:28:58.512555098 +0200 > > > @@ -91,7 +91,7 @@ $(LIBA): $(OBJS) > > > $(RANLIB) $@ > > > > > > $(LIBSO): $(LOBJS) > > > - $(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lsepol > > > -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version- > > > script=libsemanage.map,-z,defs > > > + $(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lm -lsepol > > > -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version- > > > script=libsemanage.map,-z,defs > > > ln -sf $@ $(TARGET) > > > > > > $(LIBPC): $(LIBPC).in ../VERSION > > > --- a/src/semanage_store.c 2017-04-20 16:30:21.218209972 > > > +0200 > > > +++ b/src/semanage_store.c 2017-04-25 22:24:35.883556172 > > > +0200 > > > @@ -45,6 +45,8 @@ typedef struct dbase_policydb dbase_t; > > > #include <dirent.h> > > > #include <errno.h> > > > #include <fcntl.h> > > > +#include <math.h> > > > +#include <signal.h> > > > #include <stdio.h> > > > #include <stdio_ext.h> > > > #include <stdlib.h> > > > @@ -52,11 +54,24 @@ typedef struct dbase_policydb dbase_t; > > > #include <unistd.h> > > > #include <sys/file.h> > > > #include <sys/stat.h> > > > +#include <sys/sysctl.h> > > > #include <sys/types.h> > > > #include <sys/wait.h> > > > #include <limits.h> > > > #include <libgen.h> > > > > > > +#include <linux/sysctl.h> > > > + > > > +#ifndef CONFIG_BASE_SMALL > > > +#define CONFIG_BASE_SMALL 0 > > > +#endif > > > + > > > +#include <linux/threads.h> > > > + > > > +#ifndef PID_MAX_DEFAULT > > > +#define PID_MAX_DEFAULT 32768 > > > +#endif > > > + > > > #include "debug.h" > > > #include "utilities.h" > > > > > > @@ -76,6 +91,8 @@ enum semanage_file_defs { > > > static char > > > *semanage_paths[SEMANAGE_NUM_STORES][SEMANAGE_STORE_NUM_PATHS]; > > > static char *semanage_files[SEMANAGE_NUM_FILES] = { NULL }; > > > static int semanage_paths_initialized = 0; > > > +static int pid_max; > > > +static ssize_t pid_max_length; > > > > > > /* These are paths relative to the bottom of the module store */ > > > static const char *semanage_relative_files[SEMANAGE_NUM_FILES] = > > > { > > > @@ -427,8 +442,23 @@ cleanup: > > > int semanage_check_init(semanage_handle_t *sh, const char > > > *prefix) > > > { > > > int rc; > > > + int fd; > > > + char root[PATH_MAX]; > > > + ssize_t amount_read; > > > + > > > if (semanage_paths_initialized == 0) { > > > - char root[PATH_MAX]; > > > + pid_max = PID_MAX_DEFAULT; > > > + pid_max_length = ceil(log10(PID_MAX_DEFAULT + > > > 1)); > > > + > > > + fd = open("/proc/sys/kernel/pid_max", O_RDONLY); > > > + if (fd > 0) { > > > + char sysctlstring[pid_max_length]; > > > + amount_read = read(fd, sysctlstring, > > > pid_max_length); > > > + if (amount_read > 0) { > > > + pid_max = atoi(sysctlstring); > > > + pid_max_length = > > > ceil(log10(pid_max + 1)); > > > + } > > > + } > > > > > > rc = snprintf(root, > > > sizeof(root), > > > @@ -528,16 +558,23 @@ char *semanage_conf_path(void) > > > > > > /**************** functions that create module store > > > ***************/ > > > > > > -/* Check that the semanage store exists. If 'create' is non- > > > zero > > > then > > > - * create the directories. Returns 0 if module store exists > > > (either > > > - * already or just created), -1 if does not exist or could not > > > be > > > - * read, or -2 if it could not create the store. */ > > > +/* Check that the semanage store exists and that the read lock > > > can > > > be > > > + * taken. If 'create' is non-zero then it creates the > > > directories > > > + * and the lock file. Returns 0 if the module store exists > > > (either > > > + * already or just created) and the read lock can be taken, -1 > > > if > > > it > > > + * does not exist or it is not possible to read from it, or -2 > > > if > > > it > > > + * could not create the store or it could not take the lock > > > file. > > > */ > > > int semanage_create_store(semanage_handle_t * sh, int create) > > > { > > > struct stat sb; > > > int mode_mask = R_OK | W_OK | X_OK; > > > const char *path = semanage_files[SEMANAGE_ROOT]; > > > int fd; > > > + pid_t pid, lock_pid; > > > + char *pid_string, *lock_pid_string; > > > + size_t pid_length; > > > + ssize_t pid_bytes; > > > + int invalid_lock = 0; > > > > > > if (stat(path, &sb) == -1) { > > > if (errno == ENOENT && create) { > > > @@ -607,24 +644,81 @@ int semanage_create_store(semanage_handl > > > return -1; > > > } > > > } > > > + pid = getpid(); > > > + pid_string = malloc(pid_max_length * sizeof(char)); > > > + sprintf(pid_string, "%d", pid); > > > + pid_length = strlen(pid_string); > > > path = semanage_files[SEMANAGE_READ_LOCK]; > > > if (stat(path, &sb) == -1) { > > > if (errno == ENOENT && create) { > > > if ((fd = creat(path, S_IRUSR | > > > S_IWUSR)) > > > == -1) { > > > ERR(sh, "Could not create lock > > > file at %s.", > > > path); > > > + free(pid_string); > > > return -2; > > > } > > > + pid_bytes = write(fd, pid_string, > > > pid_length); > > > > I'm pretty sure there is a race here. and another one between > > stat() > > and > > creat(). you can have two processes create and truncate the file > > then > > one write on top of the other. leaving lock files around is > > definitely > > the safest option. and are 0 bytes so its not like you're wasting > > any > > disk space. > > > > Overall I disagree with changing this just because a random file is > > left > > around. NFS may be a legit reason to change but as russel said, I > > think > > locks work somewhat over NFS too. Also, NFS mounting /home or /usr > > is > > common but /var is pretty commonly not NFS mounted. > > I agree; absent a demonstration of an actual problem with the current > libsemanage locking scheme, let's leave it alone. Keeping around two > empty files is not a problem and this is a fairly well-established > paradigm for locking on Linux. flock(2) works over NFS since Linux > 2.6.12. If I am not wrong, I found the information on the following web page: https://linux.die.net/man/2/flock It looks like a manual page, but I don't know if that information is obsolete or not (there's no date, that's the bad side of some of the web). There are other reports online, I have not tested so I cannot tell. As for the problem pointed out by Jason, I have improved it by removing the code that leads to the other race condition: if you need a revised version (v4) of the patch, please let me know, otherwise I take there isn't enough interest in the change or the information is wrong/outdated. Regards, Guido ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] libsemanage: remove lock files 2017-04-26 18:13 ` Guido Trentalancia @ 2017-04-26 18:25 ` Stephen Smalley 0 siblings, 0 replies; 19+ messages in thread From: Stephen Smalley @ 2017-04-26 18:25 UTC (permalink / raw) To: Guido Trentalancia, selinux On Wed, 2017-04-26 at 20:13 +0200, Guido Trentalancia wrote: > Hello. > > On Wed, 26/04/2017 at 08.56 -0400, Stephen Smalley wrote: > > On Wed, 2017-04-26 at 20:03 +0800, Jason Zaman wrote: > > > On Tue, Apr 25, 2017 at 10:35:17PM +0200, Guido Trentalancia > > > wrote: > > > > Do not use flock() for file locking, but instead use generic > > > > text > > > > files > > > > that keep track of the process ID (PID) of the locking process. > > > > > > > > Remove semanage read and transaction lock files upon releasing > > > > them. > > > > > > > > This third version fixes a bug in the previous version and also > > > > applies > > > > cleanly to the latest git tree. > > > > > > > > Signed-off-by: Guido Trentalancia <guido@trentalancia.net> > > > > --- > > > > src/Makefile | 2 > > > > src/semanage_store.c | 214 > > > > +++++++++++++++++++++++++++++++++++++- > > > > ------------- > > > > 2 files changed, 160 insertions(+), 56 deletions(-) > > > > > > > > --- a/src/Makefile 2017-04-25 22:27:38.105555427 +0200 > > > > +++ b/src/Makefile 2017-04-25 22:28:58.512555098 +0200 > > > > @@ -91,7 +91,7 @@ $(LIBA): $(OBJS) > > > > $(RANLIB) $@ > > > > > > > > $(LIBSO): $(LOBJS) > > > > - $(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lsepol > > > > -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version- > > > > script=libsemanage.map,-z,defs > > > > + $(CC) $(CFLAGS) $(LDFLAGS) -shared -o $@ $^ -lm > > > > -lsepol > > > > -laudit -lselinux -lbz2 -Wl,-soname,$(LIBSO),--version- > > > > script=libsemanage.map,-z,defs > > > > ln -sf $@ $(TARGET) > > > > > > > > $(LIBPC): $(LIBPC).in ../VERSION > > > > --- a/src/semanage_store.c 2017-04-20 16:30:21.218209972 > > > > +0200 > > > > +++ b/src/semanage_store.c 2017-04-25 22:24:35.883556172 > > > > +0200 > > > > @@ -45,6 +45,8 @@ typedef struct dbase_policydb dbase_t; > > > > #include <dirent.h> > > > > #include <errno.h> > > > > #include <fcntl.h> > > > > +#include <math.h> > > > > +#include <signal.h> > > > > #include <stdio.h> > > > > #include <stdio_ext.h> > > > > #include <stdlib.h> > > > > @@ -52,11 +54,24 @@ typedef struct dbase_policydb dbase_t; > > > > #include <unistd.h> > > > > #include <sys/file.h> > > > > #include <sys/stat.h> > > > > +#include <sys/sysctl.h> > > > > #include <sys/types.h> > > > > #include <sys/wait.h> > > > > #include <limits.h> > > > > #include <libgen.h> > > > > > > > > +#include <linux/sysctl.h> > > > > + > > > > +#ifndef CONFIG_BASE_SMALL > > > > +#define CONFIG_BASE_SMALL 0 > > > > +#endif > > > > + > > > > +#include <linux/threads.h> > > > > + > > > > +#ifndef PID_MAX_DEFAULT > > > > +#define PID_MAX_DEFAULT 32768 > > > > +#endif > > > > + > > > > #include "debug.h" > > > > #include "utilities.h" > > > > > > > > @@ -76,6 +91,8 @@ enum semanage_file_defs { > > > > static char > > > > *semanage_paths[SEMANAGE_NUM_STORES][SEMANAGE_STORE_NUM_PATHS]; > > > > static char *semanage_files[SEMANAGE_NUM_FILES] = { NULL }; > > > > static int semanage_paths_initialized = 0; > > > > +static int pid_max; > > > > +static ssize_t pid_max_length; > > > > > > > > /* These are paths relative to the bottom of the module store > > > > */ > > > > static const char *semanage_relative_files[SEMANAGE_NUM_FILES] > > > > = > > > > { > > > > @@ -427,8 +442,23 @@ cleanup: > > > > int semanage_check_init(semanage_handle_t *sh, const char > > > > *prefix) > > > > { > > > > int rc; > > > > + int fd; > > > > + char root[PATH_MAX]; > > > > + ssize_t amount_read; > > > > + > > > > if (semanage_paths_initialized == 0) { > > > > - char root[PATH_MAX]; > > > > + pid_max = PID_MAX_DEFAULT; > > > > + pid_max_length = ceil(log10(PID_MAX_DEFAULT + > > > > 1)); > > > > + > > > > + fd = open("/proc/sys/kernel/pid_max", > > > > O_RDONLY); > > > > + if (fd > 0) { > > > > + char sysctlstring[pid_max_length]; > > > > + amount_read = read(fd, sysctlstring, > > > > pid_max_length); > > > > + if (amount_read > 0) { > > > > + pid_max = atoi(sysctlstring); > > > > + pid_max_length = > > > > ceil(log10(pid_max + 1)); > > > > + } > > > > + } > > > > > > > > rc = snprintf(root, > > > > sizeof(root), > > > > @@ -528,16 +558,23 @@ char *semanage_conf_path(void) > > > > > > > > /**************** functions that create module store > > > > ***************/ > > > > > > > > -/* Check that the semanage store exists. If 'create' is non- > > > > zero > > > > then > > > > - * create the directories. Returns 0 if module store exists > > > > (either > > > > - * already or just created), -1 if does not exist or could not > > > > be > > > > - * read, or -2 if it could not create the store. */ > > > > +/* Check that the semanage store exists and that the read lock > > > > can > > > > be > > > > + * taken. If 'create' is non-zero then it creates the > > > > directories > > > > + * and the lock file. Returns 0 if the module store exists > > > > (either > > > > + * already or just created) and the read lock can be taken, -1 > > > > if > > > > it > > > > + * does not exist or it is not possible to read from it, or -2 > > > > if > > > > it > > > > + * could not create the store or it could not take the lock > > > > file. > > > > */ > > > > int semanage_create_store(semanage_handle_t * sh, int create) > > > > { > > > > struct stat sb; > > > > int mode_mask = R_OK | W_OK | X_OK; > > > > const char *path = semanage_files[SEMANAGE_ROOT]; > > > > int fd; > > > > + pid_t pid, lock_pid; > > > > + char *pid_string, *lock_pid_string; > > > > + size_t pid_length; > > > > + ssize_t pid_bytes; > > > > + int invalid_lock = 0; > > > > > > > > if (stat(path, &sb) == -1) { > > > > if (errno == ENOENT && create) { > > > > @@ -607,24 +644,81 @@ int semanage_create_store(semanage_handl > > > > return -1; > > > > } > > > > } > > > > + pid = getpid(); > > > > + pid_string = malloc(pid_max_length * sizeof(char)); > > > > + sprintf(pid_string, "%d", pid); > > > > + pid_length = strlen(pid_string); > > > > path = semanage_files[SEMANAGE_READ_LOCK]; > > > > if (stat(path, &sb) == -1) { > > > > if (errno == ENOENT && create) { > > > > if ((fd = creat(path, S_IRUSR | > > > > S_IWUSR)) > > > > == -1) { > > > > ERR(sh, "Could not create lock > > > > file at %s.", > > > > path); > > > > + free(pid_string); > > > > return -2; > > > > } > > > > + pid_bytes = write(fd, pid_string, > > > > pid_length); > > > > > > I'm pretty sure there is a race here. and another one between > > > stat() > > > and > > > creat(). you can have two processes create and truncate the file > > > then > > > one write on top of the other. leaving lock files around is > > > definitely > > > the safest option. and are 0 bytes so its not like you're wasting > > > any > > > disk space. > > > > > > Overall I disagree with changing this just because a random file > > > is > > > left > > > around. NFS may be a legit reason to change but as russel said, I > > > think > > > locks work somewhat over NFS too. Also, NFS mounting /home or > > > /usr > > > is > > > common but /var is pretty commonly not NFS mounted. > > > > I agree; absent a demonstration of an actual problem with the > > current > > libsemanage locking scheme, let's leave it alone. Keeping around > > two > > empty files is not a problem and this is a fairly well-established > > paradigm for locking on Linux. flock(2) works over NFS since Linux > > 2.6.12. > > If I am not wrong, I found the information on the following web page: > > https://linux.die.net/man/2/flock > > It looks like a manual page, but I don't know if that information is > obsolete or not (there's no date, that's the bad side of some of the > web). It is out of date. See: http://man7.org/linux/man-pages/man2/flock.2.html > There are other reports online, I have not tested so I cannot tell. > > As for the problem pointed out by Jason, I have improved it by > removing > the code that leads to the other race condition: if you need a > revised > version (v4) of the patch, please let me know, otherwise I take there > isn't enough interest in the change or the information is > wrong/outdated. Absent evidence of a problem, not interested. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] libsemanage: remove lock files 2017-04-25 20:06 ` [PATCH v2] " Guido Trentalancia 2017-04-25 20:35 ` [PATCH v3] " Guido Trentalancia @ 2017-04-26 0:31 ` Russell Coker 1 sibling, 0 replies; 19+ messages in thread From: Russell Coker @ 2017-04-26 0:31 UTC (permalink / raw) To: selinux On Wed, 26 Apr 2017 06:06:13 AM Guido Trentalancia wrote: > > 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. > > No, I didn't mean the complicate circumstance of NFS shared amongst > multiple systems. > > I only meant the simpler (and most common) situation where the NFS is > mounted for only one system, therefore PIDs are unique and there is no > need to include the hostname. flock(2) seems to indicate that locks always worked locally on NFS filesystems and thus would always have worked in that case. Please do some testing and prove that the problem occurs on NFS-root systems. > > Really pidfiles are so horrible that one of the noteworthy features > > of systemd > > is removing the need for them. > > I am not that pessimistic about locking with aid of PIDs. > > To be honest, the current situation has more drawbacks in my opinion. > > In particular, I don't like the fact that it leaves unused lock files > around the filesystem. Everything else that uses lock files does that. > > 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. > > Please forget sharing NFS. I meant dedicated NFS. > > Anyway, I have created a new version of the patch that should probably > improve the previous race condition. > > If you have a way of testing a dedicated store over NFS, then I'd like > to hear back from you the result of such testing ! > > But, if you are not interested in testing it, then never mind... I think that when someone wants to change behavior that is the same as used in many programs they should demonstrate that it has a problem. -- My Main Blog http://etbe.coker.com.au/ My Documents Blog http://doc.coker.com.au/ ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2017-04-26 18:25 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-20 14:38 [PATCH] libsemanage: remove lock files Guido Trentalancia 2017-04-20 15:44 ` Stephen Smalley 2017-04-20 15:45 ` Guido Trentalancia 2017-04-20 15:56 ` Stephen Smalley 2017-04-20 15:56 ` Guido Trentalancia 2017-04-20 16:08 ` Stephen Smalley 2017-04-20 16:09 ` Guido Trentalancia 2017-04-24 12:06 ` Alan Jenkins 2017-04-24 12:08 ` Alan Jenkins 2017-04-24 17:51 ` Guido Trentalancia 2017-04-24 18:38 ` Guido Trentalancia 2017-04-25 6:30 ` Russell Coker 2017-04-25 20:06 ` [PATCH v2] " Guido Trentalancia 2017-04-25 20:35 ` [PATCH v3] " Guido Trentalancia 2017-04-26 12:03 ` Jason Zaman 2017-04-26 12:56 ` Stephen Smalley 2017-04-26 18:13 ` Guido Trentalancia 2017-04-26 18:25 ` Stephen Smalley 2017-04-26 0:31 ` [PATCH v2] " Russell Coker
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.