restorecond matchpathcon patch

restorecond matchpathcon patch

[Steve G]

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

Hi,

I was investigating why restorecond consumes so much memory and noticed that
valgrind is reporting about 18mb of reachable memory at exit. It seems that
matchpathcon sits on a lot of memory that may or may not help restorcond. This
patch causes matchpathcon to free its memory after we get its results. 

This helps cleanup valgrind output so that leaks are confined to utmpter and
another list. But I have to wonder if the strategy of allocating memory for each
file in a directory is a good thing or if we can just depend on partial path
matching to decide if we should care about the file. 

IOW, this patch cleans up one problem, but I wonder if the underlying algorithm
should be changed to be les of a memory hog.

-Steve


 
____________________________________________________________________________________
TV dinner still cooling? 
Check out "Tonight's Picks" on Yahoo! TV.
http://tv.yahoo.com/

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 1515479032-policycoreutils-2.0.1-restorecond-leak.patch --]
[-- Type: text/x-patch; name="policycoreutils-2.0.1-restorecond-leak.patch", Size: 635 bytes --]

diff -urp policycoreutils-2.0.1.orig/restorecond/restorecond.c policycoreutils-2.0.1/restorecond/restorecond.c
--- policycoreutils-2.0.1.orig/restorecond/restorecond.c	2007-02-19 21:25:54.000000000 -0500
+++ policycoreutils-2.0.1/restorecond/restorecond.c	2007-02-19 21:28:09.000000000 -0500
@@ -188,7 +188,9 @@ static void restore(const char *filename
 		return;
 	}
 
-	if (matchpathcon(filename, st.st_mode, &scontext) < 0) {
+	retcontext = matchpathcon(filename, st.st_mode, &scontext);
+	matchpathcon_fini();
+	if (retcontext < 0) {
 		if (errno == ENOENT)
 			return;
 		syslog(LOG_ERR, "matchpathcon(%s) failed %s\n", filename,

Re: restorecond matchpathcon patch

[Stephen Smalley]

On Tue, 2007-02-20 at 07:33 -0800, Steve G wrote:
> Hi,
> 
> I was investigating why restorecond consumes so much memory and noticed that
> valgrind is reporting about 18mb of reachable memory at exit. It seems that
> matchpathcon sits on a lot of memory that may or may not help restorcond. This
> patch causes matchpathcon to free its memory after we get its results. 
> 
> This helps cleanup valgrind output so that leaks are confined to utmpter and
> another list. But I have to wonder if the strategy of allocating memory for each
> file in a directory is a good thing or if we can just depend on partial path
> matching to decide if we should care about the file. 
> 
> IOW, this patch cleans up one problem, but I wonder if the underlying algorithm
> should be changed to be les of a memory hog.

Possibly the FCGlob work will help address that problem.
http://selinux-symposium.org/2007/abstracts.php#fcglob
James Athey (cc'd) - what is the implementation status and prospects for
code submission?

Calling matchpathcon_fini each time will require re-creation of the
entire in-memory table from the file contexts configuration on each
matchpathcon call, so you are trading off runtime performance for memory
here.  Normally, upon the first matchpathcon call or an explicit
matchpathcon_init call, libselinux loads the file contexts configuration
from the relevant files and builds the in-memory table, including
compiling the pathname regexes to internal form, so that subsequent
matchpathcon calls can just walk the in-memory table and call regexec on
the precompiled expressions.  That memory should be stable though after
initialization - it shouldn't grow over time.  The only memory that
would grow over time might be the translated context that is cached on
first use by matchpathcon, although matchpathcon then frees the raw
context so you'd only grow by the difference between the two (if the
translated context was longer).

-- 
Stephen Smalley
National Security Agency


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

Re: restorecond matchpathcon patch

[Steve G]

>> IOW, this patch cleans up one problem, but I wonder if the underlying
>> algorithm should be changed to be less of a memory hog.
>
>Possibly the FCGlob work will help address that problem.

Is that restorecond work or libselinux work? The change in algorithm I was
thinking about was in restorecond. IOW, it could cache the correct context for
the handful of files it cares about and use matchpathcon only for the paths that
contain metacharacters. It could monitor for policy reloads and correct its cache
in that event. For what its doing, it consumes a lot of memory as is.

>Calling matchpathcon_fini each time will require re-creation of the
>entire in-memory table from the file contexts configuration on each
>matchpathcon call, so you are trading off runtime performance for memory
>here.

If you want to leave it alone, we should have something like this added to
restorecond.c:

@@ -483,6 +483,7 @@ int main(int argc, char **argv)

        watch_list_free(master_fd);
        close(master_fd);
+       matchpathcon_fini();
        if (pidfile)
                unlink(pidfile);

so that valgrind can spot real memory leaks. Besides matchpathcon, valgrind is
reporting other memory issues.

-Steve


 
____________________________________________________________________________________
Any questions? Get answers on any topic at www.Answers.yahoo.com.  Try it now.

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

Re: restorecond matchpathcon patch

[Stephen Smalley]

On Tue, 2007-02-20 at 08:57 -0800, Steve G wrote:
> >> IOW, this patch cleans up one problem, but I wonder if the underlying
> >> algorithm should be changed to be less of a memory hog.
> >
> >Possibly the FCGlob work will help address that problem.
> 
> Is that restorecond work or libselinux work? The change in algorithm I was
> thinking about was in restorecond. IOW, it could cache the correct context for
> the handful of files it cares about and use matchpathcon only for the paths that
> contain metacharacters. It could monitor for policy reloads and correct its cache
> in that event. For what its doing, it consumes a lot of memory as is.

libselinux work - replacing the use of pathname regexes in the file
contexts configuration with a simpler globbing syntax.  It doesn't
matter how many times restorecond calls matchpathcon (and restorecond is
just passing full pathnames to it, which matchpathcon then matches
against the table of pathname regexes to find the right context); the
issue is the memory consumed by the precompiled regexes loaded from the
file contexts configuration on the very first matchpathcon call.

> >Calling matchpathcon_fini each time will require re-creation of the
> >entire in-memory table from the file contexts configuration on each
> >matchpathcon call, so you are trading off runtime performance for memory
> >here.
> 
> If you want to leave it alone, we should have something like this added to
> restorecond.c:
> 
> @@ -483,6 +483,7 @@ int main(int argc, char **argv)
> 
>         watch_list_free(master_fd);
>         close(master_fd);
> +       matchpathcon_fini();
>         if (pidfile)
>                 unlink(pidfile);
> 
> so that valgrind can spot real memory leaks. Besides matchpathcon, valgrind is
> reporting other memory issues.

Yes, that would be fine.

-- 
Stephen Smalley
National Security Agency


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

RE: restorecond matchpathcon patch

[James Athey]

> Possibly the FCGlob work will help address that problem.
> http://selinux-symposium.org/2007/abstracts.php#fcglob
> James Athey (cc'd) - what is the implementation status and prospects for
> code submission?

Here are the tasks we're working on, in order:

1. FCGlob reader and analyzer, written in Python.
2. Translate Reference Policy's fc files into FCGlob, using the results of step 1 to read and compile them.  After compilation, offer to convert the FCGlob specifications back to regular expressions, so that the resulting policy will work on systems with pre-FCGlob toolchains.
3. Update the toolchain to support FCGlob, incorporating the lessons learned from steps 1 and 2.

We're currently working on step 1.  Hence, the toolchain patches will take a little while to appear.  It's great to see that we've gotten the community's attention with our proposal, and we'll be eager to get everyone's thoughts on the work as it's completed.  We'll also be presenting our paper on the subject at this year's SELinux Symposium.

For those of you who haven't heard of FCGlob, it's a replacement syntax for file specifications that looks a lot like shell globbing.  It's a lot easier to write FCGlob specifications, because the syntax is much cleaner than regular expressions, as well as more familiar to most people.  It includes some common-sense rules that make it possible to treat file contexts as sets (in a computer science sense), so it becomes easy to matchpathcon files, find conflicts between specifications, and even display the file system as specified by the file contexts in the policy.

~James Athey




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